Re: [PATCH] async: Suppress GCC13 false positive in aio_bh_poll()

[Juan Quintela <quintela@redhat.com>]

Cédric Le Goater <clg@kaod.org> wrote:
> From: Cédric Le Goater <clg@redhat.com>
>
> GCC13 reports an error :
>
> ../util/async.c: In function ‘aio_bh_poll’:
> include/qemu/queue.h:303:22: error: storing the address of local
> variable ‘slice’ in ‘*ctx.bh_slice_list.sqh_last’
> [-Werror=dangling-pointer=]
>   303 |     (head)->sqh_last = &(elm)->field.sqe_next;                          \
>       |     ~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~
> ../util/async.c:169:5: note: in expansion of macro ‘QSIMPLEQ_INSERT_TAIL’
>   169 |     QSIMPLEQ_INSERT_TAIL(&ctx->bh_slice_list, &slice, next);
>       |     ^~~~~~~~~~~~~~~~~~~~
> ../util/async.c:161:17: note: ‘slice’ declared here
>   161 |     BHListSlice slice;
>       |                 ^~~~~
> ../util/async.c:161:17: note: ‘ctx’ declared here
>
> But the local variable 'slice' is removed from the global context list
> in following loop of the same routine. Add a pragma to silent GCC.
>
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Daniel P. Berrangé <berrange@redhat.com>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
>  util/async.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/util/async.c b/util/async.c
> index 21016a1ac7..856e1a8a33 100644
> --- a/util/async.c
> +++ b/util/async.c
> @@ -164,7 +164,21 @@ int aio_bh_poll(AioContext *ctx)
>  
>      /* Synchronizes with QSLIST_INSERT_HEAD_ATOMIC in aio_bh_enqueue().  */
>      QSLIST_MOVE_ATOMIC(&slice.bh_list, &ctx->bh_list);
> +
> +    /*
> +     * GCC13 [-Werror=dangling-pointer=] complains that the local variable
> +     * 'slice' is being stored in the global 'ctx->bh_slice_list' but the
> +     * list is emptied before this function returns.
> +     */
> +#if !defined(__clang__)
> +#pragma GCC diagnostic push
> +#pragma GCC diagnostic ignored "-Wpragmas"
> +#pragma GCC diagnostic ignored "-Wdangling-pointer="
> +#endif
>      QSIMPLEQ_INSERT_TAIL(&ctx->bh_slice_list, &slice, next);
> +#if !defined(__clang__)
> +#pragma GCC diagnostic pop
> +#endif
>  
>      while ((s = QSIMPLEQ_FIRST(&ctx->bh_slice_list))) {
>          QEMUBH *bh;

I know, I know.

I like to make fun of the compiler as the next guy.  But it is not
simpler this other change, just put the variable in the heap?

Later, Juan.


From bb5792a6763a451c72ef5cfd78b09032689f54e5 Mon Sep 17 00:00:00 2001
From: Juan Quintela <quintela@redhat.com>
Date: Tue, 25 Apr 2023 15:19:11 +0200
Subject: [PATCH] Silent GCC13 warning

Gcc complains about putting a local variable on a global list, not
noticing that we remove the whole list before leaving the function.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 util/async.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/util/async.c b/util/async.c
index 21016a1ac7..7a8432e9e9 100644
--- a/util/async.c
+++ b/util/async.c
@@ -158,13 +158,17 @@ void aio_bh_call(QEMUBH *bh)
 /* Multiple occurrences of aio_bh_poll cannot be called concurrently. */
 int aio_bh_poll(AioContext *ctx)
 {
-    BHListSlice slice;
+    /*
+     * gcc13 complains about putting a local variable
+     * in a global list, so put it on the heap.
+     */
+    g_autofree BHListSlice *slice = g_new(BHListSlice, 1);
     BHListSlice *s;
     int ret = 0;
 
     /* Synchronizes with QSLIST_INSERT_HEAD_ATOMIC in aio_bh_enqueue().  */
-    QSLIST_MOVE_ATOMIC(&slice.bh_list, &ctx->bh_list);
-    QSIMPLEQ_INSERT_TAIL(&ctx->bh_slice_list, &slice, next);
+    QSLIST_MOVE_ATOMIC(&slice->bh_list, &ctx->bh_list);
+    QSIMPLEQ_INSERT_TAIL(&ctx->bh_slice_list, slice, next);
 
     while ((s = QSIMPLEQ_FIRST(&ctx->bh_slice_list))) {
         QEMUBH *bh;
-- 
2.40.0