Stefan Weil <s...@weilnetz.de> writes:
> Am 13.08.18 um 19:52 schrieb Aleksandar Markovic:
>
>> From: Aleksandar Markovic <amarko...@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.
