* [Qemu-devel] [PATCH] Fix: fp-test uninitialized member floatX::exp
@ 2019-07-30 10:13 Andrey Shinkevich
2019-08-13 12:03 ` Andrey Shinkevich
2019-08-20 15:16 ` Andrey Shinkevich
0 siblings, 2 replies; 9+ messages in thread
From: Andrey Shinkevich @ 2019-07-30 10:13 UTC (permalink / raw)
To: qemu-devel; +Cc: vsementsov, jhauser, cota, den, andrey.shinkevich, alex.bennee
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;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [Qemu-devel] [PATCH] Fix: fp-test uninitialized member floatX::exp
2019-07-30 10:13 [Qemu-devel] [PATCH] Fix: fp-test uninitialized member floatX::exp Andrey Shinkevich
@ 2019-08-13 12:03 ` Andrey Shinkevich
2019-08-13 12:21 ` Alex Bennée
2019-08-20 15:16 ` Andrey Shinkevich
1 sibling, 1 reply; 9+ messages in thread
From: Andrey Shinkevich @ 2019-08-13 12:03 UTC (permalink / raw)
To: qemu-devel
Cc: alex.bennee, cota, jhauser, Denis Lunev, Vladimir Sementsov-Ogievskiy
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;
>
--
With the best regards,
Andrey Shinkevich
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [Qemu-devel] [PATCH] Fix: fp-test uninitialized member floatX::exp
2019-08-13 12:03 ` Andrey Shinkevich
@ 2019-08-13 12:21 ` Alex Bennée
2019-08-20 17:01 ` Andrey Shinkevich
0 siblings, 1 reply; 9+ messages in thread
From: Alex Bennée @ 2019-08-13 12:21 UTC (permalink / raw)
To: Andrey Shinkevich
Cc: Vladimir Sementsov-Ogievskiy, cota, jhauser, qemu-devel, Denis Lunev
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [Qemu-devel] [PATCH] Fix: fp-test uninitialized member floatX::exp
2019-08-13 12:21 ` Alex Bennée
@ 2019-08-20 17:01 ` Andrey Shinkevich
2019-08-27 15:05 ` Andrey Shinkevich
2019-08-27 15:59 ` Alex Bennée
0 siblings, 2 replies; 9+ messages in thread
From: Andrey Shinkevich @ 2019-08-20 17:01 UTC (permalink / raw)
To: Alex Bennée
Cc: Denis Lunev, Vladimir Sementsov-Ogievskiy, jhauser, cota, qemu-devel
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
>
--
With the best regards,
Andrey Shinkevich
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [Qemu-devel] [PATCH] Fix: fp-test uninitialized member floatX::exp
2019-08-20 17:01 ` Andrey Shinkevich
@ 2019-08-27 15:05 ` Andrey Shinkevich
2019-08-27 15:59 ` Alex Bennée
1 sibling, 0 replies; 9+ messages in thread
From: Andrey Shinkevich @ 2019-08-27 15:05 UTC (permalink / raw)
To: Alex Bennée
Cc: Vladimir Sementsov-Ogievskiy, jhauser, Denis Lunev, cota, qemu-devel
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
>>
>
--
With the best regards,
Andrey Shinkevich
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [Qemu-devel] [PATCH] Fix: fp-test uninitialized member floatX::exp
2019-08-20 17:01 ` Andrey Shinkevich
2019-08-27 15:05 ` Andrey Shinkevich
@ 2019-08-27 15:59 ` Alex Bennée
2019-08-29 15:50 ` Andrey Shinkevich
1 sibling, 1 reply; 9+ messages in thread
From: Alex Bennée @ 2019-08-27 15:59 UTC (permalink / raw)
To: Andrey Shinkevich
Cc: Denis Lunev, Vladimir Sementsov-Ogievskiy, jhauser, cota, qemu-devel
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [Qemu-devel] [PATCH] Fix: fp-test uninitialized member floatX::exp
2019-08-27 15:59 ` Alex Bennée
@ 2019-08-29 15:50 ` Andrey Shinkevich
0 siblings, 0 replies; 9+ messages in thread
From: Andrey Shinkevich @ 2019-08-29 15:50 UTC (permalink / raw)
To: Alex Bennée
Cc: Denis Lunev, Vladimir Sementsov-Ogievskiy, jhauser, cota, qemu-devel
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
>
--
With the best regards,
Andrey Shinkevich
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] Fix: fp-test uninitialized member floatX::exp
2019-07-30 10:13 [Qemu-devel] [PATCH] Fix: fp-test uninitialized member floatX::exp Andrey Shinkevich
2019-08-13 12:03 ` Andrey Shinkevich
@ 2019-08-20 15:16 ` Andrey Shinkevich
2019-08-20 15:21 ` Peter Maydell
1 sibling, 1 reply; 9+ messages in thread
From: Andrey Shinkevich @ 2019-08-20 15:16 UTC (permalink / raw)
To: qemu-devel
Cc: alex.bennee, cota, jhauser, Denis Lunev, Vladimir Sementsov-Ogievskiy
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;
>
--
With the best regards,
Andrey Shinkevich
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [Qemu-devel] [PATCH] Fix: fp-test uninitialized member floatX::exp
2019-08-20 15:16 ` Andrey Shinkevich
@ 2019-08-20 15:21 ` Peter Maydell
0 siblings, 0 replies; 9+ messages in thread
From: Peter Maydell @ 2019-08-20 15:21 UTC (permalink / raw)
To: Andrey Shinkevich
Cc: Vladimir Sementsov-Ogievskiy, Denis Lunev, alex.bennee,
qemu-devel, cota, jhauser
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
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-08-29 15:51 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-30 10:13 [Qemu-devel] [PATCH] Fix: fp-test uninitialized member floatX::exp Andrey Shinkevich
2019-08-13 12:03 ` Andrey Shinkevich
2019-08-13 12:21 ` Alex Bennée
2019-08-20 17:01 ` Andrey Shinkevich
2019-08-27 15:05 ` Andrey Shinkevich
2019-08-27 15:59 ` Alex Bennée
2019-08-29 15:50 ` Andrey Shinkevich
2019-08-20 15:16 ` Andrey Shinkevich
2019-08-20 15:21 ` Peter Maydell
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).