Message ID | 20230816091522.1292029-1-thuth@redhat.com |
---|---|
State | New |
Headers | show |
Series | subprojects/berkeley-testfloat-3: Update to fix a problem with compiler warnings | expand |
On 16/8/23 11:15, Thomas Huth wrote: > Update the berkeley-testfloat-3 wrap to include a patch provided by > Olaf Hering. This fixes a problem with "control reaches end of non-void > function [-Werror=return-type]" compiler warning/errors that are now > enabled by default in certain versions of GCC. e7af9751d9 has: --- a/source/genCases_f32.c +++ b/source/genCases_f32.c @@ -411,6 +411,7 @@ static float32_t f32Random( void ) case 6: return f32RandomQInfP3(); case 7: + default: return f32RandomQInfPInf(); } Why the default is with 7 and not 3 or 5? Shouldn't we abort() or mark with __builtin_unreachable()? > > Reported-by: Olaf Hering <olaf@aepfle.de> > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > subprojects/berkeley-testfloat-3.wrap | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-)
On Wed, 16 Aug 2023 at 10:16, Thomas Huth <thuth@redhat.com> wrote: > > Update the berkeley-testfloat-3 wrap to include a patch provided by > Olaf Hering. This fixes a problem with "control reaches end of non-void > function [-Werror=return-type]" compiler warning/errors that are now > enabled by default in certain versions of GCC. > > Reported-by: Olaf Hering <olaf@aepfle.de> > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > subprojects/berkeley-testfloat-3.wrap | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) This seems like a reasonable place to ask: should we just pull in the testfloat and softfloat repos to be part of the main qemu repo? AIUI we've definitively forked both of these, so we don't care about trying to make it easy to resync with upstream. Having them in separate git repos seems to have some clear disadvantages: * it's harder to update them * changes to them can end up skipping the usual code review process, because it's a different patch flow to the normal one * we get extra meson subproject infrastructure to deal with Are there any reasons to keep them separate ? thanks -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On Wed, 16 Aug 2023 at 10:16, Thomas Huth <thuth@redhat.com> wrote: >> >> Update the berkeley-testfloat-3 wrap to include a patch provided by >> Olaf Hering. This fixes a problem with "control reaches end of non-void >> function [-Werror=return-type]" compiler warning/errors that are now >> enabled by default in certain versions of GCC. >> >> Reported-by: Olaf Hering <olaf@aepfle.de> >> Signed-off-by: Thomas Huth <thuth@redhat.com> >> --- >> subprojects/berkeley-testfloat-3.wrap | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) > > This seems like a reasonable place to ask: should we just pull > in the testfloat and softfloat repos to be part of the main > qemu repo? We've definitely forked the softfloat inside QEMU with the refactor some time ago. For the testing repos we have lightly modified them to build the test code but only by a few patches. We might want to keep the ability to re-base on a new release if say test float gains fp16 or bfloat16 support. > AIUI we've definitively forked both of these, so > we don't care about trying to make it easy to resync with > upstream. Having them in separate git repos seems to have some > clear disadvantages: > * it's harder to update them > * changes to them can end up skipping the usual code > review process, because it's a different patch flow > to the normal one > * we get extra meson subproject infrastructure to deal with > > Are there any reasons to keep them separate ? > > thanks > -- PMM
Wed, 16 Aug 2023 14:49:07 +0200 Philippe Mathieu-Daudé <philmd@linaro.org>: > Why the default is with 7 and not 3 or 5? > Shouldn't we abort() or mark with __builtin_unreachable()? In the referenced bug it was suggested to add a trailing "unreachable". But suggestion arrived after my MR, and I'm also not sure if there is a way to implement it for all supported compilers. Olaf
On 16/08/2023 14.49, Philippe Mathieu-Daudé wrote: > On 16/8/23 11:15, Thomas Huth wrote: >> Update the berkeley-testfloat-3 wrap to include a patch provided by >> Olaf Hering. This fixes a problem with "control reaches end of non-void >> function [-Werror=return-type]" compiler warning/errors that are now >> enabled by default in certain versions of GCC. > > e7af9751d9 has: > > --- a/source/genCases_f32.c > +++ b/source/genCases_f32.c > @@ -411,6 +411,7 @@ static float32_t f32Random( void ) > case 6: > return f32RandomQInfP3(); > case 7: > + default: > return f32RandomQInfPInf(); > } > > Why the default is with 7 and not 3 or 5? > Shouldn't we abort() or mark with __builtin_unreachable()? If you look at the code, it's really impossible to get any other values here, so an abort() or something similar would not really help to discover additional bugs (except buggy compilers maybe). So I personally don't care too much, but if it bugs you, feel free to send another patch on top of it. Thomas
diff --git a/subprojects/berkeley-testfloat-3.wrap b/subprojects/berkeley-testfloat-3.wrap index 6ad80a37b2..c86dc078a8 100644 --- a/subprojects/berkeley-testfloat-3.wrap +++ b/subprojects/berkeley-testfloat-3.wrap @@ -1,5 +1,5 @@ [wrap-git] url = https://gitlab.com/qemu-project/berkeley-testfloat-3 -revision = 40619cbb3bf32872df8c53cc457039229428a263 +revision = e7af9751d9f9fd3b47911f51a5cfd08af256a9ab patch_directory = berkeley-testfloat-3 depth = 1
Update the berkeley-testfloat-3 wrap to include a patch provided by Olaf Hering. This fixes a problem with "control reaches end of non-void function [-Werror=return-type]" compiler warning/errors that are now enabled by default in certain versions of GCC. Reported-by: Olaf Hering <olaf@aepfle.de> Signed-off-by: Thomas Huth <thuth@redhat.com> --- subprojects/berkeley-testfloat-3.wrap | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)