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