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

[Richard Henderson <richard.henderson@linaro.org>]

On 4/10/20 2:38 AM, Aleksandar Markovic wrote:
> 17:55 Uto, 07.04.2020. Alex Bennée <alex.bennee@linaro.org
> <mailto:alex.bennee@linaro.org>> је написао/ла:
>>
>> From: Richard Henderson <richard.henderson@linaro.org
> <mailto:richard.henderson@linaro.org>>
>>
>> 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 <richard.henderson@linaro.org
> <mailto:richard.henderson@linaro.org>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org
> <mailto:alex.bennee@linaro.org>>
>> Message-Id: <20200327232042.10008-1-richard.henderson@linaro.org
> <mailto:20200327232042.10008-1-richard.henderson@linaro.org>>
>> Message-Id: <20200403191150.863-8-alex.bennee@linaro.org
> <mailto:20200403191150.863-8-alex.bennee@linaro.org>>
>>
>> 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.

> 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


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


r~