Re: [Qemu-devel] Handling of fall through code (was: [PATCH v8 04/87] target/mips: Mark switch fallthroughs with interpretable comments

[Aleksandar Markovic]

On Jul 7, 2019 10:26 PM, "Stefan Weil" <address@hidden> wrote:
>
> Am 13.08.18 um 19:52 schrieb Aleksandar Markovic:
>
>> From: Aleksandar Markovic <address@hidden>
>>
>> Mark switch fallthroughs with comments, in cases fallthroughs
>> are intentional.
>
>
>
> This is a general problem all over the QEMU code. I usually compile with
nearly all warnings enabled and get now lots of errors with the latest code
and after updating to gcc-8.3.0 (Debian buster). It should be reproducible
by enabling -Werror=implicit-fallthrough.
>

Hi, Stefan.

Thanks for bringing this to the attention of the community.

I am sure there is a number of nasty bugs of this nature in our code - yet
to be found.

> The current situation is like this:
>
> - Some code has fallthrough comments which are accepted by the compiler.
>
> - Other code has fallthrough comments which are not accepted (resulting
in a compiler error).
>
> - Some code is correct, but has no indication that the fallthrough is
intentional.
>
> - There is also fallthrough code which is obviously not correct (even in
target/mips/translate.c).

Can you please be more specific about those cases from
target/mips/translate.c?

Yours,
Aleksandar

>
>
> I suggest to enable -Werror=implicit-fallthrough by default and add a new
macro to mark all fallthrough locations which are correct, but not accepted
by the compiler.
>
> This can be done with a definition for GCC compatible compilers in
include/qemu/compiler.h:
>
> #define QEMU_FALLTHROUGH __attribute__ ((fallthrough))
>
> Then fallthrough code would look like this:
>
>     case 1:
>         do_something();
>         QEMU_FALLTHROUGH;
>
>     case 2:
>
>
> VIXL_FALLTHROUGH also needs a similar definition to work with gcc-8.3.0.
>
> Please comment. Would you prefer another macro name or a macro with
parentheses like this:
>
> #define QEMU_FALLTHROUGH() __attribute__ ((fallthrough))
>
>
> As soon as there is consensus on the macro name and form, I can send a
patch which adds it (but would not mind if someone else adds it).
>
> Then I suggest that the maintainers build with the fallthrough warning
enabled and fix all warnings, either by using the new macro or by adding
the missing break.
>
> Finally we can enforce the warning by default.
>
>
> Another macro which is currently missing is a scanf variant of
GCC_FMT_ATTR.
>
> I suggest to add and use a GCC_SCANF_ATTR macro:
>
> #define GCC_SCANF_ATTR(n, m) __attribute__((format(gnu_scanf, n, m)))
>
> A more regular solution would require renaming GCC_FMT_ATTR to
GCC_FMT_PRINTF and use GCC_FMT_SCANF for the new macro.
>
>
> Regards
> Stefan Weil
>
>
>