On Thu, 3 Jun 2021 at 22:49, Richard Henderson
<richard.hender...@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.ben...@linaro.org>
> Signed-off-by: Richard Henderson <richard.hender...@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
