1. Patchew
  2. QEMU
  3. View series

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

Cédric Le Goater posted 1 patch 1 year ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20230420202939.1982044-1-clg@kaod.org
Maintainers: Stefan Hajnoczi <stefanha@redhat.com>, Fam Zheng <fam@euphon.net>
util/async.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
[PATCH] async: Suppress GCC13 false positive in aio_bh_poll()
Posted by Cédric Le Goater 1 year ago 
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;
-- 
2.40.0
Re: [PATCH] async: Suppress GCC13 false positive in aio_bh_poll()
Posted by Thomas Huth 11 months, 2 weeks ago 
On 20/04/2023 22.29, Cédric Le Goater 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.

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
Re: [PATCH] async: Suppress GCC13 false positive in aio_bh_poll()
Posted by Michael Tokarev 11 months, 2 weeks ago 
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
Re: [PATCH] async: Suppress GCC13 false positive in aio_bh_poll()
Posted by Paolo Bonzini 1 year ago 
Queued, thanks.

Paolo
Re: [PATCH] async: Suppress GCC13 false positive in aio_bh_poll()
Posted by Juan Quintela 1 year ago 
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
Re: [PATCH] async: Suppress GCC13 false positive in aio_bh_poll()
Posted by Daniel P. Berrangé 1 year ago 
On Tue, Apr 25, 2023 at 03:22:15PM +0200, Juan Quintela wrote:
> 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;

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

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 :|
Re: [PATCH] async: Suppress GCC13 false positive in aio_bh_poll()
Posted by Daniel P. Berrangé 1 year ago 
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 <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;
> 
> 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
-- 
|: 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 :|
Re: [PATCH] async: Suppress GCC13 false positive in aio_bh_poll()
Posted by Paolo Bonzini 1 year ago 
Il mar 25 apr 2023, 15:31 Daniel P. Berrangé <berrange@redhat.com> 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 :|
>
>
Re: [PATCH] async: Suppress GCC13 false positive in aio_bh_poll()
Posted by Juan Quintela 1 year ago 
Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il mar 25 apr 2023, 15:31 Daniel P. Berrangé <berrange@redhat.com> 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.
Re: [PATCH] async: Suppress GCC13 false positive in aio_bh_poll()
Posted by Paolo Bonzini 1 year ago 
Il ven 28 apr 2023, 18:25 Juan Quintela <quintela@redhat.com> 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.
>
>
Re: [PATCH] async: Suppress GCC13 false positive in aio_bh_poll()
Posted by Thomas Huth 1 year ago 
On 20/04/2023 22.29, Cédric Le Goater 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"

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 <thuth@redhat.com>
Re: [PATCH] async: Suppress GCC13 false positive in aio_bh_poll()
Posted by Cédric Le Goater 1 year ago 
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 <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"
> 
> Why do we need to ignore -Wpragmas here?

To handle GCC 8.5 as suggested by Philippe :

   https://lore.kernel.org/qemu-devel/24d45c41-2f62-76a2-4294-fcfa346c9683@linaro.org/

> 
>> +#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 <thuth@redhat.com>
> 

Thanks,

C.
Re: [PATCH] async: Suppress GCC13 false positive in aio_bh_poll()
Posted by Stefan Hajnoczi 1 year ago 
On Thu, 20 Apr 2023 at 16:31, 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

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Re: [PATCH] async: Suppress GCC13 false positive in aio_bh_poll()
Posted by Philippe Mathieu-Daudé 1 year ago 
On 20/4/23 22:29, Cédric Le Goater 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(+)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Re: [PATCH] async: Suppress GCC13 false positive in aio_bh_poll()
Posted by Daniel Henrique Barboza 1 year ago 

On 4/20/23 17:29, Cédric Le Goater 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>
> ---


Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Tested-by: Daniel Henrique Barboza <danielhb413@gmail.com>


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;
Re: [PATCH] async: Suppress GCC13 false positive in aio_bh_poll()
Posted by Daniel Henrique Barboza 1 year ago 

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 <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>
>> ---
> 
> 
> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> Tested-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> 
> 
> 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;
Re: [PATCH] async: Suppress GCC13 false positive in aio_bh_poll()
Posted by Cédric Le Goater 1 year ago 
+ Φλ

On 4/20/23 22:29, Cédric Le Goater 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;

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.