Re: [PULL 06/29] softfloat: Move compare_floats to softfloat-parts.c.inc

[Peter Maydell <peter.maydell@linaro.org>]

On Thu, 3 Jun 2021 at 22:49, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Rename to parts$N_compare.  Rename all of the intermediate
> functions to ftype_do_compare.  Rename the hard-float functions
> to ftype_hs_compare.  Convert float128 to FloatParts128.
>
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

I was wading through some of this code trying to figure out
whether some of Coverity's new issues are false positives, and
noticed something odd about this old commit:

> diff --git a/fpu/softfloat.c b/fpu/softfloat.c
> index 4fee5a6cb7..6f1bbbe6cf 100644
> --- a/fpu/softfloat.c
> +++ b/fpu/softfloat.c
> @@ -882,6 +882,14 @@ static FloatParts128 *parts128_minmax(FloatParts128 *a, FloatParts128 *b,
>  #define parts_minmax(A, B, S, F) \
>      PARTS_GENERIC_64_128(minmax, A)(A, B, S, F)
>
> +static int parts64_compare(FloatParts64 *a, FloatParts64 *b,
> +                           float_status *s, bool q);
> +static int parts128_compare(FloatParts128 *a, FloatParts128 *b,
> +                            float_status *s, bool q);

Here we define these two functions as returning "int"...

> +static FloatRelation QEMU_FLATTEN
> +float16_do_compare(float16 a, float16 b, float_status *s, bool is_quiet)
>  {


> +    float16_unpack_canonical(&pa, a, s);
> +    float16_unpack_canonical(&pb, b, s);
> +    return parts_compare(&pa, &pb, s, is_quiet);
>  }

...but here we use the return value directly in a function
that returns a FloatRelation...

> diff --git a/fpu/softfloat-parts.c.inc b/fpu/softfloat-parts.c.inc
> index b9094768db..3dacb5b4f0 100644
> --- a/fpu/softfloat-parts.c.inc
> +++ b/fpu/softfloat-parts.c.inc
> @@ -1018,3 +1018,60 @@ static FloatPartsN *partsN(minmax)(FloatPartsN *a, FloatPartsN *b,
>      }
>      return cmp < 0 ? b : a;
>  }
> +
> +/*
> + * Floating point compare
> + */
> +static FloatRelation partsN(compare)(FloatPartsN *a, FloatPartsN *b,
> +                                     float_status *s, bool is_quiet)
> +{

...and unless I'm getting confused by the macro usage here,
the actual definition of the functions returns a FloatRelation.
(I'm not sure why the compiler doesn't complain about the mismatch.)

> +    int ab_mask = float_cmask(a->cls) | float_cmask(b->cls);
> +    int cmp;
> +
> +    if (likely(ab_mask == float_cmask_normal)) {
> +        if (a->sign != b->sign) {
> +            goto a_sign;
> +        }
> +        if (a->exp != b->exp) {
> +            cmp = a->exp < b->exp ? -1 : 1;
> +        } else {
> +            cmp = frac_cmp(a, b);
> +        }
> +        if (a->sign) {
> +            cmp = -cmp;
> +        }
> +        return cmp;

This code path seems to be written to assume an
integer -1 or 1 return value...

> +    }
> +
> +    if (unlikely(ab_mask & float_cmask_anynan)) {
> +        if (!is_quiet || (ab_mask & float_cmask_snan)) {
> +            float_raise(float_flag_invalid, s);
> +        }
> +        return float_relation_unordered;
> +    }
> +
> +    if (ab_mask & float_cmask_zero) {
> +        if (ab_mask == float_cmask_zero) {
> +            return float_relation_equal;
> +        } else if (a->cls == float_class_zero) {
> +            goto b_sign;
> +        } else {
> +            goto a_sign;
> +        }
> +    }
> +
> +    if (ab_mask == float_cmask_inf) {
> +        if (a->sign == b->sign) {
> +            return float_relation_equal;

...but code later in the function works with and returns the
float_relation_* enumeration values.

> +        }
> +    } else if (b->cls == float_class_inf) {
> +        goto b_sign;
> +    } else {
> +        g_assert(a->cls == float_class_inf);
> +    }
> +
> + a_sign:
> +    return a->sign ? float_relation_less : float_relation_greater;
> + b_sign:
> +    return b->sign ? float_relation_greater : float_relation_less;
> +}

FWIW, the Coverity issues are CID 1487134, 1487139, 1487151, 1487184,
where for some reason it thinks that floatx80_compare() and
floatx80_compare_quiet() can return 3 and thus that there is a
potential array overrun. (I've marked these all as false positives
in the UI, anyway.)

thanks
-- PMM