[v1,5/7] fpu/softfloat: avoid undefined behaviour when normalising empty sigs

Message ID 20200327094945.23768-6-alex.bennee@linaro.org
State New
Headers show
Series
  • A selection of sanitiser fixes
Related show

Commit Message

Alex Bennée March 27, 2020, 9:49 a.m.
The undefined behaviour checker pointed out that a shift of 64 would
lead to undefined behaviour.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
 fpu/softfloat.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

-- 
2.20.1

Comments

Peter Maydell March 27, 2020, 10:09 a.m. | #1
On Fri, 27 Mar 2020 at 09:49, Alex Bennée <alex.bennee@linaro.org> wrote:
>

> The undefined behaviour checker pointed out that a shift of 64 would

> lead to undefined behaviour.

>

> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

> ---

>  fpu/softfloat.c | 11 ++++++++---

>  1 file changed, 8 insertions(+), 3 deletions(-)

>

> diff --git a/fpu/softfloat.c b/fpu/softfloat.c

> index 301ce3b537b..444d35920dd 100644

> --- a/fpu/softfloat.c

> +++ b/fpu/softfloat.c

> @@ -3834,9 +3834,14 @@ void normalizeFloatx80Subnormal(uint64_t aSig, int32_t *zExpPtr,

>  {

>      int8_t shiftCount;

>

> -    shiftCount = clz64(aSig);

> -    *zSigPtr = aSig<<shiftCount;

> -    *zExpPtr = 1 - shiftCount;

> +    if (aSig) {

> +        shiftCount = clz64(aSig);

> +        *zSigPtr = aSig << shiftCount;

> +        *zExpPtr = 1 - shiftCount;

> +    } else {

> +        *zSigPtr = 0;

> +        *zExpPtr = 1 - 64;

> +    }

>  }


Ah yes, I saw this one in Coverity: CID 1421991.

RTH marked the Coverity issue as a false positive with the rationale
"We assume an out-of-range shift count is merely IMPLEMENTATION DEFINED
 and not UNDEFINED (in the Arm ARM sense), and so cannot turn a 0 value
 into a non-zero value."
but I think I disagree with that. We can assume that for the TCG IR
where we get to define shift semantics because we're doing the codegen,
but we can't assume it in C code, because it's not included in the set
of extended guarantees provided by -fwrapv as far as I know.

That said, is it valid for this function to be called with a zero
aSig value ? I think all these normalizeFloat*Subnormal() functions
assume non-zero sig input, and the only callsite where it's not clearly
obvious that this is obvious that the sig input is non-zero is the call to
normalizeFloatx80Subnormal() from addFloatx80Sigs(). So perhaps we
just need to check and fix that callsite ??

thanks
-- PMM
Aleksandar Markovic March 27, 2020, 10:13 a.m. | #2
11:53 Pet, 27.03.2020. Alex Bennée <alex.bennee@linaro.org> је написао/ла:
>

> The undefined behaviour checker


Alex, what exactly is "undefined behaviour checker"? If this is a test, can
you give us a link?

Sincerely,
Aleksandar

> pointed out that a shift of 64 would

> lead to undefined behaviour.

>

> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

> ---

>  fpu/softfloat.c | 11 ++++++++---

>  1 file changed, 8 insertions(+), 3 deletions(-)

>

> diff --git a/fpu/softfloat.c b/fpu/softfloat.c

> index 301ce3b537b..444d35920dd 100644

> --- a/fpu/softfloat.c

> +++ b/fpu/softfloat.c

> @@ -3834,9 +3834,14 @@ void normalizeFloatx80Subnormal(uint64_t aSig,

int32_t *zExpPtr,
>  {

>      int8_t shiftCount;

>

> -    shiftCount = clz64(aSig);

> -    *zSigPtr = aSig<<shiftCount;

> -    *zExpPtr = 1 - shiftCount;

> +    if (aSig) {

> +        shiftCount = clz64(aSig);

> +        *zSigPtr = aSig << shiftCount;

> +        *zExpPtr = 1 - shiftCount;

> +    } else {

> +        *zSigPtr = 0;

> +        *zExpPtr = 1 - 64;

> +    }

>  }

>

>

 /*----------------------------------------------------------------------------
> --

> 2.20.1

>

>
<p dir="ltr"></p>
<p dir="ltr">11:53 Pet, 27.03.2020. Alex Bennée &lt;<a href="mailto:alex.bennee@linaro.org">alex.bennee@linaro.org</a>&gt; је написао/ла:<br>
&gt;<br>
&gt; The undefined behaviour checker</p>
<p dir="ltr">Alex, what exactly is &quot;undefined behaviour checker&quot;? If this is a test, can you give us a link?</p>
<p dir="ltr">Sincerely,<br>
Aleksandar<br></p>
<p dir="ltr">&gt; pointed out that a shift of 64 would<br>
&gt; lead to undefined behaviour.<br>
&gt;<br>
&gt; Signed-off-by: Alex Bennée &lt;<a href="mailto:alex.bennee@linaro.org">alex.bennee@linaro.org</a>&gt;<br>
&gt; ---<br>
&gt;  fpu/softfloat.c | 11 ++++++++---<br>
&gt;  1 file changed, 8 insertions(+), 3 deletions(-)<br>
&gt;<br>
&gt; diff --git a/fpu/softfloat.c b/fpu/softfloat.c<br>
&gt; index 301ce3b537b..444d35920dd 100644<br>
&gt; --- a/fpu/softfloat.c<br>
&gt; +++ b/fpu/softfloat.c<br>
&gt; @@ -3834,9 +3834,14 @@ void normalizeFloatx80Subnormal(uint64_t aSig, int32_t *zExpPtr,<br>
&gt;  {<br>
&gt;      int8_t shiftCount;<br>
&gt;<br>
&gt; -    shiftCount = clz64(aSig);<br>
&gt; -    *zSigPtr = aSig&lt;&lt;shiftCount;<br>
&gt; -    *zExpPtr = 1 - shiftCount;<br>
&gt; +    if (aSig) {<br>
&gt; +        shiftCount = clz64(aSig);<br>
&gt; +        *zSigPtr = aSig &lt;&lt; shiftCount;<br>
&gt; +        *zExpPtr = 1 - shiftCount;<br>
&gt; +    } else {<br>
&gt; +        *zSigPtr = 0;<br>
&gt; +        *zExpPtr = 1 - 64;<br>
&gt; +    }<br>
&gt;  }<br>
&gt;<br>
&gt;  /*----------------------------------------------------------------------------<br>
&gt; -- <br>
&gt; 2.20.1<br>
&gt;<br>
&gt;<br>
</p>
Alex Bennée March 27, 2020, 10:31 a.m. | #3
Aleksandar Markovic <aleksandar.qemu.devel@gmail.com> writes:

> 11:53 Pet, 27.03.2020. Alex Bennée <alex.bennee@linaro.org> је написао/ла:

>>

>> The undefined behaviour checker

>

> Alex, what exactly is "undefined behaviour checker"? If this is a test, can

> you give us a link?


It's enabled by our sanitizers build:

  ../../configure --cc=clang-8 --cxx=clang++-8 --enable-sanitizers

>

> Sincerely,

> Aleksandar

>

>> pointed out that a shift of 64 would

>> lead to undefined behaviour.

>>

>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

>> ---

>>  fpu/softfloat.c | 11 ++++++++---

>>  1 file changed, 8 insertions(+), 3 deletions(-)

>>

>> diff --git a/fpu/softfloat.c b/fpu/softfloat.c

>> index 301ce3b537b..444d35920dd 100644

>> --- a/fpu/softfloat.c

>> +++ b/fpu/softfloat.c

>> @@ -3834,9 +3834,14 @@ void normalizeFloatx80Subnormal(uint64_t aSig,

> int32_t *zExpPtr,

>>  {

>>      int8_t shiftCount;

>>

>> -    shiftCount = clz64(aSig);

>> -    *zSigPtr = aSig<<shiftCount;

>> -    *zExpPtr = 1 - shiftCount;

>> +    if (aSig) {

>> +        shiftCount = clz64(aSig);

>> +        *zSigPtr = aSig << shiftCount;

>> +        *zExpPtr = 1 - shiftCount;

>> +    } else {

>> +        *zSigPtr = 0;

>> +        *zExpPtr = 1 - 64;

>> +    }

>>  }

>>

>>

>  /*----------------------------------------------------------------------------

>> --

>> 2.20.1

>>

>>



-- 
Alex Bennée
Richard Henderson March 27, 2020, 10:27 p.m. | #4
On 3/27/20 3:09 AM, Peter Maydell wrote:
> On Fri, 27 Mar 2020 at 09:49, Alex Bennée <alex.bennee@linaro.org> wrote:

>>

>> The undefined behaviour checker pointed out that a shift of 64 would

>> lead to undefined behaviour.

>>

>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

>> ---

>>  fpu/softfloat.c | 11 ++++++++---

>>  1 file changed, 8 insertions(+), 3 deletions(-)

>>

>> diff --git a/fpu/softfloat.c b/fpu/softfloat.c

>> index 301ce3b537b..444d35920dd 100644

>> --- a/fpu/softfloat.c

>> +++ b/fpu/softfloat.c

>> @@ -3834,9 +3834,14 @@ void normalizeFloatx80Subnormal(uint64_t aSig, int32_t *zExpPtr,

>>  {

>>      int8_t shiftCount;

>>

>> -    shiftCount = clz64(aSig);

>> -    *zSigPtr = aSig<<shiftCount;

>> -    *zExpPtr = 1 - shiftCount;

>> +    if (aSig) {

>> +        shiftCount = clz64(aSig);

>> +        *zSigPtr = aSig << shiftCount;

>> +        *zExpPtr = 1 - shiftCount;

>> +    } else {

>> +        *zSigPtr = 0;

>> +        *zExpPtr = 1 - 64;

>> +    }

>>  }

> 

> Ah yes, I saw this one in Coverity: CID 1421991.

> 

> RTH marked the Coverity issue as a false positive with the rationale

> "We assume an out-of-range shift count is merely IMPLEMENTATION DEFINED

>  and not UNDEFINED (in the Arm ARM sense), and so cannot turn a 0 value

>  into a non-zero value."

> but I think I disagree with that. We can assume that for the TCG IR

> where we get to define shift semantics because we're doing the codegen,

> but we can't assume it in C code, because it's not included in the set

> of extended guarantees provided by -fwrapv as far as I know.


Perhaps.  Of course we also know from our broad knowledge of architectures,
that a compiler would really have to go out of its way for this to happen.

I really hate C in this way, sometimes.

I wonder if I have the energy to petition the committee to drop, for C202? all
of the "undefined" nonsense that only applies to sign-magnitute and
ones-compliment computers, which haven't been seen since the 70's...

> That said, is it valid for this function to be called with a zero

> aSig value ? I think all these normalizeFloat*Subnormal() functions

> assume non-zero sig input, and the only callsite where it's not clearly

> obvious that this is obvious that the sig input is non-zero is the call to

> normalizeFloatx80Subnormal() from addFloatx80Sigs(). So perhaps we

> just need to check and fix that callsite ??


You're right -- addFloatx80Sigs is the only use out of 26 that doesn't have a
preceding check for 0.


r~
Peter Maydell March 27, 2020, 10:33 p.m. | #5
On Fri, 27 Mar 2020 at 22:27, Richard Henderson
<richard.henderson@linaro.org> wrote:
> I wonder if I have the energy to petition the committee to drop, for C202? all

> of the "undefined" nonsense that only applies to sign-magnitute and

> ones-compliment computers, which haven't been seen since the 70's...


There was certainly a proposal to do that (I think from a Google
engineer) for C++, I forget whether the equivalent C change has
also been proposed.

> > That said, is it valid for this function to be called with a zero

> > aSig value ? I think all these normalizeFloat*Subnormal() functions

> > assume non-zero sig input, and the only callsite where it's not clearly

> > obvious that this is obvious that the sig input is non-zero is the call to

> > normalizeFloatx80Subnormal() from addFloatx80Sigs(). So perhaps we

> > just need to check and fix that callsite ??

>

> You're right -- addFloatx80Sigs is the only use out of 26 that doesn't have a

> preceding check for 0.


Mmm. My vote is for fixing addFloatx80Sigs -- now we just need
to figure out what the desired behaviour is.

thanks
-- PMM

Patch

diff --git a/fpu/softfloat.c b/fpu/softfloat.c
index 301ce3b537b..444d35920dd 100644
--- a/fpu/softfloat.c
+++ b/fpu/softfloat.c
@@ -3834,9 +3834,14 @@  void normalizeFloatx80Subnormal(uint64_t aSig, int32_t *zExpPtr,
 {
     int8_t shiftCount;
 
-    shiftCount = clz64(aSig);
-    *zSigPtr = aSig<<shiftCount;
-    *zExpPtr = 1 - shiftCount;
+    if (aSig) {
+        shiftCount = clz64(aSig);
+        *zSigPtr = aSig << shiftCount;
+        *zExpPtr = 1 - shiftCount;
+    } else {
+        *zSigPtr = 0;
+        *zExpPtr = 1 - 64;
+    }
 }
 
 /*----------------------------------------------------------------------------