| Message ID | 4b040fc18cb0e563e92ce084ca18b89a050a8aaa.1699606819.git.manos.pitsidianakis@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add QEMU_WARN_UNUSED_RESULT function attribute | expand |
On Fri, 10 Nov 2023 at 09:16, Manos Pitsidianakis <manos.pitsidianakis@linaro.org> wrote: > > Ignoring the return value by accident is easy to miss as a bug. Such a > bug was spotted by Coverity CID 1523899. Now, future instances of this > type of bug will produce a warning when using GCC. > > Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org> > --- > audio/audio.h | 2 +- > hw/arm/omap2.c | 8 +++++++- > hw/input/tsc210x.c | 8 +++++++- > 3 files changed, 15 insertions(+), 3 deletions(-) > > diff --git a/audio/audio.h b/audio/audio.h > index fcc22307be..b78c75962e 100644 > --- a/audio/audio.h > +++ b/audio/audio.h > @@ -94,7 +94,7 @@ typedef struct QEMUAudioTimeStamp { > void AUD_vlog (const char *cap, const char *fmt, va_list ap) G_GNUC_PRINTF(2, 0); > void AUD_log (const char *cap, const char *fmt, ...) G_GNUC_PRINTF(2, 3); > > -bool AUD_register_card (const char *name, QEMUSoundCard *card, Error **errp); > +bool AUD_register_card (const char *name, QEMUSoundCard *card, Error **errp) QEMU_WARN_UNUSED_RESULT; > void AUD_remove_card (QEMUSoundCard *card); > CaptureVoiceOut *AUD_add_capture( > AudioState *s, > diff --git a/hw/arm/omap2.c b/hw/arm/omap2.c > index f170728e7e..59fc061120 100644 > --- a/hw/arm/omap2.c > +++ b/hw/arm/omap2.c > @@ -614,7 +614,13 @@ static struct omap_eac_s *omap_eac_init(struct omap_target_agent_s *ta, > s->codec.card.name = g_strdup(current_machine->audiodev); > s->codec.card.state = audio_state_by_name(s->codec.card.name, &error_fatal); > } > - AUD_register_card("OMAP EAC", &s->codec.card, &error_fatal); > + /* > + * We pass error_fatal so on error QEMU will exit(). But we check the > + * return value to make the warn_unused_result compiler warning go away. > + */ > + if (!AUD_register_card("OMAP EAC", &s->codec.card, &error_fatal)) { > + exit(1); > + } This kind of thing is why Coverity's unused-result warning has a lot of false positives. We shouldn't introduce extra code like this to work around the fact that the tooling doesn't understand our error-handling convention (i.e. error_fatal, and the way that some functions report errors both via the return value and also via the Error** argument). thanks -- PMM
On Fri, 10 Nov 2023 12:21, Peter Maydell <peter.maydell@linaro.org> wrote: >This kind of thing is why Coverity's unused-result warning has a >lot of false positives. We shouldn't introduce extra code like >this to work around the fact that the tooling doesn't understand >our error-handling convention (i.e. error_fatal, and the way >that some functions report errors both via the return value and >also via the Error** argument). I respect that :). But I personally believe that clinging to C's inadequacies, instead of preventing bugs statically just because it adds some lines of code, is misguided. Proper code should strive to make bugs impossible in the first place. At least that is my perspective and I would like there to be constructive discussions about different approaches in the mailing list. Perhaps something good might come out of it!
On Fri, 10 Nov 2023 at 11:02, Manos Pitsidianakis <manos.pitsidianakis@linaro.org> wrote: > > On Fri, 10 Nov 2023 12:21, Peter Maydell <peter.maydell@linaro.org> wrote: > >This kind of thing is why Coverity's unused-result warning has a > >lot of false positives. We shouldn't introduce extra code like > >this to work around the fact that the tooling doesn't understand > >our error-handling convention (i.e. error_fatal, and the way > >that some functions report errors both via the return value and > >also via the Error** argument). > > I respect that :). But I personally believe that clinging to C's > inadequacies, instead of preventing bugs statically just because it adds > some lines of code, is misguided. Proper code should strive to make bugs > impossible in the first place. I generally agree. The problem here really is that we've ended up with this odd API convention that reports errors in two ways. In an ideal world we'd tidy up our APIs to report errors exactly in one way (presumably via the Error). -- PMM
On Fri, Nov 10, 2023 at 12:44:56PM +0200, Manos Pitsidianakis wrote: > On Fri, 10 Nov 2023 12:21, Peter Maydell <peter.maydell@linaro.org> wrote: > > This kind of thing is why Coverity's unused-result warning has a > > lot of false positives. We shouldn't introduce extra code like > > this to work around the fact that the tooling doesn't understand > > our error-handling convention (i.e. error_fatal, and the way > > that some functions report errors both via the return value and > > also via the Error** argument). > > I respect that :). But I personally believe that clinging to C's > inadequacies, instead of preventing bugs statically just because it adds > some lines of code, is misguided. Proper code should strive to make bugs > impossible in the first place. At least that is my perspective and I would > like there to be constructive discussions about different approaches in the > mailing list. Perhaps something good might come out of it! Your approach to the problem: if (!AUD_register_card("OMAP EAC", &s->codec.card, &error_fatal)) { exit(1); } is adding dead-code because the exit(1) will never be reachable. So while it lets you squelch the unused result warning, it is verbose and misleading to anyone who sees it. Perhaps a more viable option is to pull in gnulib's ignore_value macro #define ignore_value(x) \ (__extension__ ({ __typeof__ (x) __x = (x); (void) __x; })) and then we would have just this: ignore_value(AUD_register_card("OMAP EAC", &s->codec.card, &error_fatal)); With regards, Daniel
On Fri, Nov 10, 2023 at 11:18:39AM +0000, Peter Maydell wrote: > On Fri, 10 Nov 2023 at 11:02, Manos Pitsidianakis > <manos.pitsidianakis@linaro.org> wrote: > > > > On Fri, 10 Nov 2023 12:21, Peter Maydell <peter.maydell@linaro.org> wrote: > > >This kind of thing is why Coverity's unused-result warning has a > > >lot of false positives. We shouldn't introduce extra code like > > >this to work around the fact that the tooling doesn't understand > > >our error-handling convention (i.e. error_fatal, and the way > > >that some functions report errors both via the return value and > > >also via the Error** argument). > > > > I respect that :). But I personally believe that clinging to C's > > inadequacies, instead of preventing bugs statically just because it adds > > some lines of code, is misguided. Proper code should strive to make bugs > > impossible in the first place. > > I generally agree. The problem here really is that we've ended > up with this odd API convention that reports errors in two > ways. In an ideal world we'd tidy up our APIs to report errors > exactly in one way (presumably via the Error). The compelling thing about our slightly odd &error_fatal/error_abort approach is that we can directly get stack traces showing exactly where the error happened. If we just propagate the error up the stack to finally exit/abort, the origin context is essentially lost, and when it does abort, we don't get a snapshot of all threads at the time the error hit, as we've moved on in time. With regards, Daniel
On Fri, 10 Nov 2023 13:20, "Daniel P. Berrangé" <berrange@redhat.com> wrote: >Your approach to the problem: > > if (!AUD_register_card("OMAP EAC", &s->codec.card, &error_fatal)) { > exit(1); > } > >is adding dead-code because the exit(1) will never be reachable. So while >it lets you squelch the unused result warning, it is verbose and misleading >to anyone who sees it. > >Perhaps a more viable option is to pull in gnulib's ignore_value macro > > #define ignore_value(x) \ > (__extension__ ({ __typeof__ (x) __x = (x); (void) __x; })) > >and then we would have just this: > > ignore_value(AUD_register_card("OMAP EAC", &s->codec.card, &error_fatal)); Good suggestion, thanks! And to be fair, I did put a comment directly above that line to explain the unreachable code path. :) + /* + * We pass error_fatal so on error QEMU will exit(). But we check the + * return value to make the warn_unused_result compiler warning go away. + */
On Fri, 10 Nov 2023 13:23, "Daniel P. Berrangé" <berrange@redhat.com> wrote: >On Fri, Nov 10, 2023 at 11:18:39AM +0000, Peter Maydell wrote: >> On Fri, 10 Nov 2023 at 11:02, Manos Pitsidianakis >> <manos.pitsidianakis@linaro.org> wrote: >> > >> > On Fri, 10 Nov 2023 12:21, Peter Maydell <peter.maydell@linaro.org> wrote: >> > >This kind of thing is why Coverity's unused-result warning has a >> > >lot of false positives. We shouldn't introduce extra code like >> > >this to work around the fact that the tooling doesn't understand >> > >our error-handling convention (i.e. error_fatal, and the way >> > >that some functions report errors both via the return value and >> > >also via the Error** argument). >> > >> > I respect that :). But I personally believe that clinging to C's >> > inadequacies, instead of preventing bugs statically just because it adds >> > some lines of code, is misguided. Proper code should strive to make bugs >> > impossible in the first place. >> >> I generally agree. The problem here really is that we've ended >> up with this odd API convention that reports errors in two >> ways. In an ideal world we'd tidy up our APIs to report errors >> exactly in one way (presumably via the Error). > >The compelling thing about our slightly odd &error_fatal/error_abort >approach is that we can directly get stack traces showing exactly >where the error happened. > >If we just propagate the error up the stack to finally exit/abort, >the origin context is essentially lost, and when it does abort, we >don't get a snapshot of all threads at the time the error hit, as >we've moved on in time. FYI: It is possible to get a stack trace using gnu libunwind which is portable, dependency-free and runs on many targets: https://github.com/libunwind/libunwind#libunwind Or llvm's libunwind which is also of great quality: https://bcain-llvm.readthedocs.io/projects/libunwind/en/latest/ They are also compatible with co-routines and JIT code (At least, the GNU one is, not sure about llvm).
On Fri, 10 Nov 2023, Daniel P. Berrangé wrote: > On Fri, Nov 10, 2023 at 12:44:56PM +0200, Manos Pitsidianakis wrote: >> On Fri, 10 Nov 2023 12:21, Peter Maydell <peter.maydell@linaro.org> wrote: >>> This kind of thing is why Coverity's unused-result warning has a >>> lot of false positives. We shouldn't introduce extra code like >>> this to work around the fact that the tooling doesn't understand >>> our error-handling convention (i.e. error_fatal, and the way >>> that some functions report errors both via the return value and >>> also via the Error** argument). >> >> I respect that :). But I personally believe that clinging to C's >> inadequacies, instead of preventing bugs statically just because it adds >> some lines of code, is misguided. Proper code should strive to make bugs >> impossible in the first place. At least that is my perspective and I would >> like there to be constructive discussions about different approaches in the >> mailing list. Perhaps something good might come out of it! > > Your approach to the problem: > > if (!AUD_register_card("OMAP EAC", &s->codec.card, &error_fatal)) { > exit(1); > } > > is adding dead-code because the exit(1) will never be reachable. So while > it lets you squelch the unused result warning, it is verbose and misleading > to anyone who sees it. > > Perhaps a more viable option is to pull in gnulib's ignore_value macro > > #define ignore_value(x) \ > (__extension__ ({ __typeof__ (x) __x = (x); (void) __x; })) > > and then we would have just this: > > ignore_value(AUD_register_card("OMAP EAC", &s->codec.card, &error_fatal)); I wonder if just casting to (void) without assigning to a value could avoid the warning? In that case you would not need a macro just (void)AUD_register_card("OMAP EAC", &s->codec.card, &error_fatal); Not sure it's clearer than a macro which states what it does but the definition is a bit cryptic while this cast is simpler but may also puzzle somebody not familiar with it. Regards, BALATON Zoltan
On Fri, Nov 10, 2023 at 12:35:56PM +0100, BALATON Zoltan wrote: > On Fri, 10 Nov 2023, Daniel P. Berrangé wrote: > > On Fri, Nov 10, 2023 at 12:44:56PM +0200, Manos Pitsidianakis wrote: > > > On Fri, 10 Nov 2023 12:21, Peter Maydell <peter.maydell@linaro.org> wrote: > > > > This kind of thing is why Coverity's unused-result warning has a > > > > lot of false positives. We shouldn't introduce extra code like > > > > this to work around the fact that the tooling doesn't understand > > > > our error-handling convention (i.e. error_fatal, and the way > > > > that some functions report errors both via the return value and > > > > also via the Error** argument). > > > > > > I respect that :). But I personally believe that clinging to C's > > > inadequacies, instead of preventing bugs statically just because it adds > > > some lines of code, is misguided. Proper code should strive to make bugs > > > impossible in the first place. At least that is my perspective and I would > > > like there to be constructive discussions about different approaches in the > > > mailing list. Perhaps something good might come out of it! > > > > Your approach to the problem: > > > > if (!AUD_register_card("OMAP EAC", &s->codec.card, &error_fatal)) { > > exit(1); > > } > > > > is adding dead-code because the exit(1) will never be reachable. So while > > it lets you squelch the unused result warning, it is verbose and misleading > > to anyone who sees it. > > > > Perhaps a more viable option is to pull in gnulib's ignore_value macro > > > > #define ignore_value(x) \ > > (__extension__ ({ __typeof__ (x) __x = (x); (void) __x; })) > > > > and then we would have just this: > > > > ignore_value(AUD_register_card("OMAP EAC", &s->codec.card, &error_fatal)); > > I wonder if just casting to (void) without assigning to a value could avoid > the warning? In that case you would not need a macro just > > (void)AUD_register_card("OMAP EAC", &s->codec.card, &error_fatal); Casting to void is not sufficient, which is why I suggested the ignore_value macro, which does enough to fool the compiler into thinking the return value is used. With regards, Daniel
On 10/11/23 12:20, Daniel P. Berrangé wrote: > On Fri, Nov 10, 2023 at 12:44:56PM +0200, Manos Pitsidianakis wrote: >> On Fri, 10 Nov 2023 12:21, Peter Maydell <peter.maydell@linaro.org> wrote: >>> This kind of thing is why Coverity's unused-result warning has a >>> lot of false positives. We shouldn't introduce extra code like >>> this to work around the fact that the tooling doesn't understand >>> our error-handling convention (i.e. error_fatal, and the way >>> that some functions report errors both via the return value and >>> also via the Error** argument). >> >> I respect that :). But I personally believe that clinging to C's >> inadequacies, instead of preventing bugs statically just because it adds >> some lines of code, is misguided. Proper code should strive to make bugs >> impossible in the first place. At least that is my perspective and I would >> like there to be constructive discussions about different approaches in the >> mailing list. Perhaps something good might come out of it! > > Your approach to the problem: > > if (!AUD_register_card("OMAP EAC", &s->codec.card, &error_fatal)) { > exit(1); Rather: g_assert_not_reached(); > } > > is adding dead-code because the exit(1) will never be reachable. So while > it lets you squelch the unused result warning, it is verbose and misleading > to anyone who sees it.
diff --git a/audio/audio.h b/audio/audio.h index fcc22307be..b78c75962e 100644 --- a/audio/audio.h +++ b/audio/audio.h @@ -94,7 +94,7 @@ typedef struct QEMUAudioTimeStamp { void AUD_vlog (const char *cap, const char *fmt, va_list ap) G_GNUC_PRINTF(2, 0); void AUD_log (const char *cap, const char *fmt, ...) G_GNUC_PRINTF(2, 3); -bool AUD_register_card (const char *name, QEMUSoundCard *card, Error **errp); +bool AUD_register_card (const char *name, QEMUSoundCard *card, Error **errp) QEMU_WARN_UNUSED_RESULT; void AUD_remove_card (QEMUSoundCard *card); CaptureVoiceOut *AUD_add_capture( AudioState *s, diff --git a/hw/arm/omap2.c b/hw/arm/omap2.c index f170728e7e..59fc061120 100644 --- a/hw/arm/omap2.c +++ b/hw/arm/omap2.c @@ -614,7 +614,13 @@ static struct omap_eac_s *omap_eac_init(struct omap_target_agent_s *ta, s->codec.card.name = g_strdup(current_machine->audiodev); s->codec.card.state = audio_state_by_name(s->codec.card.name, &error_fatal); } - AUD_register_card("OMAP EAC", &s->codec.card, &error_fatal); + /* + * We pass error_fatal so on error QEMU will exit(). But we check the + * return value to make the warn_unused_result compiler warning go away. + */ + if (!AUD_register_card("OMAP EAC", &s->codec.card, &error_fatal)) { + exit(1); + } memory_region_init_io(&s->iomem, NULL, &omap_eac_ops, s, "omap.eac", omap_l4_region_size(ta, 0)); diff --git a/hw/input/tsc210x.c b/hw/input/tsc210x.c index 950506fb38..003c664b56 100644 --- a/hw/input/tsc210x.c +++ b/hw/input/tsc210x.c @@ -1102,7 +1102,13 @@ static void tsc210x_init(TSC210xState *s, s->card.name = g_strdup(current_machine->audiodev); s->card.state = audio_state_by_name(s->card.name, &error_fatal); } - AUD_register_card(s->name, &s->card, &error_fatal); + /* + * We pass error_fatal so on error QEMU will exit(). But we check the + * return value to make the warn_unused_result compiler warning go away. + */ + if (!AUD_register_card(s->name, &s->card, &error_fatal)) { + return; + } qemu_register_reset((void *) tsc210x_reset, s); vmstate_register(NULL, 0, vmsd, s);
Ignoring the return value by accident is easy to miss as a bug. Such a bug was spotted by Coverity CID 1523899. Now, future instances of this type of bug will produce a warning when using GCC. Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org> --- audio/audio.h | 2 +- hw/arm/omap2.c | 8 +++++++- hw/input/tsc210x.c | 8 +++++++- 3 files changed, 15 insertions(+), 3 deletions(-)