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

[Cédric Le Goater]

On 3/21/23 12:57, Paolo Bonzini wrote:
> On 3/21/23 09:33, Cédric Le Goater wrote:
> > +static void aio_bh_slice_insert(AioContext *ctx, BHListSlice *slice)
> > +{
> > +    QSIMPLEQ_INSERT_TAIL(&ctx->bh_slice_list, slice, next);
> > +}
> > +
> >   /* Multiple occurrences of aio_bh_poll cannot be called concurrently. */
> >   int aio_bh_poll(AioContext *ctx)
> >   {
> > @@ -164,7 +169,13 @@ 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);
> > -    QSIMPLEQ_INSERT_TAIL(&ctx->bh_slice_list, &slice, next);
> > +
> > +    /*
> > +     * GCC13 [-Werror=dangling-pointer=] complains that the local variable
> > +     * 'slice' is being stored in a global list in 'ctx->bh_slice_list'.
> > +     * Use a helper to silent the compiler
> > +     */
> > +    aio_bh_slice_insert(ctx, &slice);
> >       while ((s = QSIMPLEQ_FIRST(&ctx->bh_slice_list))) {
> >           QEMUBH *bh;
> > --
>
> Sorry, but an API that has "insert" and not "remove", and where the argument is 
> *expected to be* a local variable (which must be removed to avoid a dangling pointer---and the 
> warning is exactly -Wdangling-pointer), ranks at least -7 in the bad API ranking[1].

:)

> I tried wrapping the BHListSlice and BHListSlice* into an iterator struct 
> (which is also really overkill, but at least---in theory---it's idiomatic), but 
> the code was hard to follow.
>
> The fact that the workaround is so ugly, in my opinion, points even more 
> strongly at the compiler being in the wrong here.

It was initially called slice_dangling_pointer_fixup() how's that ?

An alternative could be :

@@ -164,7 +164,14 @@ 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 list 'ctx->bh_slice_list'.
+     */
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wdangling-pointer="
     QSIMPLEQ_INSERT_TAIL(&ctx->bh_slice_list, &slice, next);
+#pragma GCC diagnostic pop
 
     while ((s = QSIMPLEQ_FIRST(&ctx->bh_slice_list))) {
         QEMUBH *bh;

May be that's more explicit. I wonder if we need to ifdef clang also.

Thanks,

C.