Re: [Qemu-devel] [RFC PATCH 0/3] add QEMU_WARN_NONNULL_ARGS() macro

[Philippe Mathieu-Daudé]

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