Message ID | 20180117131821.18700-1-f4bug@amsat.org |
---|---|
Headers | show |
Series | add QEMU_WARN_NONNULL_ARGS() macro | expand |
On 01/17/2018 10:18 AM, Philippe Mathieu-Daudé wrote: > Some old PoC series I remember after reading > http://lists.nongnu.org/archive/html/qemu-devel/2018-01/msg03545.html > > I had few more changes but then I found the code was harder to read > so I didn't continue further. > Only 2 patches are included as example. > > This might still be useful in few cases, so I'm still sending as RFC > to have different thoughts. > > This macro is, however, helpful to the Clang static analizer (reducing > false positive). > > BTW another useful macro for the static analizer I used is: > > #define QEMU_FALLTHROUGH __attribute__((fallthrough)) and: #define QEMU_STATIC_ANALYSIS_ASSERT(expression) assert(expression) i.e.: linux-user/syscall.c:5581:9: warning: Dereference of undefined pointer value if (*host_rt_dev_ptr != 0) { ^~~~~~~~~~~~~~~~ This can safely be silenced using: unlock_user(argptr, arg, 0); + QEMU_STATIC_ANALYSIS_ASSERT(host_rt_dev_ptr); ret = get_errno(safe_ioctl(fd, ie->host_cmd, buf_temp)); if (*host_rt_dev_ptr != 0) { ... Or: target/mips/msa_helper.c:1008:23: warning: The result of the '<<' expression is undefined int64_t r_bit = 1 << (DF_BITS(df) - 2); ~~^~~~~~~~~~~~~~~~~~~~ Using: static inline int64_t msa_mulr_q_df(uint32_t df, int64_t arg1, int64_t arg2) { int64_t q_min = DF_MIN_INT(df); int64_t q_max = DF_MAX_INT(df); - int64_t r_bit = 1 << (DF_BITS(df) - 2); + int64_t r_bit; + QEMU_STATIC_ANALYSIS_ASSERT(DF_BITS(df) < 32); + r_bit = 1 << (DF_BITS(df) - 2); Similar: target/arm/helper.c:4283:25: warning: The result of the '>>' expression is undefined len = cto32(bas >> basstart); ~~~~^~~~~~~~~~~ Using: basstart = ctz32(bas); +QEMU_STATIC_ANALYSIS_ASSERT(basstart <= 8); /* bas is at most 8-bit */ len = cto32(bas >> basstart); Another: monitor.c:1481:26: warning: Access to field 'name' results in a dereference of a null pointer (loaded from variable 'mr') addr, mr->name, ptr); ^~~~~~~~ Using: if (local_err) { error_report_err(local_err); return; + } else { + QEMU_STATIC_ANALYSIS_ASSERT(ptr != NULL); } physaddr = vtop(ptr, &local_err); if (local_err) { error_report_err(local_err); } else { monitor_printf(mon, "Host physical address for 0x%" HWADDR_PRIx " (%s) is 0x%" PRIx64 "\n", addr, mr->name, (uint64_t) physaddr); etc.. Too many false positive leads to unused report, but personally I still prefer readable code. > > It replaces the /* fall through */ comment, i.e.: > > switch (rap) { > case BCR_SWS: > if (!(CSR_STOP(s) || CSR_SPND(s))) > return; > val &= ~0x0300; > QEMU_FALLTHROUGH; > case BCR_LNKST: > case BCR_LED1: > case BCR_LED2: > case BCR_LED3: > case BCR_MC: > case BCR_FDC: > case BCR_BSBC: > case BCR_EECAS: > case BCR_PLAT: > s->bcr[rap] = val; > break; > > Regards, > > Phil. > > Philippe Mathieu-Daudé (3): > compiler: add QEMU_WARN_NONNULL_ARGS() > virtio: let virtio_add/clear_feature() use QEMU_WARN_NONNULL_ARGS() > utils: let qemu_find_file() use QEMU_WARN_NONNULL_ARGS() > > include/hw/virtio/virtio.h | 2 ++ > include/qemu-common.h | 2 +- > include/qemu/compiler.h | 2 ++ > 3 files changed, 5 insertions(+), 1 deletion(-) >
On 01/17/2018 05:18 AM, Philippe Mathieu-Daudé wrote: > BTW another useful macro for the static analizer I used is: > > #define QEMU_FALLTHROUGH __attribute__((fallthrough)) > > It replaces the /* fall through */ comment, i.e.: That's unfortunate. Does it help if you use the actual lint spelling of /* FALLTHRU */? r~
On 01/17/2018 09:36 AM, Richard Henderson wrote: > On 01/17/2018 05:18 AM, Philippe Mathieu-Daudé wrote: >> BTW another useful macro for the static analizer I used is: >> >> #define QEMU_FALLTHROUGH __attribute__((fallthrough)) >> >> It replaces the /* fall through */ comment, i.e.: > > That's unfortunate. Does it help if you use the actual lint spelling of /* > FALLTHRU */? New gcc has this: '-Wimplicit-fallthrough=N' Warn when a switch case falls through. For example: ... Since there are occasions where a switch case fall through is desirable, GCC provides an attribute, '__attribute__ ((fallthrough))', that is to be used along with a null statement to suppress this warning that would normally occur: switch (cond) { case 1: bar (0); __attribute__ ((fallthrough)); default: ... } C++17 provides a standard way to suppress the '-Wimplicit-fallthrough' warning using '[[fallthrough]];' instead of the GNU attribute. In C++11 or C++14 users can use '[[gnu::fallthrough]];', which is a GNU extension. Instead of the these attributes, it is also possible to add a fallthrough comment to silence the warning. The whole body of the C or C++ style comment should match the given regular expressions listed below. The option argument N specifies what kind of comments are accepted: * '-Wimplicit-fallthrough=0' disables the warning altogether. * '-Wimplicit-fallthrough=1' matches '.*' regular expression, any comment is used as fallthrough comment. * '-Wimplicit-fallthrough=2' case insensitively matches '.*falls?[ \t-]*thr(ough|u).*' regular expression. * '-Wimplicit-fallthrough=3' case sensitively matches one of the following regular expressions: * '-fallthrough' * '@fallthrough@' * 'lint -fallthrough[ \t]*' * '[ \t.!]*(ELSE,? |INTENTIONAL(LY)? )? FALL(S | |-)?THR(OUGH|U)[ \t.!]*(-[^\n\r]*)?' * '[ \t.!]*(Else,? |Intentional(ly)? )? Fall((s | |-)[Tt]|t)hr(ough|u)[ \t.!]*(-[^\n\r]*)?' * '[ \t.!]*([Ee]lse,? |[Ii]ntentional(ly)? )? fall(s | |-)?thr(ough|u)[ \t.!]*(-[^\n\r]*)?' * '-Wimplicit-fallthrough=4' case sensitively matches one of the following regular expressions: * '-fallthrough' * '@fallthrough@' * 'lint -fallthrough[ \t]*' * '[ \t]*FALLTHR(OUGH|U)[ \t]*' * '-Wimplicit-fallthrough=5' doesn't recognize any comments as fallthrough comments, only attributes disable the warning. The comment needs to be followed after optional whitespace and other comments by 'case' or 'default' keywords or by a user label that precedes some 'case' or 'default' label. switch (cond) { case 1: bar (0); /* FALLTHRU */ default: ... } The '-Wimplicit-fallthrough=3' warning is enabled by '-Wextra'. No thanks to level 5, these days, you HAVE to use a macro that expands to the attribute and/or C17 spelling, rather than relying on the lint spelling, if you cannot control what level a user will request; although with level 3, the lint spelling of a comment still works. I'm not sure which static analyzers recognize which other spellings; which is why a macro that consistently expands to the same string known to work, rather than 20 different ad hoc comments (many of which work, but auditing that all of them work is a bigger task), may be worthwhile.