| Message ID | cc3f26a7-a8b7-35a8-9ffa-c5940c9c1927@weilnetz.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 13 May 2018 at 10:57, Stefan Weil <sw@weilnetz.de> wrote: > Am 13.05.2018 um 11:06 schrieb Stefan Weil: >> It now prevents compiler warnings (enabled with -Wimplicit-fallthrough= >> or -Wextra) as intended. >> >> Signed-off-by: Stefan Weil <sw@weilnetz.de> >> --- >> >> I suggest to add and use a similar macro QEMU_FALLTHROUGH() >> for the rest of the code and can provide a patch if that's >> fine for everyone. >> >> Regards >> Stefan >> >> disas/libvixl/vixl/globals.h | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/disas/libvixl/vixl/globals.h b/disas/libvixl/vixl/globals.h >> index 61dc9f7f7e..33c4231d91 100644 >> --- a/disas/libvixl/vixl/globals.h >> +++ b/disas/libvixl/vixl/globals.h >> @@ -112,6 +112,8 @@ inline void USE(T1, T2, T3, T4) {} >> // C++11(201103L). >> #if __has_warning("-Wimplicit-fallthrough") && __cplusplus >= 201103L >> #define VIXL_FALLTHROUGH() [[clang::fallthrough]] //NOLINT >> +#elif defined(__GNUC__) >> + #define VIXL_FALLTHROUGH() __attribute__((fallthrough)) >> #else >> #define VIXL_FALLTHROUGH() do {} while (0) >> #endif > > > Even with the above patch, disas/libvixl raises a compiler warning for a > fall through case. The patch below fixes that warning, but I am not sure > whether a fall through is correct there. This sort of question is probably best asked of upstream VIXL, rather than here... thanks -- PMM
On 13 May 2018 at 10:57, Stefan Weil <sw@weilnetz.de> wrote: > Even with the above patch, disas/libvixl raises a compiler warning for a > fall through case. The patch below fixes that warning, but I am not sure > whether a fall through is correct there. > > Stefan > > > diff --git a/disas/libvixl/vixl/a64/disasm-a64.cc > b/disas/libvixl/vixl/a64/disasm-a64.cc > index 7a58a5c087..5481d94209 100644 > --- a/disas/libvixl/vixl/a64/disasm-a64.cc > +++ b/disas/libvixl/vixl/a64/disasm-a64.cc > @@ -2986,6 +2986,7 @@ int Disassembler::SubstituteImmediateField(const > Instruction* instr, > return 3; > } > } > + VIXL_FALLTHROUGH(); // ??? > } > case 'C': { // ICondB - Immediate Conditional Branch. > int64_t offset = instr->ImmCondBranch() << 2; This is fixed in upstream vixl, in fact: https://git.linaro.org/arm/vixl.git/commit/?id=de326f850f736c3a337fda52845ed3d2e620cc02 thanks -- PMM
Am 15.05.2018 um 14:13 schrieb Peter Maydell: > On 13 May 2018 at 10:57, Stefan Weil <sw@weilnetz.de> wrote: >> Even with the above patch, disas/libvixl raises a compiler warning for a >> fall through case. The patch below fixes that warning, but I am not sure >> whether a fall through is correct there. >> >> Stefan >> >> >> diff --git a/disas/libvixl/vixl/a64/disasm-a64.cc >> b/disas/libvixl/vixl/a64/disasm-a64.cc >> index 7a58a5c087..5481d94209 100644 >> --- a/disas/libvixl/vixl/a64/disasm-a64.cc >> +++ b/disas/libvixl/vixl/a64/disasm-a64.cc >> @@ -2986,6 +2986,7 @@ int Disassembler::SubstituteImmediateField(const >> Instruction* instr, >> return 3; >> } >> } >> + VIXL_FALLTHROUGH(); // ??? >> } >> case 'C': { // ICondB - Immediate Conditional Branch. >> int64_t offset = instr->ImmCondBranch() << 2; > This is fixed in upstream vixl, in fact: > > https://git.linaro.org/arm/vixl.git/commit/?id=de326f850f736c3a337fda52845ed3d2e620cc02 > > thanks > -- PMM That fix will work for the moment, but is not sufficient for the future because it only supports gcc 7.x, but not gcc 8.x or later: +#elif __GNUC__ == 7 How do we proceed with the code in QEMU? Do you have a plan to update the vixl code? As vixl is obviously no longer maintained on GitHub, I am not sure whom I could contact. And what about my other question / suggestion: "I suggest to add and use a similar macro QEMU_FALLTHROUGH() for the rest of the code and can provide a patch if that's fine for everyone." gcc gives lots of fallthrough warnings, and many code locations don't contain a comment stating that the fall through is fine. Thanks Stefan
On 15 May 2018 at 14:13, Stefan Weil <sw@weilnetz.de> wrote: > This is fixed in upstream vixl, in fact: > > https://git.linaro.org/arm/vixl.git/commit/?id=de326f850f736c3a337fda52845ed3d2e620cc02 > > That fix will work for the moment, but is not sufficient for the future > because it only supports gcc 7.x, but not gcc 8.x or later: > > +#elif __GNUC__ == 7 > > How do we proceed with the code in QEMU? Do you have a plan to update the > vixl code? As vixl is obviously no longer maintained on GitHub, I am not > sure whom I could contact. The github page points you to the linaro repo which is the new upstream. That said, I think we're planning to deprecate vixl now we have the capstone support. Richard, what's the status here? Could we just remove the vixl code now? > And what about my other question / suggestion: > > "I suggest to add and use a similar macro QEMU_FALLTHROUGH() for the rest of > the code and can provide a patch if that's fine for everyone." > > gcc gives lots of fallthrough warnings, and many code locations don't > contain a comment stating that the fall through is fine. I guess that having the compiler check is better than finding them later with coverity. It's a shame gcc doesn't support the standard mechanism of using /* fallthrough */ to mark these, though. thanks -- PMM
On 05/15/2018 06:25 AM, Peter Maydell wrote: > On 15 May 2018 at 14:13, Stefan Weil <sw@weilnetz.de> wrote: >> This is fixed in upstream vixl, in fact: >> >> https://git.linaro.org/arm/vixl.git/commit/?id=de326f850f736c3a337fda52845ed3d2e620cc02 >> >> That fix will work for the moment, but is not sufficient for the future >> because it only supports gcc 7.x, but not gcc 8.x or later: >> >> +#elif __GNUC__ == 7 >> >> How do we proceed with the code in QEMU? Do you have a plan to update the >> vixl code? As vixl is obviously no longer maintained on GitHub, I am not >> sure whom I could contact. > > The github page points you to the linaro repo which is the new upstream. > That said, I think we're planning to deprecate vixl now we have the > capstone support. Richard, what's the status here? Could we just remove > the vixl code now? We could just remove vixl, yes. I'd like to see updates to capstone to support instructions post v8.0, but it's not like we have those with vixl either... > I guess that having the compiler check is better than finding them > later with coverity. It's a shame gcc doesn't support the standard > mechanism of using /* fallthrough */ to mark these, though. It does. Apparently not by default anymore, however: @item @option{-Wimplicit-fallthrough=0} disables the warning altogether. @item @option{-Wimplicit-fallthrough=1} matches @code{.*} regular expression, any comment is used as fallthrough comment. @item @option{-Wimplicit-fallthrough=2} case insensitively matches @code{.*falls?[ \t-]*thr(ough|u).*} regular expression. @item @option{-Wimplicit-fallthrough=3} case sensitively matches one of the following regular expressions: ... I think either =2 or =1 would work for us in QEMU. r~
On 15 May 2018 at 15:46, Richard Henderson <rth@twiddle.net> wrote: > On 05/15/2018 06:25 AM, Peter Maydell wrote: >> I guess that having the compiler check is better than finding them >> later with coverity. It's a shame gcc doesn't support the standard >> mechanism of using /* fallthrough */ to mark these, though. > > It does. Apparently not by default anymore, however: > > @item @option{-Wimplicit-fallthrough=0} disables the warning altogether. > > @item @option{-Wimplicit-fallthrough=1} matches @code{.*} regular > expression, any comment is used as fallthrough comment. > > @item @option{-Wimplicit-fallthrough=2} case insensitively matches > @code{.*falls?[ \t-]*thr(ough|u).*} regular expression. > > @item @option{-Wimplicit-fallthrough=3} case sensitively matches one of the > following regular expressions: > ... > > I think either =2 or =1 would work for us in QEMU. 1 sounds too broad, we don't want any old comment to count. 2 is probably what we want. thanks -- PMM
On 15 May 2018 at 14:13, Stefan Weil <sw@weilnetz.de> wrote: > That fix will work for the moment, but is not sufficient for the future > because it only supports gcc 7.x, but not gcc 8.x or later: > > +#elif __GNUC__ == 7 Fix to make that use >= is currently going through upstream vixl code review: https://review.linaro.org/#/c/25282/ thanks -- PMM
On 15 May 2018 at 15:46, Richard Henderson <rth@twiddle.net> wrote: > On 05/15/2018 06:25 AM, Peter Maydell wrote: >> That said, I think we're planning to deprecate vixl now we have the >> capstone support. Richard, what's the status here? Could we just remove >> the vixl code now? > > We could just remove vixl, yes. I'd like to see updates to capstone to support > instructions post v8.0, but it's not like we have those with vixl either... Just to check my understanding: with QEMU at the moment, you always get the capstone disassembler unless you specifically turn it off by passing --disable-capstone to configure, right (since we provide it as a submodule)? We put that in in September last year, and we haven't had a pile of complaints about the disassembly (or indeed any complaints that I can recall), so I think we can consider it a success, and remove both vixl and the ancient binutils arm disassembler. It would also be interesting to try interacting with capstone upstream about adding support for newer instructions (for instance they don't do the v8M insns). Do you know if capstone deals with new insns via resync from LLVM or if they've entirely forked and just make changes locally by hand? thanks -- PMM
On 05/18/2018 03:34 AM, Peter Maydell wrote: > On 15 May 2018 at 15:46, Richard Henderson <rth@twiddle.net> wrote: >> On 05/15/2018 06:25 AM, Peter Maydell wrote: >>> That said, I think we're planning to deprecate vixl now we have the >>> capstone support. Richard, what's the status here? Could we just remove >>> the vixl code now? >> >> We could just remove vixl, yes. I'd like to see updates to capstone to support >> instructions post v8.0, but it's not like we have those with vixl either... > > Just to check my understanding: with QEMU at the moment, you > always get the capstone disassembler unless you specifically > turn it off by passing --disable-capstone to configure, right > (since we provide it as a submodule)? Correct. > We put that in in September last year, and we haven't had a > pile of complaints about the disassembly (or indeed any > complaints that I can recall), so I think we can consider it > a success, and remove both vixl and the ancient binutils arm > disassembler. Yep. > It would also be interesting to try interacting with capstone > upstream about adding support for newer instructions (for > instance they don't do the v8M insns). Do you know if capstone > deals with new insns via resync from LLVM or if they've > entirely forked and just make changes locally by hand? They seem to have entirely forked, but I'm not completely sure. r~
diff --git a/disas/libvixl/vixl/a64/disasm-a64.cc b/disas/libvixl/vixl/a64/disasm-a64.cc index 7a58a5c087..5481d94209 100644 --- a/disas/libvixl/vixl/a64/disasm-a64.cc +++ b/disas/libvixl/vixl/a64/disasm-a64.cc @@ -2986,6 +2986,7 @@ int Disassembler::SubstituteImmediateField(const Instruction* instr, return 3; } } + VIXL_FALLTHROUGH(); // ??? } case 'C': { // ICondB - Immediate Conditional Branch.