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

[Markus Armbruster <armbru@redhat.com>]

Stefan Weil <sw@weilnetz.de> writes:

> Am 13.08.18 um 19:52 schrieb Aleksandar Markovic:
>
>> From: Aleksandar Markovic <amarkovic@wavecomp.com>
>>
>> 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.
>
> 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.

I'd treat that as a bug.

> - There is also fallthrough code which is obviously not correct (even
> in target/mips/translate.c).

Bug.

> 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))

In my opinion, the macro is no clearer than proper comments.

I'd prefer -Wimplicit-fallthrough=1 or 2.  The former makes gcc accept
any comment.  The latter makes it accept '.*falls?[ \t-]*thr(ough|u).*',
which should still match the majority of our comments.  Less churn than
the macro.

> 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)))

Do we define our own scanf()-like functions?  If yes, decorating them
with the attribute is a good idea.

However, the gnu_ in gnu_scanf tells the compiler we're linking with the
GNU C Library, which seems unwise.  Hmm, we already use gnu_printf.
Commit 9c9e7d51bf0:

    Newer gcc versions support format gnu_printf which is
    better suited for use in QEMU than format printf
    (QEMU always uses standard format strings (even with mingw32)).

Should we limit the use of gnu_printf to #ifdef _WIN32?

> A more regular solution would require renaming GCC_FMT_ATTR to
> GCC_FMT_PRINTF and use GCC_FMT_SCANF for the new macro.

Quite some churn, but regularity matters.