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(-)
--
2.15.1
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.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
© 2016 - 2025 Red Hat, Inc.