Re: [PATCH for-8.0 1/3] async: Suppress GCC13 false positive in aio_bh_poll()

[Daniel P . Berrangé]

On Tue, Mar 21, 2023 at 11:22:33AM +0100, Paolo Bonzini wrote:
> On 3/21/23 09:33, 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 an intermediate helper to
> > silent GCC. No functional change.
> 
> Before doing this, I would like to see a case where this bug was _not_
> caught by either Coverity (which is currently offline but I'm fixing it
> right now) or just cursory review.

IMHO coverity is not a substitute for this, because it is only available
post merge, while the GCC warning is available to all maintainers on
every build. As for code review, mistakes inevitably happen. 

Personally I find the code in this method pretty obtuse. It is hard to
reason about it to convince yourself that it is safe to be adding the
local variable to the global linked list and have it removed again
before returning.

Stefan has explained why it is correct, but I tend to think of the compiler
warning here as a sign that the code might be better to be written in a
different way that is more obviously correct. If this really is the best
way to write this method though, an alternative could be selectively
disabling the warning with a local pragma, along with adding a comment
to the method to explain why this unusual code pattern is indeed safe.

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