| Message ID | 20230321083322.663561-2-clg@kaod.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fixes for GCC13 | expand |
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
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
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
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>
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
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.
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
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;