[v2] fpu: add mechanism to check for invalid long double formats

The macro require_valid_floatx80(value, status) will check for malformed
extended precision encodings, and if one is found, generate an
invalid-operation exception and return NaN. This check has been added to
the beginning of the basic 80-bit float arithmetic functions.

Version 2: fix code style

Signed-off-by: Andrew Dutcher <andrew@andrewdutcher.com>
---
 fpu/softfloat-specialize.h | 12 ++++++++++++
 fpu/softfloat.c            | 11 +++++++++++
 include/fpu/softfloat.h    | 13 +++++++++++++
 3 files changed, 36 insertions(+)

On 14 August 2016 at 01:33, Andrew Dutcher <andrew@andrewdutcher.com> wrote:
> The macro require_valid_floatx80(value, status) will check for malformed
> extended precision encodings, and if one is found, generate an
> invalid-operation exception and return NaN. This check has been added to
> the beginning of the basic 80-bit float arithmetic functions.

Hi; thanks for this patch. I think it's in general good,
but you haven't applied it to enough of the floatx80 functions.
We also need to check:
 * conversions from floatx80 to another float format
   (floatx80_to_float32/64/128)
 * conversions from floatx80 to integer
   (various floatx80_to_* functions)
 * comparisons
 * floatx80_scalbn (used in the FSCALE insn)
[basically we need to cover everything that the intel manual
 calls an "arithmetic instruction".]

I also have a few style issues which I'll remark on inline below.

> Version 2: fix code style

This sort of v1-to-v2 changelog should go below the '---' line,
so it doesn't go in the final commit message in git.

> Signed-off-by: Andrew Dutcher <andrew@andrewdutcher.com>
> ---
>  fpu/softfloat-specialize.h | 12 ++++++++++++
>  fpu/softfloat.c            | 11 +++++++++++
>  include/fpu/softfloat.h    | 13 +++++++++++++
>  3 files changed, 36 insertions(+)
>
> diff --git a/fpu/softfloat-specialize.h b/fpu/softfloat-specialize.h
> index 43d0890..0e6ec25 100644
> --- a/fpu/softfloat-specialize.h
> +++ b/fpu/softfloat-specialize.h
> @@ -203,6 +203,18 @@ void float_raise(int8_t flags, float_status *status)
>  }
>
>  /*----------------------------------------------------------------------------
> +| Asserts that the given value must be a valid floatx80 encoding. If The given
> +| value is a pseudo-NaN, pseudo-infiinty, or un-normal, raise an invalid
> +| operation exception and cause the parent function to return NaN.
> +*----------------------------------------------------------------------------*/
> +
> +#define require_valid_floatx80(a, status)           \
> +    if (floatx80_invalid_encoding((a))) {               \
> +        float_raise(float_flag_invalid, (status));        \
> +        return floatx80_default_nan((status));            \
> +    }

Macros that incorporate hidden flow control tend to be bad for
readability, and the softfloat code is already dangerously
close to unreadable. I think it would be better to just expand
this macro out in the places where you're using it: saying
    if (floatx80_invalid_encoding(a) || floatx80_invalid_encoding(b)) {
        float_raise(float_flag_invalid, status);
        return floatx80_default_nan(status);
    }

is a few more lines but I think clearer.

> +
> +/*----------------------------------------------------------------------------
>  | Internal canonical NaN format.
>  *----------------------------------------------------------------------------*/
>  typedef struct {
> diff --git a/fpu/softfloat.c b/fpu/softfloat.c
> index 9b1eccf..a921e5e 100644
> --- a/fpu/softfloat.c
> +++ b/fpu/softfloat.c
> @@ -5279,6 +5279,8 @@ floatx80 floatx80_add(floatx80 a, floatx80 b, float_status *status)
>  {
>      flag aSign, bSign;
>
> +    require_valid_floatx80(a, status);
> +    require_valid_floatx80(b, status);
>      aSign = extractFloatx80Sign( a );
>      bSign = extractFloatx80Sign( b );
>      if ( aSign == bSign ) {
> @@ -5300,6 +5302,8 @@ floatx80 floatx80_sub(floatx80 a, floatx80 b, float_status *status)
>  {
>      flag aSign, bSign;
>
> +    require_valid_floatx80(a, status);
> +    require_valid_floatx80(b, status);
>      aSign = extractFloatx80Sign( a );
>      bSign = extractFloatx80Sign( b );
>      if ( aSign == bSign ) {
> @@ -5323,6 +5327,8 @@ floatx80 floatx80_mul(floatx80 a, floatx80 b, float_status *status)
>      int32_t aExp, bExp, zExp;
>      uint64_t aSig, bSig, zSig0, zSig1;
>
> +    require_valid_floatx80(a, status);
> +    require_valid_floatx80(b, status);
>      aSig = extractFloatx80Frac( a );
>      aExp = extractFloatx80Exp( a );
>      aSign = extractFloatx80Sign( a );
> @@ -5380,6 +5386,8 @@ floatx80 floatx80_div(floatx80 a, floatx80 b, float_status *status)
>      uint64_t aSig, bSig, zSig0, zSig1;
>      uint64_t rem0, rem1, rem2, term0, term1, term2;
>
> +    require_valid_floatx80(a, status);
> +    require_valid_floatx80(b, status);
>      aSig = extractFloatx80Frac( a );
>      aExp = extractFloatx80Exp( a );
>      aSign = extractFloatx80Sign( a );
> @@ -5461,6 +5469,8 @@ floatx80 floatx80_rem(floatx80 a, floatx80 b, float_status *status)
>      uint64_t aSig0, aSig1, bSig;
>      uint64_t q, term0, term1, alternateASig0, alternateASig1;
>
> +    require_valid_floatx80(a, status);
> +    require_valid_floatx80(b, status);
>      aSig0 = extractFloatx80Frac( a );
>      aExp = extractFloatx80Exp( a );
>      aSign = extractFloatx80Sign( a );
> @@ -5556,6 +5566,7 @@ 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;
>
> +    require_valid_floatx80(a, status);
>      aSig0 = extractFloatx80Frac( a );
>      aExp = extractFloatx80Exp( a );
>      aSign = extractFloatx80Sign( a );
> diff --git a/include/fpu/softfloat.h b/include/fpu/softfloat.h
> index 0e57ee5..569730f 100644
> --- a/include/fpu/softfloat.h
> +++ b/include/fpu/softfloat.h
> @@ -671,6 +671,19 @@ static inline int floatx80_is_any_nan(floatx80 a)
>  floatx80 floatx80_default_nan(float_status *status);

I would move this function up to just below
floatx80_is_any_nan() so all the "return some property
of the floatx80 encoding" functions are together.

>
>  /*----------------------------------------------------------------------------
> +| 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.

We could say here also
"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 & 0x8000000000000000) && !!(a.high & 0x7FFF);

The '!!' isn't necessary, because && is a boolean operator.
If you want to be concise, you can just omit it. If you
want to be clear you can use "(a.high & 0x7fff) != 0".

"(1 << 63)" is easier to check against the spec than 0x8<lots of 0s>.

> +}
> +
> +

Stray extra blank line.

> +/*----------------------------------------------------------------------------
>  | Software IEC/IEEE quadruple-precision conversion routines.
>  *----------------------------------------------------------------------------*/
>  int32_t float128_to_int32(float128, float_status *status);
> --
> 2.7.4

thanks
-- PMM