[PATCH for-8.0 0/3] Fixes for GCC13

[PATCH for-8.0 0/3] Fixes for GCC13

[Cédric Le Goater]

Hello,

I activated a GH workflow using fedora rawhide and found out that
there were a couple of compile breakage with the new GCC. Here are
fixes, the first requiring more attention.

Thanks,

C. 

Cédric Le Goater (3):
  async: Suppress GCC13 false positive in aio_bh_poll()
  target/s390x: Fix float_comp_to_cc() prototype
  target/ppc: Fix helper_pminsn() prototype

 target/s390x/s390x-internal.h |  3 ++-
 target/ppc/excp_helper.c      |  2 +-
 util/async.c                  | 13 ++++++++++++-
 3 files changed, 15 insertions(+), 3 deletions(-)

-- 
2.39.2

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

[Cédric Le Goater]

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.

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 | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/util/async.c b/util/async.c
index 21016a1ac7..45be1ed218 100644
--- a/util/async.c
+++ b/util/async.c
@@ -155,6 +155,11 @@ void aio_bh_call(QEMUBH *bh)
     bh->cb(bh->opaque);
 }
 
+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;
-- 
2.39.2

[PATCH for-8.0 2/3] target/s390x: Fix float_comp_to_cc() prototype

[Cédric Le Goater]

From: Cédric Le Goater <clg@redhat.com>

GCC13 reports an error :

../target/s390x/tcg/fpu_helper.c:123:5: error: conflicting types for ‘float_comp_to_cc’ due to enum/integer mismatch; have ‘int(CPUS390XState *, FloatRelation)’ {aka ‘int(struct CPUArchState *, FloatRelation)’} [-Werror=enum-int-mismatch]

  123 | int float_comp_to_cc(CPUS390XState *env, FloatRelation float_compare)
      |     ^~~~~~~~~~~~~~~~
In file included from ../target/s390x/tcg/fpu_helper.c:23:
../target/s390x/s390x-internal.h:302:5: note: previous declaration of ‘float_comp_to_cc’ with type ‘int(CPUS390XState *, int)’ {aka ‘int(struct CPUArchState *, int)’}
  302 | int float_comp_to_cc(CPUS390XState *env, int float_compare);
      |     ^~~~~~~~~~~~~~~~

Cc: Thomas Huth <thuth@redhat.com>
Cc: Richard Henderson <richard.henderson@linaro.org>
Cc: David Hildenbrand <david@redhat.com>
Cc: Ilya Leoshkevich <iii@linux.ibm.com>
Fixes: 71bfd65c5f ("softfloat: Name compare relation enum")
Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
 target/s390x/s390x-internal.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/target/s390x/s390x-internal.h b/target/s390x/s390x-internal.h
index 5d4361d35b..825252d728 100644
--- a/target/s390x/s390x-internal.h
+++ b/target/s390x/s390x-internal.h
@@ -11,6 +11,7 @@
 #define S390X_INTERNAL_H
 
 #include "cpu.h"
+#include "fpu/softfloat.h"
 
 #ifndef CONFIG_USER_ONLY
 typedef struct LowCore {
@@ -299,7 +300,7 @@ uint32_t set_cc_nz_f128(float128 v);
 uint8_t s390_softfloat_exc_to_ieee(unsigned int exc);
 int s390_swap_bfp_rounding_mode(CPUS390XState *env, int m3);
 void s390_restore_bfp_rounding_mode(CPUS390XState *env, int old_mode);
-int float_comp_to_cc(CPUS390XState *env, int float_compare);
+int float_comp_to_cc(CPUS390XState *env, FloatRelation float_compare);
 
 #define DCMASK_ZERO             0x0c00
 #define DCMASK_NORMAL           0x0300
-- 
2.39.2

[PATCH for-8.0 3/3] target/ppc: Fix helper_pminsn() prototype

[Cédric Le Goater]

From: Cédric Le Goater <clg@redhat.com>

GCC13 reports an error:

../target/ppc/excp_helper.c:2625:6: error: conflicting types for ‘helper_pminsn’ due to enum/integer mismatch; have ‘void(CPUPPCState *, powerpc_pm_insn_t)’ {aka ‘void(struct CPUArchState *, powerpc_pm_insn_t)’} [-Werror=enum-int-mismatch]
 2625 | void helper_pminsn(CPUPPCState *env, powerpc_pm_insn_t insn)
      |      ^~~~~~~~~~~~~
In file included from /home/legoater/work/qemu/qemu.git/include/qemu/osdep.h:49,
                 from ../target/ppc/excp_helper.c:19:
/home/legoater/work/qemu/qemu.git/include/exec/helper-head.h:23:27: note: previous declaration of ‘helper_pminsn’ with type ‘void(CPUArchState *, uint32_t)’ {aka ‘void(CPUArchState *, unsigned int)’}
   23 | #define HELPER(name) glue(helper_, name)
      |                           ^~~~~~~

Cc: Daniel Henrique Barboza <danielhb413@gmail.com>
Fixes: 7778a575c7 ("ppc: Add P7/P8 Power Management instructions")
Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
 target/ppc/excp_helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index 287659c74d..199328f4b6 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -2622,7 +2622,7 @@ void helper_scv(CPUPPCState *env, uint32_t lev)
     }
 }
 
-void helper_pminsn(CPUPPCState *env, powerpc_pm_insn_t insn)
+void helper_pminsn(CPUPPCState *env, uint32_t insn)
 {
     CPUState *cs;
 
-- 
2.39.2

Re: [PATCH for-8.0 2/3] target/s390x: Fix float_comp_to_cc() prototype

[Philippe Mathieu-Daudé]

On 21/3/23 09:33, Cédric Le Goater wrote:
> From: Cédric Le Goater <clg@redhat.com>
> 
> GCC13 reports an error :
> 
> ../target/s390x/tcg/fpu_helper.c:123:5: error: conflicting types for ‘float_comp_to_cc’ due to enum/integer mismatch; have ‘int(CPUS390XState *, FloatRelation)’ {aka ‘int(struct CPUArchState *, FloatRelation)’} [-Werror=enum-int-mismatch]
> 
>    123 | int float_comp_to_cc(CPUS390XState *env, FloatRelation float_compare)
>        |     ^~~~~~~~~~~~~~~~
> In file included from ../target/s390x/tcg/fpu_helper.c:23:
> ../target/s390x/s390x-internal.h:302:5: note: previous declaration of ‘float_comp_to_cc’ with type ‘int(CPUS390XState *, int)’ {aka ‘int(struct CPUArchState *, int)’}
>    302 | int float_comp_to_cc(CPUS390XState *env, int float_compare);
>        |     ^~~~~~~~~~~~~~~~
> 
> Cc: Thomas Huth <thuth@redhat.com>
> Cc: Richard Henderson <richard.henderson@linaro.org>
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Ilya Leoshkevich <iii@linux.ibm.com>
> Fixes: 71bfd65c5f ("softfloat: Name compare relation enum")
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
>   target/s390x/s390x-internal.h | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

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

[Paolo Bonzini]

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.

I'd rather remove the warning.

Paolo

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

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

[Paolo Bonzini]

[-- Attachment #1: Type: text/plain, Size: 1974 bytes --]

Il mar 21 mar 2023, 11:30 Daniel P. Berrangé <berrange@redhat.com> ha
scritto:

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

Okay, then I would like to see a single SIGSEGV in QEMU that was caused by
a local variable making its way to a global pointer.

As to this specific case, we could add a bool removed flag to BHListSlice
and assert it before aio_bh_poll() returns, but I think even that is
overkill.

Paolo

[-- Attachment #2: Type: text/html, Size: 2731 bytes --]

Re: [PATCH for-8.0 3/3] target/ppc: Fix helper_pminsn() prototype

[Daniel Henrique Barboza]

On 3/21/23 05:33, Cédric Le Goater wrote:
> From: Cédric Le Goater <clg@redhat.com>
> 
> GCC13 reports an error:
> 
> ../target/ppc/excp_helper.c:2625:6: error: conflicting types for ‘helper_pminsn’ due to enum/integer mismatch; have ‘void(CPUPPCState *, powerpc_pm_insn_t)’ {aka ‘void(struct CPUArchState *, powerpc_pm_insn_t)’} [-Werror=enum-int-mismatch]
>   2625 | void helper_pminsn(CPUPPCState *env, powerpc_pm_insn_t insn)
>        |      ^~~~~~~~~~~~~
> In file included from /home/legoater/work/qemu/qemu.git/include/qemu/osdep.h:49,
>                   from ../target/ppc/excp_helper.c:19:
> /home/legoater/work/qemu/qemu.git/include/exec/helper-head.h:23:27: note: previous declaration of ‘helper_pminsn’ with type ‘void(CPUArchState *, uint32_t)’ {aka ‘void(CPUArchState *, unsigned int)’}
>     23 | #define HELPER(name) glue(helper_, name)
>        |                           ^~~~~~~
> 
> Cc: Daniel Henrique Barboza <danielhb413@gmail.com>
> Fixes: 7778a575c7 ("ppc: Add P7/P8 Power Management instructions")
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---

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

>   target/ppc/excp_helper.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index 287659c74d..199328f4b6 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -2622,7 +2622,7 @@ void helper_scv(CPUPPCState *env, uint32_t lev)
>       }
>   }
>   
> -void helper_pminsn(CPUPPCState *env, powerpc_pm_insn_t insn)
> +void helper_pminsn(CPUPPCState *env, uint32_t insn)
>   {
>       CPUState *cs;
>   

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1408 bytes --]

On Tue, Mar 21, 2023 at 09:33:20AM +0100, 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.
> 
> 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 | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)

Thanks!

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Paolo Bonzini]

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

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.

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

[Paolo Bonzini]

On 3/21/23 13:16, Cédric Le Goater wrote:
> 
> +    /*
> +     * 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;

Yeah, that's clearer.  Maybe even add "but the list is emptied before 
this function returns".

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

I think clang understand the GCC pragma as well.

Paolo

Re: [PATCH for-8.0 2/3] target/s390x: Fix float_comp_to_cc() prototype

[Richard Henderson]

On 3/21/23 01:33, Cédric Le Goater wrote:
> From: Cédric Le Goater<clg@redhat.com>
> 
> GCC13 reports an error :
> 
> ../target/s390x/tcg/fpu_helper.c:123:5: error: conflicting types for ‘float_comp_to_cc’ due to enum/integer mismatch; have ‘int(CPUS390XState *, FloatRelation)’ {aka ‘int(struct CPUArchState *, FloatRelation)’} [-Werror=enum-int-mismatch]
> 
>    123 | int float_comp_to_cc(CPUS390XState *env, FloatRelation float_compare)
>        |     ^~~~~~~~~~~~~~~~
> In file included from ../target/s390x/tcg/fpu_helper.c:23:
> ../target/s390x/s390x-internal.h:302:5: note: previous declaration of ‘float_comp_to_cc’ with type ‘int(CPUS390XState *, int)’ {aka ‘int(struct CPUArchState *, int)’}
>    302 | int float_comp_to_cc(CPUS390XState *env, int float_compare);
>        |     ^~~~~~~~~~~~~~~~
> 
> Cc: Thomas Huth<thuth@redhat.com>
> Cc: Richard Henderson<richard.henderson@linaro.org>
> Cc: David Hildenbrand<david@redhat.com>
> Cc: Ilya Leoshkevich<iii@linux.ibm.com>
> Fixes: 71bfd65c5f ("softfloat: Name compare relation enum")
> Signed-off-by: Cédric Le Goater<clg@redhat.com>
> ---
>   target/s390x/s390x-internal.h | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH for-8.0 3/3] target/ppc: Fix helper_pminsn() prototype

[Richard Henderson]

On 3/21/23 01:33, Cédric Le Goater wrote:
> From: Cédric Le Goater<clg@redhat.com>
> 
> GCC13 reports an error:
> 
> ../target/ppc/excp_helper.c:2625:6: error: conflicting types for ‘helper_pminsn’ due to enum/integer mismatch; have ‘void(CPUPPCState *, powerpc_pm_insn_t)’ {aka ‘void(struct CPUArchState *, powerpc_pm_insn_t)’} [-Werror=enum-int-mismatch]
>   2625 | void helper_pminsn(CPUPPCState *env, powerpc_pm_insn_t insn)
>        |      ^~~~~~~~~~~~~
> In file included from /home/legoater/work/qemu/qemu.git/include/qemu/osdep.h:49,
>                   from ../target/ppc/excp_helper.c:19:
> /home/legoater/work/qemu/qemu.git/include/exec/helper-head.h:23:27: note: previous declaration of ‘helper_pminsn’ with type ‘void(CPUArchState *, uint32_t)’ {aka ‘void(CPUArchState *, unsigned int)’}
>     23 | #define HELPER(name) glue(helper_, name)
>        |                           ^~~~~~~
> 
> Cc: Daniel Henrique Barboza<danielhb413@gmail.com>
> Fixes: 7778a575c7 ("ppc: Add P7/P8 Power Management instructions")
> Signed-off-by: Cédric Le Goater<clg@redhat.com>
> ---
>   target/ppc/excp_helper.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH for-8.0 2/3] target/s390x: Fix float_comp_to_cc() prototype

[Thomas Huth]

On 21/03/2023 09.33, Cédric Le Goater wrote:
> From: Cédric Le Goater <clg@redhat.com>
> 
> GCC13 reports an error :
> 
> ../target/s390x/tcg/fpu_helper.c:123:5: error: conflicting types for ‘float_comp_to_cc’ due to enum/integer mismatch; have ‘int(CPUS390XState *, FloatRelation)’ {aka ‘int(struct CPUArchState *, FloatRelation)’} [-Werror=enum-int-mismatch]
> 
>    123 | int float_comp_to_cc(CPUS390XState *env, FloatRelation float_compare)
>        |     ^~~~~~~~~~~~~~~~
> In file included from ../target/s390x/tcg/fpu_helper.c:23:
> ../target/s390x/s390x-internal.h:302:5: note: previous declaration of ‘float_comp_to_cc’ with type ‘int(CPUS390XState *, int)’ {aka ‘int(struct CPUArchState *, int)’}
>    302 | int float_comp_to_cc(CPUS390XState *env, int float_compare);
>        |     ^~~~~~~~~~~~~~~~
> 
> Cc: Thomas Huth <thuth@redhat.com>
> Cc: Richard Henderson <richard.henderson@linaro.org>
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Ilya Leoshkevich <iii@linux.ibm.com>
> Fixes: 71bfd65c5f ("softfloat: Name compare relation enum")
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
>   target/s390x/s390x-internal.h | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/target/s390x/s390x-internal.h b/target/s390x/s390x-internal.h
> index 5d4361d35b..825252d728 100644
> --- a/target/s390x/s390x-internal.h
> +++ b/target/s390x/s390x-internal.h
> @@ -11,6 +11,7 @@
>   #define S390X_INTERNAL_H
>   
>   #include "cpu.h"
> +#include "fpu/softfloat.h"
>   
>   #ifndef CONFIG_USER_ONLY
>   typedef struct LowCore {
> @@ -299,7 +300,7 @@ uint32_t set_cc_nz_f128(float128 v);
>   uint8_t s390_softfloat_exc_to_ieee(unsigned int exc);
>   int s390_swap_bfp_rounding_mode(CPUS390XState *env, int m3);
>   void s390_restore_bfp_rounding_mode(CPUS390XState *env, int old_mode);
> -int float_comp_to_cc(CPUS390XState *env, int float_compare);
> +int float_comp_to_cc(CPUS390XState *env, FloatRelation float_compare);
>   
>   #define DCMASK_ZERO             0x0c00
>   #define DCMASK_NORMAL           0x0300

Reviewed-by: Thomas Huth <thuth@redhat.com>