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

[Paolo Bonzini <pbonzini@redhat.com>]

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.

Paolo

[1] http://sweng.the-davies.net/Home/rustys-api-design-manifesto