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

["Cédric Le Goater" <clg@redhat.com>]

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.