Re: [PULL 08/13] softfloat: Fix BAD_SHIFT from normalizeFloatx80Subnormal

[Aleksandar Markovic]

17:17 Pet, 10.04.2020. Richard Henderson <address@hidden> је написао/ла:
>
> On 4/10/20 2:38 AM, Aleksandar Markovic wrote:
> > 17:55 Uto, 07.04.2020. Alex Bennée <address@hidden
> > <mailto:address@hidden>> је написао/ла:
> >>
> >> From: Richard Henderson <address@hidden
> > <mailto:address@hidden>>
> >>
> >> All other calls to normalize*Subnormal detect zero input before
> >> the call -- this is the only outlier.  This case can happen with
> >> +0.0 + +0.0 = +0.0 or -0.0 + -0.0 = -0.0, so return a zero of
> >> the correct sign.
> >>
> >> Reported-by: Coverity (CID 1421991)
> >> Signed-off-by: Richard Henderson <address@hidden
> > <mailto:address@hidden>>
> >> Signed-off-by: Alex Bennée <address@hidden
> > <mailto:address@hidden>>
> >> Message-Id: <address@hidden
> > <mailto:address@hidden>>
> >> Message-Id: <address@hidden
> > <mailto:address@hidden>>
> >>
> >> diff --git a/fpu/softfloat.c b/fpu/softfloat.c
> >> index 301ce3b537b..ae6ba718540 100644
> >> --- a/fpu/softfloat.c
> >> +++ b/fpu/softfloat.c
> >> @@ -5856,6 +5856,9 @@ static floatx80 addFloatx80Sigs(floatx80 a, floatx80 b,
> > flag zSign,
> >>          zSig1 = 0;
> >>          zSig0 = aSig + bSig;
> >>          if ( aExp == 0 ) {
> >> +            if (zSig0 == 0) {
> >> +                return packFloatx80(zSign, 0, 0);
> >> +            }
> >>              normalizeFloatx80Subnormal( zSig0, &zExp, &zSig0 );
> >>              goto roundAndPack;
> >>          }
> >> --
> >> 2.20.1
> >>
> >>
> >
> > We in MIPS have extensive FP tests, that certainly include many cases of
> > operations with +0 and -0. And they are all correct even before this patch.
>
> This is for the 80-bit extended-double type, only used on x86 and m68k.  You
> will not execute this path using MIPS.
>

Thanks, Richard!

First and foremost, may health be with you and all people of United States and all Americas!!

Yes, the fact that m68k also uses 80-bit FP arithmetic was known to me. Though probably many people think 80-bit FP is limited to x86.

I was just afraid of some strange way that other targets may end up using the function in question. But again, thanks for reassurances!

> > Alex, from the commit message, it not clear if this is a fix of a bug (in which
> > case a test example would be useful to have, and the assesment on what
> > scenarios could be affected), or just a correction for some rare condition that
> > practically for all intents and purposes was never triggered, or perhaps
> > something third.
>
> This only avoids a Coverity out-of-range shift warning.
>
> Beforehand, we executed 0 << 64, got 0 as the result (regardless of whether or
> not the host truncates the shift count), and constructed the correctly signed
> fp zero in the end.
>
> There was more discussion about this in an earlier thread, associated with a
> different patch for this same problem:
>
> https://lists.nongnu.org/archive/html/qemu-devel/2020-03/msg08278.html
>

OK. I didn't have a chance to read this pattivular thread. Thanks for the pointer!

>
> > Secondly, and not related to this patch only, I see more and more patches
> > integrated into the main tree without "Reviewed-by:" tag. I don't think this is
> > the best way an open source community works. In my personal opinion, this must
> > stop.
>
> The only way to avoid this is to have more developers review code outside their
> own bailiwick.  The patch has been on the list for two weeks and was pinged
> twice.
>
> Although why Alex didn't add his own R-b to my patch when merging it to his
> branch, I don't know.
>

I also have a very similar impression as yours, that Alex in fact reviewed the patch (as if he implicitely gave R-B, but forgot to insert it in a hurry, given these hectic days around 5.0 final release).

Best regards, and best wishes to all Sietlans, or wherever you happen to live!

Aleksandar

>
> r~