Re: [Qemu-devel] Handling of fall through code

[Stefan Weil]

Am 08.07.19 um 06:40 schrieb Markus Armbruster:

> Stefan Weil <address@hidden> writes:
>
> > - Some code is correct, but has no indication that the fallthrough is
> > intentional.
> I'd treat that as a bug.


Sure.


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


Yes, of course.


> > 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.
[...]
> 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.


xen_device_backend_scanf, xs_node_vscanf, xs_node_scanf, 
xen_device_frontend_scanf

Maybe more. The compiler can tell you missing attributes.


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


No, because we don't want lots of conditional code with different format 
strings for POSIX and Windows (I made that commit 9 years ago).


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


I could do that when adding the new macro, but would like to hear more 
opinions on that.

Thank you,

Stefan