Re: [Qemu-devel] [PATCH] Fix: fp-test uninitialized member floatX::exp

[Andrey Shinkevich]

On 13/08/2019 15:21, Alex Bennée wrote:
> 
> Andrey Shinkevich <address@hidden> writes:
> 
>> PINGING...
> 
> Sorry about the delay. I did attempt see if the existing code threw up
> any errors when built with clang's undefined sanitizer. I think this is
> because xPtr->exp will only get read if none of the xPtr->isFOO returns
> false. In all those cases xPtr->exp is set.
> 
> What pointed you towards this missing initialisations?
> 

I am sorry about missing the message. It appeared in other email thread 
where I didn't expect. So, I missed the response.
When I ran the fp-tests under the Valgrind, I got lots of reports about 
using uninitialized memory. They all disappeared after applying this 
patch. I concluded that there are paths that use xPtr->exp uninitialized.

$ /usr/bin/valgrind --leak-check=no --trace-children=yes 
--keep-stacktraces=alloc-and-free --track-origins=yes 
--log-file=myqemu-%p.log make check-softfloat

==720268== Conditional jump or move depends on uninitialised value(s)
==720268==    at 0x112C72: floatXRoundToInt (slowfloat.c:1371)
==720268==    by 0x115920: slow_f16_roundToInt (slowfloat.c:2408)
==720268==    by 0x133A87: test_az_f16_rx (test_az_f16_rx.c:73)
==720268==    by 0x10E635: do_testfloat (fp-test.c:304)
==720268==    by 0x10FD02: run_test (fp-test.c:1003)
==720268==    by 0x10FDA4: main (fp-test.c:1017)
==720268==  Uninitialised value was created by a stack allocation
==720268==    at 0x1158D3: slow_f16_roundToInt (slowfloat.c:2404)

==720311== Conditional jump or move depends on uninitialised value(s)
==720311==    at 0x112E54: floatXAdd (slowfloat.c:1411)
==720311==    by 0x115A2D: slow_f16_sub (slowfloat.c:2431)
==720311==    by 0x133CEC: test_abz_f16 (test_abz_f16.c:70)
==720311==    by 0x10E6D5: do_testfloat (fp-test.c:326)
==720311==    by 0x10FD02: run_test (fp-test.c:1003)
==720311==    by 0x10FDA4: main (fp-test.c:1017)
==720311==  Uninitialised value was created by a stack allocation
==720311==    at 0x1159C0: slow_f16_sub (slowfloat.c:2425)

==720273== Conditional jump or move depends on uninitialised value(s)
==720273==    at 0x113D54: floatXEq (slowfloat.c:1661)
==720273==    by 0x115EAD: slow_f16_eq_signaling (slowfloat.c:2538)
==720273==    by 0x1341D3: test_ab_f16_z_bool (test_ab_f16_z_bool.c:71)
==720273==    by 0x10E7DE: do_testfloat (fp-test.c:358)
==720273==    by 0x10FD02: run_test (fp-test.c:1003)
==720273==    by 0x10FDA4: main (fp-test.c:1017)
==720273==  Uninitialised value was created by a stack allocation
==720273==    at 0x115E38: slow_f16_eq_signaling (slowfloat.c:2530)

Even if Valgrind is wrong, the purpose of the patch is to reduce the 
number of error reports from the Valgrind to locate other memory serious 
issues, if any.

Andrey

>>
>> On 30/07/2019 13:13, Andrey Shinkevich wrote:
>>> Not all the paths in the functions, such as f16ToFloatX(), initialize
>>> the member 'exp' of the structure floatX.
>>>
>>> Signed-off-by: Andrey Shinkevich <address@hidden>
>>> ---
>>>    source/slowfloat.c | 4 ++++
>>>    1 file changed, 4 insertions(+)
>>>
>>> diff --git a/tests/fp/berkeley-testfloat-3/source/slowfloat.c 
>>> b/tests/fp/berkeley-testfloat-3/source/slowfloat.c
>>> index 4e84656..6e0f0a6 100644
>>> --- a/tests/fp/berkeley-testfloat-3/source/slowfloat.c
>>> +++ b/tests/fp/berkeley-testfloat-3/source/slowfloat.c
>>> @@ -623,6 +623,7 @@ static void f16ToFloatX( float16_t a, struct floatX 
>>> *xPtr )
>>>        xPtr->isInf = false;
>>>        xPtr->isZero = false;
>>>        xPtr->sign = ((uiA & 0x8000) != 0);
>>> +    xPtr->exp = 0;
>>>        exp = uiA>>10 & 0x1F;
>>>        sig64 = uiA & 0x03FF;
>>>        sig64 <<= 45;
>>> @@ -759,6 +760,7 @@ static void f32ToFloatX( float32_t a, struct floatX 
>>> *xPtr )
>>>        xPtr->isInf = false;
>>>        xPtr->isZero = false;
>>>        xPtr->sign = ((uiA & 0x80000000) != 0);
>>> +    xPtr->exp = 0;
>>>        exp = uiA>>23 & 0xFF;
>>>        sig64 = uiA & 0x007FFFFF;
>>>        sig64 <<= 32;
>>> @@ -895,6 +897,7 @@ static void f64ToFloatX( float64_t a, struct floatX 
>>> *xPtr )
>>>        xPtr->isInf = false;
>>>        xPtr->isZero = false;
>>>        xPtr->sign = ((uiA & UINT64_C( 0x8000000000000000 )) != 0);
>>> +    xPtr->exp = 0;
>>>        exp = uiA>>52 & 0x7FF;
>>>        sig64 = uiA & UINT64_C( 0x000FFFFFFFFFFFFF );
>>>        if ( exp == 0x7FF ) {
>>> @@ -1220,6 +1223,7 @@ static void f128MToFloatX( const float128_t *aPtr, 
>>> struct floatX *xPtr )
>>>        xPtr->isZero = false;
>>>        uiA64 = uiAPtr->v64;
>>>        xPtr->sign = ((uiA64 & UINT64_C( 0x8000000000000000 )) != 0);
>>> +    xPtr->exp = 0;
>>>        exp = uiA64>>48 & 0x7FFF;
>>>        sig.v64 = uiA64 & UINT64_C( 0x0000FFFFFFFFFFFF );
>>>        sig.v0  = uiAPtr->v0;
>>>
> 
> 
> --
> Alex Bennée
> 

-- 
With the best regards,
Andrey Shinkevich