[Qemu-devel] [PATCH] disas/libvixl: Fix VIXL_FALLTHROUGH macro for QEMU

[Qemu-devel] [PATCH] disas/libvixl: Fix VIXL_FALLTHROUGH macro for QEMU

[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
-- 
2.11.0

Re: [Qemu-devel] [PATCH] disas/libvixl: Fix VIXL_FALLTHROUGH macro for QEMU

[Stefan Weil]

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.

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;

Re: [Qemu-devel] [PATCH] disas/libvixl: Fix VIXL_FALLTHROUGH macro for QEMU

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH] disas/libvixl: Fix VIXL_FALLTHROUGH macro for QEMU

[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

Re: [Qemu-devel] [PATCH] disas/libvixl: Fix VIXL_FALLTHROUGH macro for QEMU

[Stefan Weil]

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

Re: [Qemu-devel] [PATCH] disas/libvixl: Fix VIXL_FALLTHROUGH macro for QEMU

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH] disas/libvixl: Fix VIXL_FALLTHROUGH macro for QEMU

[Richard Henderson]

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~

Re: [Qemu-devel] [PATCH] disas/libvixl: Fix VIXL_FALLTHROUGH macro for QEMU

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH] disas/libvixl: Fix VIXL_FALLTHROUGH macro for QEMU

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH] disas/libvixl: Fix VIXL_FALLTHROUGH macro for QEMU

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH] disas/libvixl: Fix VIXL_FALLTHROUGH macro for QEMU

[Richard Henderson]

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~