| Message ID | 1564481593-776647-1-git-send-email-andrey.shinkevich@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix: fp-test uninitialized member floatX::exp | expand |
PINGING... 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 <andrey.shinkevich@virtuozzo.com> > --- > 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; >
Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> 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? > > 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 <andrey.shinkevich@virtuozzo.com> >> --- >> 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
PINGING... 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 <andrey.shinkevich@virtuozzo.com> > --- > 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; >
On Tue, 20 Aug 2019 at 16:18, Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> wrote: > > PINGING... Hi -- Alex asked you a question on the 13th when you last pinged this patch, which I don't think you've replied to yet (or if you did then my mail client has filed it somewhere unhelpful). thanks -- PMM
On 13/08/2019 15:21, Alex Bennée wrote: > > Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> 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 <andrey.shinkevich@virtuozzo.com> >>> --- >>> 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 >
I just worry about this simple patch to be lost because I got some mess with it in my Thunderbird email threads. Andrey On 20/08/2019 20:01, Andrey Shinkevich wrote: > On 13/08/2019 15:21, Alex Bennée wrote: >> >> Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> 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 <andrey.shinkevich@virtuozzo.com> >>>> --- >>>> 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 >> >
Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> writes: > On 13/08/2019 15:21, Alex Bennée wrote: >> >> Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> 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 It would be useful to know what tests are being run (V=1 will show you). I can't replicate the failure with: valgrind --leak-check=no --trace-children=yes --keep-stacktraces=alloc-and-free --track-origins=yes ./fp-test -s -l 2 -r all f16_to_f32 f16_to_f64 f16_to_f128 f32_to_f16 f32_to_f64 > > ==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 <andrey.shinkevich@virtuozzo.com> >>>> --- >>>> 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 >> -- Alex Bennée
On 27/08/2019 18:59, Alex Bennée wrote: > > Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> writes: > >> On 13/08/2019 15:21, Alex Bennée wrote: >>> >>> Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> 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 > > It would be useful to know what tests are being run (V=1 will show you). > I can't replicate the failure with: > > valgrind --leak-check=no --trace-children=yes --keep-stacktraces=alloc-and-free --track-origins=yes ./fp-test -s -l 2 -r all f16_to_f32 f16_to_f64 f16_to_f128 f32_to_f16 f32_to_f64 > They are ./fp-test -s -l 1 -r all f16_roundToInt f32_roundToInt f64_roundToInt f128_roundToInt ./fp-test -s -l 1 -r all f16_eq f32_eq f64_eq extF80_eq f128_eq ./fp-test -s -l 1 -r all f16_eq_signaling f32_eq_signaling f64_eq_signaling extF80_eq_signaling f128_eq_signaling ./fp-test -s -l 1 -r all f16_add f32_add f64_add extF80_add f128_add ./fp-test -s -l 1 -r all f16_sub f32_sub f64_sub extF80_sub f128_sub Andrey >> >> ==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 <andrey.shinkevich@virtuozzo.com> >>>>> --- >>>>> 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 >>> > > > -- > Alex Bennée >
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;
Not all the paths in the functions, such as f16ToFloatX(), initialize the member 'exp' of the structure floatX. Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> --- source/slowfloat.c | 4 ++++ 1 file changed, 4 insertions(+)