Am 08.07.19 um 06:40 schrieb Markus Armbruster:
Stefan Weil <s...@weilnetz.de> 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
