async: Suppress GCC13 false positive in aio_bh_poll()

Commit Message

Cédric Le Goater April 20, 2023, 8:29 p.m. UTC

From: Cédric Le Goater <[e-mail]>

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 <[e-mail]>
Cc: Paolo Bonzini <[e-mail]>
Cc: Daniel P. Berrangé <[e-mail]>
Signed-off-by: Cédric Le Goater <[e-mail]>
---
 util/async.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

Comments

Cédric Le Goater April 20, 2023, 8:36 p.m. UTC | #1

+ Φλ

On 4/20/23 22:29, Cédric Le Goater wrote:
> From: Cédric Le Goater <[e-mail]>
> 
> 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 <[e-mail]>
> Cc: Paolo Bonzini <[e-mail]>
> Cc: Daniel P. Berrangé <[e-mail]>
> Signed-off-by: Cédric Le Goater <[e-mail]>
> ---
>   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;

Daniel Henrique Barboza April 20, 2023, 9:07 p.m. UTC | #2

On 4/20/23 17:29, Cédric Le Goater wrote:
> From: Cédric Le Goater <[e-mail]>
> 
> 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 <[e-mail]>
> Cc: Paolo Bonzini <[e-mail]>
> Cc: Daniel P. Berrangé <[e-mail]>
> Signed-off-by: Cédric Le Goater <[e-mail]>
> ---


Reviewed-by: Daniel Henrique Barboza <[e-mail]>
Tested-by: Daniel Henrique Barboza <[e-mail]>


If no one opposes I'll queue this patch, and the following 2 already reviewed
patches, in ppc-next:

[PATCH for-8.0 v2 3/3] target/ppc: Fix helper_pminsn() prototype
[PATCH for-8.0 v2 2/3] target/s390x: Fix float_comp_to_cc() prototype


The reason is that I updated to Fedora 38 too soon and became aggravated by
these GCC13 false positives.



Thanks,


Daniel



>   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;

Daniel Henrique Barboza April 20, 2023, 9:28 p.m. UTC | #3

On 4/20/23 18:07, Daniel Henrique Barboza wrote:
> 
> 
> On 4/20/23 17:29, Cédric Le Goater wrote:
>> From: Cédric Le Goater <[e-mail]>
>>
>> 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 <[e-mail]>
>> Cc: Paolo Bonzini <[e-mail]>
>> Cc: Daniel P. Berrangé <[e-mail]>
>> Signed-off-by: Cédric Le Goater <[e-mail]>
>> ---
> 
> 
> Reviewed-by: Daniel Henrique Barboza <[e-mail]>
> Tested-by: Daniel Henrique Barboza <[e-mail]>
> 
> 
> If no one opposes I'll queue this patch, and the following 2 already reviewed
> patches, in ppc-next:
> 
> [PATCH for-8.0 v2 3/3] target/ppc: Fix helper_pminsn() prototype
> [PATCH for-8.0 v2 2/3] target/s390x: Fix float_comp_to_cc() prototype


Nevermind, these 2 patches are already applied. We're missing just this one.



Daniel

> 
> 
> The reason is that I updated to Fedora 38 too soon and became aggravated by
> these GCC13 false positives.
> 
> 
> 
> Thanks,
> 
> 
> Daniel
> 
> 
> 
>>   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;

Philippe Mathieu-Daudé April 21, 2023, 6:08 a.m. UTC | #4

On 20/4/23 22:29, Cédric Le Goater wrote:
> From: Cédric Le Goater <[e-mail]>
> 
> 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 <[e-mail]>
> Cc: Paolo Bonzini <[e-mail]>
> Cc: Daniel P. Berrangé <[e-mail]>
> Signed-off-by: Cédric Le Goater <[e-mail]>
> ---
>   util/async.c | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)

Reviewed-by: Philippe Mathieu-Daudé <[e-mail]>

Stefan Hajnoczi April 22, 2023, 2:06 p.m. UTC | #5

On Thu, 20 Apr 2023 at 16:31, Cédric Le Goater <[e-mail]> wrote:
>
> From: Cédric Le Goater <[e-mail]>
>
> 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 <[e-mail]>
> Cc: Paolo Bonzini <[e-mail]>
> Cc: Daniel P. Berrangé <[e-mail]>
> Signed-off-by: Cédric Le Goater <[e-mail]>
> ---
>  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

Reviewed-by: Stefan Hajnoczi <[e-mail]>

Thomas Huth April 24, 2023, 6:33 a.m. UTC | #6

On 20/04/2023 22.29, Cédric Le Goater wrote:
> From: Cédric Le Goater <[e-mail]>
> 
> 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 <[e-mail]>
> Cc: Paolo Bonzini <[e-mail]>
> Cc: Daniel P. Berrangé <[e-mail]>
> Signed-off-by: Cédric Le Goater <[e-mail]>
> ---
>   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"

Why do we need to ignore -Wpragmas here?

> +#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;

Anyway:
Reviewed-by: Thomas Huth <[e-mail]>

Cédric Le Goater April 24, 2023, 6:54 a.m. UTC | #7

On 4/24/23 08:33, Thomas Huth wrote:
> On 20/04/2023 22.29, Cédric Le Goater wrote:
>> From: Cédric Le Goater <[e-mail]>
>>
>> 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 <[e-mail]>
>> Cc: Paolo Bonzini <[e-mail]>
>> Cc: Daniel P. Berrangé <[e-mail]>
>> Signed-off-by: Cédric Le Goater <[e-mail]>
>> ---
>>   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"
> 
> Why do we need to ignore -Wpragmas here?

To handle GCC 8.5 as suggested by Philippe :

   https://lore.kernel.org/qemu-devel/[e-mail]/

> 
>> +#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;
> 
> Anyway:
> Reviewed-by: Thomas Huth <[e-mail]>
> 

Thanks,

C.

Juan Quintela April 25, 2023, 1:22 p.m. UTC | #8

Cédric Le Goater <[e-mail]> wrote:
> From: Cédric Le Goater <[e-mail]>
>
> 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 <[e-mail]>
> Cc: Paolo Bonzini <[e-mail]>
> Cc: Daniel P. Berrangé <[e-mail]>
> Signed-off-by: Cédric Le Goater <[e-mail]>
> ---
>  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 <[e-mail]>
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 <[e-mail]>
---
 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;

Daniel P. Berrangé April 25, 2023, 1:24 p.m. UTC | #9

On Tue, Apr 25, 2023 at 03:22:15PM +0200, Juan Quintela wrote:
> Cédric Le Goater <[e-mail]> wrote:
> > From: Cédric Le Goater <[e-mail]>
> >
> > 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 <[e-mail]>
> > Cc: Paolo Bonzini <[e-mail]>
> > Cc: Daniel P. Berrangé <[e-mail]>
> > Signed-off-by: Cédric Le Goater <[e-mail]>
> > ---
> >  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 <[e-mail]>
> 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 <[e-mail]>
> ---
>  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;

This must be a memory leak since you're adding a g_new but not
adding any g_free

With regards,
Daniel

Daniel P. Berrangé April 25, 2023, 1:30 p.m. UTC | #10

On Tue, Apr 25, 2023 at 02:24:59PM +0100, Daniel P. Berrangé wrote:
> On Tue, Apr 25, 2023 at 03:22:15PM +0200, Juan Quintela wrote:
> > Cédric Le Goater <[e-mail]> wrote:
> > > From: Cédric Le Goater <[e-mail]>
> > >
> > > 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 <[e-mail]>
> > > Cc: Paolo Bonzini <[e-mail]>
> > > Cc: Daniel P. Berrangé <[e-mail]>
> > > Signed-off-by: Cédric Le Goater <[e-mail]>
> > > ---
> > >  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 <[e-mail]>
> > 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 <[e-mail]>
> > ---
> >  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;
> 
> This must be a memory leak since you're adding a g_new but not
> adding any g_free

Sorry, I'm failing to read properly today. It uses g_autofree
so there is no leak.

With regards,
Daniel

Paolo Bonzini April 28, 2023, 10:55 a.m. UTC | #11

Queued, thanks.

Paolo

Paolo Bonzini April 28, 2023, 2:26 p.m. UTC | #12

Il mar 25 apr 2023, 15:31 Daniel P. Berrangé <[e-mail]> ha
scritto:

> > > -    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;
> > >
> >
> > This must be a memory leak since you're adding a g_new but not
> > adding any g_free
>
> Sorry, I'm failing to read properly today. It uses g_autofree
> so there is no leak.
>

On the other hand, if the pointer to the heap-allocated BHListSlice
escaped, this would be a dangling pointer as well—just not the kind that
the new GCC warning can report.

So this patch is also doing nothing but shut up the compiler; but it's
doing so in an underhanded manner and with a runtime cost, and as such it's
worse than Cedric's patch.

Paolo


> With regards,
> Daniel
> --
> |: https://berrange.com      -o-
> https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-
> https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-
> https://www.instagram.com/dberrange :|
>
>

Juan Quintela April 28, 2023, 4:24 p.m. UTC | #13

Paolo Bonzini <[e-mail]> wrote:
> Il mar 25 apr 2023, 15:31 Daniel P. Berrangé <[e-mail]> ha
> scritto:
>
>> > > -    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;
>> > >
>> >
>> > This must be a memory leak since you're adding a g_new but not
>> > adding any g_free
>>
>> Sorry, I'm failing to read properly today. It uses g_autofree
>> so there is no leak.
>>
>
> On the other hand, if the pointer to the heap-allocated BHListSlice
> escaped, this would be a dangling pointer as well—just not the kind that
> the new GCC warning can report.

I don't agree here.
If with my patch it becomes a dangling pointer because we free it.
With Cedric patch it is a local variable that gets exited out of the
function that created it.

Choose your poison.  One thing is bad and the other is worse.

> So this patch is also doing nothing but shut up the compiler; but it's
> doing so in an underhanded manner and with a runtime cost, and as such it's
> worse than Cedric's patch.

Ok.  I don't care (enogouh) about this to continue a discussion.. Can we
get Cedric patch upstream?

Thanks, Juan.

Paolo Bonzini April 28, 2023, 4:55 p.m. UTC | #14

Il ven 28 apr 2023, 18:25 Juan Quintela <[e-mail]> ha scritto:

> > On the other hand, if the pointer to the heap-allocated BHListSlice
> > escaped, this would be a dangling pointer as well—just not the kind that
> > the new GCC warning can report.
>
> I don't agree here.
> If with my patch it becomes a dangling pointer because we free it.
> With Cedric patch it is a local variable that gets exited out of the
> function that created it.


> Choose your poison.  One thing is bad and the other is worse.
>

Not sure which is worse—explicitly disabling a warning, at least, clearly
says the compiler finds it iffy.

> So this patch is also doing nothing but shut up the compiler; but it's
> > doing so in an underhanded manner and with a runtime cost, and as such
> it's
> > worse than Cedric's patch.
>
> Ok.  I don't care (enogouh) about this to continue a discussion.. Can we
> get Cedric patch upstream?
>

Yes I am sending the pull request very soon.

Paolo


> Thanks, Juan.
>
>

Thomas Huth May 17, 2023, 6:35 a.m. UTC | #15

On 20/04/2023 22.29, Cédric Le Goater wrote:
> From: Cédric Le Goater <[e-mail]>
> 
> 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.

I think this should also go into the next stable release (now on CC:), we're 
already getting bug reports about this:

  https://gitlab.com/qemu-project/qemu/-/issues/1655

  Thomas

Michael Tokarev May 17, 2023, 6:54 a.m. UTC | #16

17.05.2023 09:35, Thomas Huth wrote:
..

> I think this should also go into the next stable release (now on CC:), we're already getting bug reports about this:
> 
>   https://gitlab.com/qemu-project/qemu/-/issues/1655

Yes, I already picked that one up after noticing win32/win64 CI build failures.

Thank you for pointing this out!

/mjt

Patch

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;