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