[Qemu-devel] [PATCH v4] fpu: add mechanism to check for invalid long double formats

[Qemu-devel] [PATCH v4] fpu: add mechanism to check for invalid long double formats

[Andrew Dutcher]

All operations that take a floatx80 as an operand need to have their
inputs checked for malformed encodings. In all of these cases, use the
function floatx80_invalid_encoding to perform the check. If an invalid
operand is found, raise an invalid operation exception, and then return
either NaN (for fp-typed results) or the integer indefinite value (the
minimum representable signed integer value, for int-typed results).

Signed-off-by: Andrew Dutcher <andrew@andrewdutcher.com>
---

Version 4: Remove comments, since apparently it's still 1998. If anyone wants
to know what the value is for, they can check git blame.

 fpu/softfloat.c         | 60 +++++++++++++++++++++++++++++++++++++++++++++++++
 include/fpu/softfloat.h | 15 +++++++++++++
 2 files changed, 75 insertions(+)

diff --git a/fpu/softfloat.c b/fpu/softfloat.c
index 9b1eccf..5ff5df5 100644
--- a/fpu/softfloat.c
+++ b/fpu/softfloat.c
@@ -4814,6 +4814,10 @@ int32_t floatx80_to_int32(floatx80 a, float_status *status)
     int32_t aExp, shiftCount;
     uint64_t aSig;
 
+    if (floatx80_invalid_encoding(a)) {
+        float_raise(float_flag_invalid, status);
+        return 1 << 31;
+    }
     aSig = extractFloatx80Frac( a );
     aExp = extractFloatx80Exp( a );
     aSign = extractFloatx80Sign( a );
@@ -4842,6 +4846,10 @@ int32_t floatx80_to_int32_round_to_zero(floatx80 a, float_status *status)
     uint64_t aSig, savedASig;
     int32_t z;
 
+    if (floatx80_invalid_encoding(a)) {
+        float_raise(float_flag_invalid, status);
+        return 1 << 31;
+    }
     aSig = extractFloatx80Frac( a );
     aExp = extractFloatx80Exp( a );
     aSign = extractFloatx80Sign( a );
@@ -4888,6 +4896,10 @@ int64_t floatx80_to_int64(floatx80 a, float_status *status)
     int32_t aExp, shiftCount;
     uint64_t aSig, aSigExtra;
 
+    if (floatx80_invalid_encoding(a)) {
+        float_raise(float_flag_invalid, status);
+        return 1 << 63;
+    }
     aSig = extractFloatx80Frac( a );
     aExp = extractFloatx80Exp( a );
     aSign = extractFloatx80Sign( a );
@@ -4929,6 +4941,10 @@ int64_t floatx80_to_int64_round_to_zero(floatx80 a, float_status *status)
     uint64_t aSig;
     int64_t z;
 
+    if (floatx80_invalid_encoding(a)) {
+        float_raise(float_flag_invalid, status);
+        return 1 << 63;
+    }
     aSig = extractFloatx80Frac( a );
     aExp = extractFloatx80Exp( a );
     aSign = extractFloatx80Sign( a );
@@ -4971,6 +4987,10 @@ float32 floatx80_to_float32(floatx80 a, float_status *status)
     int32_t aExp;
     uint64_t aSig;
 
+    if (floatx80_invalid_encoding(a)) {
+        float_raise(float_flag_invalid, status);
+        return float32_default_nan(status);
+    }
     aSig = extractFloatx80Frac( a );
     aExp = extractFloatx80Exp( a );
     aSign = extractFloatx80Sign( a );
@@ -4999,6 +5019,10 @@ float64 floatx80_to_float64(floatx80 a, float_status *status)
     int32_t aExp;
     uint64_t aSig, zSig;
 
+    if (floatx80_invalid_encoding(a)) {
+        float_raise(float_flag_invalid, status);
+        return float64_default_nan(status);
+    }
     aSig = extractFloatx80Frac( a );
     aExp = extractFloatx80Exp( a );
     aSign = extractFloatx80Sign( a );
@@ -5027,6 +5051,10 @@ float128 floatx80_to_float128(floatx80 a, float_status *status)
     int aExp;
     uint64_t aSig, zSig0, zSig1;
 
+    if (floatx80_invalid_encoding(a)) {
+        float_raise(float_flag_invalid, status);
+        return float128_default_nan(status);
+    }
     aSig = extractFloatx80Frac( a );
     aExp = extractFloatx80Exp( a );
     aSign = extractFloatx80Sign( a );
@@ -5052,6 +5080,10 @@ floatx80 floatx80_round_to_int(floatx80 a, float_status *status)
     uint64_t lastBitMask, roundBitsMask;
     floatx80 z;
 
+    if (floatx80_invalid_encoding(a)) {
+        float_raise(float_flag_invalid, status);
+        return floatx80_default_nan(status);
+    }
     aExp = extractFloatx80Exp( a );
     if ( 0x403E <= aExp ) {
         if ( ( aExp == 0x7FFF ) && (uint64_t) ( extractFloatx80Frac( a )<<1 ) ) {
@@ -5279,6 +5311,10 @@ floatx80 floatx80_add(floatx80 a, floatx80 b, float_status *status)
 {
     flag aSign, bSign;
 
+    if (floatx80_invalid_encoding(a) || floatx80_invalid_encoding(b)) {
+        float_raise(float_flag_invalid, status);
+        return floatx80_default_nan(status);
+    }
     aSign = extractFloatx80Sign( a );
     bSign = extractFloatx80Sign( b );
     if ( aSign == bSign ) {
@@ -5300,6 +5336,10 @@ floatx80 floatx80_sub(floatx80 a, floatx80 b, float_status *status)
 {
     flag aSign, bSign;
 
+    if (floatx80_invalid_encoding(a) || floatx80_invalid_encoding(b)) {
+        float_raise(float_flag_invalid, status);
+        return floatx80_default_nan(status);
+    }
     aSign = extractFloatx80Sign( a );
     bSign = extractFloatx80Sign( b );
     if ( aSign == bSign ) {
@@ -5323,6 +5363,10 @@ floatx80 floatx80_mul(floatx80 a, floatx80 b, float_status *status)
     int32_t aExp, bExp, zExp;
     uint64_t aSig, bSig, zSig0, zSig1;
 
+    if (floatx80_invalid_encoding(a) || floatx80_invalid_encoding(b)) {
+        float_raise(float_flag_invalid, status);
+        return floatx80_default_nan(status);
+    }
     aSig = extractFloatx80Frac( a );
     aExp = extractFloatx80Exp( a );
     aSign = extractFloatx80Sign( a );
@@ -5380,6 +5424,10 @@ floatx80 floatx80_div(floatx80 a, floatx80 b, float_status *status)
     uint64_t aSig, bSig, zSig0, zSig1;
     uint64_t rem0, rem1, rem2, term0, term1, term2;
 
+    if (floatx80_invalid_encoding(a) || floatx80_invalid_encoding(b)) {
+        float_raise(float_flag_invalid, status);
+        return floatx80_default_nan(status);
+    }
     aSig = extractFloatx80Frac( a );
     aExp = extractFloatx80Exp( a );
     aSign = extractFloatx80Sign( a );
@@ -5461,6 +5509,10 @@ floatx80 floatx80_rem(floatx80 a, floatx80 b, float_status *status)
     uint64_t aSig0, aSig1, bSig;
     uint64_t q, term0, term1, alternateASig0, alternateASig1;
 
+    if (floatx80_invalid_encoding(a) || floatx80_invalid_encoding(b)) {
+        float_raise(float_flag_invalid, status);
+        return floatx80_default_nan(status);
+    }
     aSig0 = extractFloatx80Frac( a );
     aExp = extractFloatx80Exp( a );
     aSign = extractFloatx80Sign( a );
@@ -5556,6 +5608,10 @@ floatx80 floatx80_sqrt(floatx80 a, float_status *status)
     uint64_t aSig0, aSig1, zSig0, zSig1, doubleZSig0;
     uint64_t rem0, rem1, rem2, rem3, term0, term1, term2, term3;
 
+    if (floatx80_invalid_encoding(a)) {
+        float_raise(float_flag_invalid, status);
+        return floatx80_default_nan(status);
+    }
     aSig0 = extractFloatx80Frac( a );
     aExp = extractFloatx80Exp( a );
     aSign = extractFloatx80Sign( a );
@@ -7645,6 +7701,10 @@ floatx80 floatx80_scalbn(floatx80 a, int n, float_status *status)
     int32_t aExp;
     uint64_t aSig;
 
+    if (floatx80_invalid_encoding(a)) {
+        float_raise(float_flag_invalid, status);
+        return floatx80_default_nan(status);
+    }
     aSig = extractFloatx80Frac( a );
     aExp = extractFloatx80Exp( a );
     aSign = extractFloatx80Sign( a );
diff --git a/include/fpu/softfloat.h b/include/fpu/softfloat.h
index 0e57ee5..c2ef9f2 100644
--- a/include/fpu/softfloat.h
+++ b/include/fpu/softfloat.h
@@ -658,6 +658,21 @@ static inline int floatx80_is_any_nan(floatx80 a)
     return ((a.high & 0x7fff) == 0x7fff) && (a.low<<1);
 }
 
+/*----------------------------------------------------------------------------
+| Return whether the given value is an invalid floatx80 encoding.
+| Invalid floatx80 encodings arise when the integer bit is not set, but
+| the exponent is not zero. The only times the integer bit is permitted to
+| be zero is in subnormal numbers and the value zero.
+| This includes what the Intel software developer's manual calls pseudo-NaNs,
+| pseudo-infinities and un-normal numbers. It does not include
+| pseudo-denormals, which must still be correctly handled as inputs even
+| if they are never generated as outputs.
+*----------------------------------------------------------------------------*/
+static inline bool floatx80_invalid_encoding(floatx80 a)
+{
+    return (a.low & (1 << 63)) == 0 && (a.high & 0x7FFF) != 0;
+}
+
 #define floatx80_zero make_floatx80(0x0000, 0x0000000000000000LL)
 #define floatx80_one make_floatx80(0x3fff, 0x8000000000000000LL)
 #define floatx80_ln2 make_floatx80(0x3ffe, 0xb17217f7d1cf79acLL)
-- 
2.7.4

Re: [Qemu-devel] [PATCH v4] fpu: add mechanism to check for invalid long double formats

[Peter Maydell]

On 15 August 2016 at 23:27, Andrew Dutcher <andrew@andrewdutcher.com> wrote:
> All operations that take a floatx80 as an operand need to have their
> inputs checked for malformed encodings. In all of these cases, use the
> function floatx80_invalid_encoding to perform the check. If an invalid
> operand is found, raise an invalid operation exception, and then return
> either NaN (for fp-typed results) or the integer indefinite value (the
> minimum representable signed integer value, for int-typed results).
>
> Signed-off-by: Andrew Dutcher <andrew@andrewdutcher.com>
> ---
>
> Version 4: Remove comments, since apparently it's still 1998. If anyone wants
> to know what the value is for, they can check git blame.

The code style gripe is not for having comments, it's for
using the "//" style comment rather than "/* ... */". Yeah,
we have some odd style requirements; at least we have a script
which will detect them automatically. (By the way, you can run
./scripts/checkpatch.pl on your patch before sending it if you
like; you don't have to wait for the patch robot to read your
email :-))

If you liked you could  define a constant for the 32-bit and
64-bit indefinite values rather than using 1 << 31 &c directly;
'return int32_indefinite;' is sufficiently self-documenting
not to need a comment.

I don't mind whether you do that, or not; your choice.

The code changes you have here are good, but you've forgotten
one piece: the comparison ops also need to handle the invalid
encodings.

floatx80_compare_internal() needs to raise float_flag_invalid
and return float_relation_unordered if either of its inputs are
invalid encodings.

There are also separate single-comparison functions:
floatx80_eq(), floatx80_le(), floatx80_lt(), floatx80_unordered(),
floatx80_eq_quiet(), floatx80_le_quiet(), floatx80_lt_quiet(),
floatx80_unordered_quiet(). The i386 guest doesn't use them but
I think for consistency we should treat invalid encodings like
NaNs there: raise float_flag_invalid and return 0.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v4] fpu: add mechanism to check for invalid long double formats

[Andrew Dutcher]

I explicitly left the check off the comparison operations because I
misread the NaN check as something equivalent to the check I would be
adding. I'll add it shortly.

With regards to adding int32_indefinite, etc constants, I think I'll
leave it as is -- I'd prefer to have *what* happens clear (return this
number), then have *why* it happens be clear (return the integer
indefinite value).

And, finally, yes, I know what C++ and C-style comments are :P I just
think every argument I've ever read in favor of only using the latter
has been complete nonsense. Regardless! Guidelines are guidelines.

Thanks,
- Andrew

On Tue, Aug 16, 2016 at 6:34 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 15 August 2016 at 23:27, Andrew Dutcher <andrew@andrewdutcher.com> wrote:
>> All operations that take a floatx80 as an operand need to have their
>> inputs checked for malformed encodings. In all of these cases, use the
>> function floatx80_invalid_encoding to perform the check. If an invalid
>> operand is found, raise an invalid operation exception, and then return
>> either NaN (for fp-typed results) or the integer indefinite value (the
>> minimum representable signed integer value, for int-typed results).
>>
>> Signed-off-by: Andrew Dutcher <andrew@andrewdutcher.com>
>> ---
>>
>> Version 4: Remove comments, since apparently it's still 1998. If anyone wants
>> to know what the value is for, they can check git blame.
>
> The code style gripe is not for having comments, it's for
> using the "//" style comment rather than "/* ... */". Yeah,
> we have some odd style requirements; at least we have a script
> which will detect them automatically. (By the way, you can run
> ./scripts/checkpatch.pl on your patch before sending it if you
> like; you don't have to wait for the patch robot to read your
> email :-))
>
> If you liked you could  define a constant for the 32-bit and
> 64-bit indefinite values rather than using 1 << 31 &c directly;
> 'return int32_indefinite;' is sufficiently self-documenting
> not to need a comment.
>
> I don't mind whether you do that, or not; your choice.
>
> The code changes you have here are good, but you've forgotten
> one piece: the comparison ops also need to handle the invalid
> encodings.
>
> floatx80_compare_internal() needs to raise float_flag_invalid
> and return float_relation_unordered if either of its inputs are
> invalid encodings.
>
> There are also separate single-comparison functions:
> floatx80_eq(), floatx80_le(), floatx80_lt(), floatx80_unordered(),
> floatx80_eq_quiet(), floatx80_le_quiet(), floatx80_lt_quiet(),
> floatx80_unordered_quiet(). The i386 guest doesn't use them but
> I think for consistency we should treat invalid encodings like
> NaNs there: raise float_flag_invalid and return 0.
>
> thanks
> -- PMM

Re: [Qemu-devel] [PATCH v4] fpu: add mechanism to check for invalid long double formats

[Andrew Dutcher]

Also- I'm having issues applying the new patch:

@@ -5768,7 +5774,9 @@ int floatx80_lt(floatx80 a, floatx80 b,
float_status *status)
 *----------------------------------------------------------------------------*/
 int floatx80_unordered(floatx80 a, floatx80 b, float_status *status)
 {
-    if (    (    ( extractFloatx80Exp( a ) == 0x7FFF )
+    if (    floatx80_invalid_encoding( a )
+         || floatx80_invalid_encoding( b )
+         || (    ( extractFloatx80Exp( a ) == 0x7FFF )
               && (uint64_t) ( extractFloatx80Frac( a )<<1 ) )
          || (    ( extractFloatx80Exp( b ) == 0x7FFF )
               && (uint64_t) ( extractFloatx80Frac( b )<<1 ) )

When I do this, the style checker complains about the spaces after the
open parens and before the close parens. I now have to change this
entire stanza to be styled correctly, since I'm replacing the original
first line...

On Tue, Aug 16, 2016 at 3:59 PM, Andrew Dutcher
<andrew@andrewdutcher.com> wrote:
> I explicitly left the check off the comparison operations because I
> misread the NaN check as something equivalent to the check I would be
> adding. I'll add it shortly.
>
> With regards to adding int32_indefinite, etc constants, I think I'll
> leave it as is -- I'd prefer to have *what* happens clear (return this
> number), then have *why* it happens be clear (return the integer
> indefinite value).
>
> And, finally, yes, I know what C++ and C-style comments are :P I just
> think every argument I've ever read in favor of only using the latter
> has been complete nonsense. Regardless! Guidelines are guidelines.
>
> Thanks,
> - Andrew
>
> On Tue, Aug 16, 2016 at 6:34 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 15 August 2016 at 23:27, Andrew Dutcher <andrew@andrewdutcher.com> wrote:
>>> All operations that take a floatx80 as an operand need to have their
>>> inputs checked for malformed encodings. In all of these cases, use the
>>> function floatx80_invalid_encoding to perform the check. If an invalid
>>> operand is found, raise an invalid operation exception, and then return
>>> either NaN (for fp-typed results) or the integer indefinite value (the
>>> minimum representable signed integer value, for int-typed results).
>>>
>>> Signed-off-by: Andrew Dutcher <andrew@andrewdutcher.com>
>>> ---
>>>
>>> Version 4: Remove comments, since apparently it's still 1998. If anyone wants
>>> to know what the value is for, they can check git blame.
>>
>> The code style gripe is not for having comments, it's for
>> using the "//" style comment rather than "/* ... */". Yeah,
>> we have some odd style requirements; at least we have a script
>> which will detect them automatically. (By the way, you can run
>> ./scripts/checkpatch.pl on your patch before sending it if you
>> like; you don't have to wait for the patch robot to read your
>> email :-))
>>
>> If you liked you could  define a constant for the 32-bit and
>> 64-bit indefinite values rather than using 1 << 31 &c directly;
>> 'return int32_indefinite;' is sufficiently self-documenting
>> not to need a comment.
>>
>> I don't mind whether you do that, or not; your choice.
>>
>> The code changes you have here are good, but you've forgotten
>> one piece: the comparison ops also need to handle the invalid
>> encodings.
>>
>> floatx80_compare_internal() needs to raise float_flag_invalid
>> and return float_relation_unordered if either of its inputs are
>> invalid encodings.
>>
>> There are also separate single-comparison functions:
>> floatx80_eq(), floatx80_le(), floatx80_lt(), floatx80_unordered(),
>> floatx80_eq_quiet(), floatx80_le_quiet(), floatx80_lt_quiet(),
>> floatx80_unordered_quiet(). The i386 guest doesn't use them but
>> I think for consistency we should treat invalid encodings like
>> NaNs there: raise float_flag_invalid and return 0.
>>
>> thanks
>> -- PMM

Re: [Qemu-devel] [PATCH v4] fpu: add mechanism to check for invalid long double formats

[Peter Maydell]

On 17 August 2016 at 01:06, Andrew Dutcher <andrew@andrewdutcher.com> wrote:
> Also- I'm having issues applying the new patch:
>
> @@ -5768,7 +5774,9 @@ int floatx80_lt(floatx80 a, floatx80 b,
> float_status *status)
>  *----------------------------------------------------------------------------*/
>  int floatx80_unordered(floatx80 a, floatx80 b, float_status *status)
>  {
> -    if (    (    ( extractFloatx80Exp( a ) == 0x7FFF )
> +    if (    floatx80_invalid_encoding( a )
> +         || floatx80_invalid_encoding( b )
> +         || (    ( extractFloatx80Exp( a ) == 0x7FFF )
>                && (uint64_t) ( extractFloatx80Frac( a )<<1 ) )
>           || (    ( extractFloatx80Exp( b ) == 0x7FFF )
>                && (uint64_t) ( extractFloatx80Frac( b )<<1 ) )
>
> When I do this, the style checker complains about the spaces after the
> open parens and before the close parens. I now have to change this
> entire stanza to be styled correctly, since I'm replacing the original
> first line...

Yes, that's the best approach. softfloat has some very weird
spacing patterns because it's third-party code. Generally
what we do is update it to match QEMU's coding style when
we have to touch a part of the code for some other reason,
which can mean needing to reindent some lines around the
ones being changed to keep checkpatch happy.

thanks
-- PMM