[PATCH] subprojects/berkeley-testfloat-3: Update to fix a problem with compiler warnings

[PATCH] subprojects/berkeley-testfloat-3: Update to fix a problem with compiler warnings

[Thomas Huth]

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(-)

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

Re: [PATCH] subprojects/berkeley-testfloat-3: Update to fix a problem with compiler warnings

[Philippe Mathieu-Daudé]

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(-)

Re: [PATCH] subprojects/berkeley-testfloat-3: Update to fix a problem with compiler warnings

[Peter Maydell]

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

Re: [PATCH] subprojects/berkeley-testfloat-3: Update to fix a problem with compiler warnings

[Alex Bennée]

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


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH] subprojects/berkeley-testfloat-3: Update to fix a problem with compiler warnings

[Olaf Hering]

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

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

[-- Attachment #2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] subprojects/berkeley-testfloat-3: Update to fix a problem with compiler warnings

[Thomas Huth]

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