diff mbox series

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

Message ID 20210603214131.629841-7-richard.henderson@linaro.org
State New
Headers show
Series softfloat patch queue | expand

Commit Message

Richard Henderson June 3, 2021, 9:41 p.m. UTC 
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>

---
 fpu/softfloat.c           | 208 ++++++++++++++------------------------
 fpu/softfloat-parts.c.inc |  57 +++++++++++
 2 files changed, 133 insertions(+), 132 deletions(-)

-- 
2.25.1

Comments

Peter Maydell March 31, 2022, 10:46 a.m. UTC | #1 
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
Richard Henderson March 31, 2022, 5:54 p.m. UTC | #2 
On 3/31/22 04:46, Peter Maydell wrote:
>> +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 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.)

That is an excellent question -- it really seems like it should have complained.

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

The FloatRelation enumerations *were* chosen to make this sort of negation work; only 
float_relation_unordered is an outlier.  But it would be clearer to use the enum type for 
cmp here.

> 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.)

Interesting about '3'.  I'll have a look.


r~
Peter Maydell March 31, 2022, 6:06 p.m. UTC | #3 
On Thu, 31 Mar 2022 at 18:54, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 3/31/22 04:46, Peter Maydell wrote:
> > 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.)
>
> Interesting about '3'.  I'll have a look.

Unfortunately it doesn't seem to give its reasoning for deciding
that the function can return [-1..3] rather than [-1..2].
But maybe it will make more sense to you.

PS: while you're there, there are also a bunch of new TCG related
issues where it alleges array indexes being out of bounds. I
suspect these are false positives, but it's probably faster
for you to analyse them. (I have a feeling Coverity can get
confused and claim an error because it's looking at an array
size it has cached from one target's NB_MMU_MODES value and
a code flow for a different target with a different NB_MMU_MODES.)

-- PMM
Richard Henderson April 1, 2022, 1:33 p.m. UTC | #4 
On 3/31/22 12:06, Peter Maydell wrote:
> PS: while you're there, there are also a bunch of new TCG related
> issues where it alleges array indexes being out of bounds. I
> suspect these are false positives, but it's probably faster
> for you to analyse them. (I have a feeling Coverity can get
> confused and claim an error because it's looking at an array
> size it has cached from one target's NB_MMU_MODES value and
> a code flow for a different target with a different NB_MMU_MODES.)

Given the placement of one of the notes,

1760 static void *atomic_mmu_lookup(CPUArchState *env, target_ulong addr,
1761                                MemOpIdx oi, int size, int prot,
1762                                uintptr_t retaddr)
1763 {
     	1. assignment: Assigning: mmu_idx = get_mmuidx(oi).
            The value of mmu_idx may now be up to 15.
1764     size_t mmu_idx = get_mmuidx(oi);

the range check in based only on the mask applied within get_mmuidx.
I'll try adding an assert vs NB_MMU_MODES within that function.


r~
diff mbox series

Patch

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);
+
+#define parts_compare(A, B, S, Q) \
+    PARTS_GENERIC_64_128(compare, A)(A, B, S, Q)
+
 /*
  * Helper functions for softfloat-parts.c.inc, per-size operations.
  */
@@ -3357,92 +3365,42 @@  MINMAX_2(float128)
 #undef MINMAX_1
 #undef MINMAX_2
 
-/* Floating point compare */
-static FloatRelation compare_floats(FloatParts64 a, FloatParts64 b, bool is_quiet,
-                                    float_status *s)
+/*
+ * Floating point compare
+ */
+
+static FloatRelation QEMU_FLATTEN
+float16_do_compare(float16 a, float16 b, float_status *s, bool is_quiet)
 {
-    if (is_nan(a.cls) || is_nan(b.cls)) {
-        if (!is_quiet ||
-            a.cls == float_class_snan ||
-            b.cls == float_class_snan) {
-            float_raise(float_flag_invalid, s);
-        }
-        return float_relation_unordered;
-    }
+    FloatParts64 pa, pb;
 
-    if (a.cls == float_class_zero) {
-        if (b.cls == float_class_zero) {
-            return float_relation_equal;
-        }
-        return b.sign ? float_relation_greater : float_relation_less;
-    } else if (b.cls == float_class_zero) {
-        return a.sign ? float_relation_less : float_relation_greater;
-    }
-
-    /* The only really important thing about infinity is its sign. If
-     * both are infinities the sign marks the smallest of the two.
-     */
-    if (a.cls == float_class_inf) {
-        if ((b.cls == float_class_inf) && (a.sign == b.sign)) {
-            return float_relation_equal;
-        }
-        return a.sign ? float_relation_less : float_relation_greater;
-    } else if (b.cls == float_class_inf) {
-        return b.sign ? float_relation_greater : float_relation_less;
-    }
-
-    if (a.sign != b.sign) {
-        return a.sign ? float_relation_less : float_relation_greater;
-    }
-
-    if (a.exp == b.exp) {
-        if (a.frac == b.frac) {
-            return float_relation_equal;
-        }
-        if (a.sign) {
-            return a.frac > b.frac ?
-                float_relation_less : float_relation_greater;
-        } else {
-            return a.frac > b.frac ?
-                float_relation_greater : float_relation_less;
-        }
-    } else {
-        if (a.sign) {
-            return a.exp > b.exp ? float_relation_less : float_relation_greater;
-        } else {
-            return a.exp > b.exp ? float_relation_greater : float_relation_less;
-        }
-    }
+    float16_unpack_canonical(&pa, a, s);
+    float16_unpack_canonical(&pb, b, s);
+    return parts_compare(&pa, &pb, s, is_quiet);
 }
 
-#define COMPARE(name, attr, sz)                                         \
-static int attr                                                         \
-name(float ## sz a, float ## sz b, bool is_quiet, float_status *s)      \
-{                                                                       \
-    FloatParts64 pa, pb;                                                \
-    float ## sz ## _unpack_canonical(&pa, a, s);                        \
-    float ## sz ## _unpack_canonical(&pb, b, s);                        \
-    return compare_floats(pa, pb, is_quiet, s);                         \
-}
-
-COMPARE(soft_f16_compare, QEMU_FLATTEN, 16)
-COMPARE(soft_f32_compare, QEMU_SOFTFLOAT_ATTR, 32)
-COMPARE(soft_f64_compare, QEMU_SOFTFLOAT_ATTR, 64)
-
-#undef COMPARE
-
 FloatRelation float16_compare(float16 a, float16 b, float_status *s)
 {
-    return soft_f16_compare(a, b, false, s);
+    return float16_do_compare(a, b, s, false);
 }
 
 FloatRelation float16_compare_quiet(float16 a, float16 b, float_status *s)
 {
-    return soft_f16_compare(a, b, true, s);
+    return float16_do_compare(a, b, s, true);
+}
+
+static FloatRelation QEMU_SOFTFLOAT_ATTR
+float32_do_compare(float32 a, float32 b, float_status *s, bool is_quiet)
+{
+    FloatParts64 pa, pb;
+
+    float32_unpack_canonical(&pa, a, s);
+    float32_unpack_canonical(&pb, b, s);
+    return parts_compare(&pa, &pb, s, is_quiet);
 }
 
 static FloatRelation QEMU_FLATTEN
-f32_compare(float32 xa, float32 xb, bool is_quiet, float_status *s)
+float32_hs_compare(float32 xa, float32 xb, float_status *s, bool is_quiet)
 {
     union_float32 ua, ub;
 
@@ -3463,25 +3421,36 @@  f32_compare(float32 xa, float32 xb, bool is_quiet, float_status *s)
     if (likely(isless(ua.h, ub.h))) {
         return float_relation_less;
     }
-    /* The only condition remaining is unordered.
+    /*
+     * The only condition remaining is unordered.
      * Fall through to set flags.
      */
  soft:
-    return soft_f32_compare(ua.s, ub.s, is_quiet, s);
+    return float32_do_compare(ua.s, ub.s, s, is_quiet);
 }
 
 FloatRelation float32_compare(float32 a, float32 b, float_status *s)
 {
-    return f32_compare(a, b, false, s);
+    return float32_hs_compare(a, b, s, false);
 }
 
 FloatRelation float32_compare_quiet(float32 a, float32 b, float_status *s)
 {
-    return f32_compare(a, b, true, s);
+    return float32_hs_compare(a, b, s, true);
+}
+
+static FloatRelation QEMU_SOFTFLOAT_ATTR
+float64_do_compare(float64 a, float64 b, float_status *s, bool is_quiet)
+{
+    FloatParts64 pa, pb;
+
+    float64_unpack_canonical(&pa, a, s);
+    float64_unpack_canonical(&pb, b, s);
+    return parts_compare(&pa, &pb, s, is_quiet);
 }
 
 static FloatRelation QEMU_FLATTEN
-f64_compare(float64 xa, float64 xb, bool is_quiet, float_status *s)
+float64_hs_compare(float64 xa, float64 xb, float_status *s, bool is_quiet)
 {
     union_float64 ua, ub;
 
@@ -3502,41 +3471,62 @@  f64_compare(float64 xa, float64 xb, bool is_quiet, float_status *s)
     if (likely(isless(ua.h, ub.h))) {
         return float_relation_less;
     }
-    /* The only condition remaining is unordered.
+    /*
+     * The only condition remaining is unordered.
      * Fall through to set flags.
      */
  soft:
-    return soft_f64_compare(ua.s, ub.s, is_quiet, s);
+    return float64_do_compare(ua.s, ub.s, s, is_quiet);
 }
 
 FloatRelation float64_compare(float64 a, float64 b, float_status *s)
 {
-    return f64_compare(a, b, false, s);
+    return float64_hs_compare(a, b, s, false);
 }
 
 FloatRelation float64_compare_quiet(float64 a, float64 b, float_status *s)
 {
-    return f64_compare(a, b, true, s);
+    return float64_hs_compare(a, b, s, true);
 }
 
 static FloatRelation QEMU_FLATTEN
-soft_bf16_compare(bfloat16 a, bfloat16 b, bool is_quiet, float_status *s)
+bfloat16_do_compare(bfloat16 a, bfloat16 b, float_status *s, bool is_quiet)
 {
     FloatParts64 pa, pb;
 
     bfloat16_unpack_canonical(&pa, a, s);
     bfloat16_unpack_canonical(&pb, b, s);
-    return compare_floats(pa, pb, is_quiet, s);
+    return parts_compare(&pa, &pb, s, is_quiet);
 }
 
 FloatRelation bfloat16_compare(bfloat16 a, bfloat16 b, float_status *s)
 {
-    return soft_bf16_compare(a, b, false, s);
+    return bfloat16_do_compare(a, b, s, false);
 }
 
 FloatRelation bfloat16_compare_quiet(bfloat16 a, bfloat16 b, float_status *s)
 {
-    return soft_bf16_compare(a, b, true, s);
+    return bfloat16_do_compare(a, b, s, true);
+}
+
+static FloatRelation QEMU_FLATTEN
+float128_do_compare(float128 a, float128 b, float_status *s, bool is_quiet)
+{
+    FloatParts128 pa, pb;
+
+    float128_unpack_canonical(&pa, a, s);
+    float128_unpack_canonical(&pb, b, s);
+    return parts_compare(&pa, &pb, s, is_quiet);
+}
+
+FloatRelation float128_compare(float128 a, float128 b, float_status *s)
+{
+    return float128_do_compare(a, b, s, false);
+}
+
+FloatRelation float128_compare_quiet(float128 a, float128 b, float_status *s)
+{
+    return float128_do_compare(a, b, s, true);
 }
 
 /* Multiply A by 2 raised to the power N.  */
@@ -6609,52 +6599,6 @@  FloatRelation floatx80_compare_quiet(floatx80 a, floatx80 b,
     return floatx80_compare_internal(a, b, 1, status);
 }
 
-static inline FloatRelation
-float128_compare_internal(float128 a, float128 b, bool is_quiet,
-                          float_status *status)
-{
-    bool aSign, bSign;
-
-    if (( ( extractFloat128Exp( a ) == 0x7fff ) &&
-          ( extractFloat128Frac0( a ) | extractFloat128Frac1( a ) ) ) ||
-        ( ( extractFloat128Exp( b ) == 0x7fff ) &&
-          ( extractFloat128Frac0( b ) | extractFloat128Frac1( b ) ) )) {
-        if (!is_quiet ||
-            float128_is_signaling_nan(a, status) ||
-            float128_is_signaling_nan(b, status)) {
-            float_raise(float_flag_invalid, status);
-        }
-        return float_relation_unordered;
-    }
-    aSign = extractFloat128Sign( a );
-    bSign = extractFloat128Sign( b );
-    if ( aSign != bSign ) {
-        if ( ( ( ( a.high | b.high )<<1 ) | a.low | b.low ) == 0 ) {
-            /* zero case */
-            return float_relation_equal;
-        } else {
-            return 1 - (2 * aSign);
-        }
-    } else {
-        if (a.low == b.low && a.high == b.high) {
-            return float_relation_equal;
-        } else {
-            return 1 - 2 * (aSign ^ ( lt128( a.high, a.low, b.high, b.low ) ));
-        }
-    }
-}
-
-FloatRelation float128_compare(float128 a, float128 b, float_status *status)
-{
-    return float128_compare_internal(a, b, 0, status);
-}
-
-FloatRelation float128_compare_quiet(float128 a, float128 b,
-                                     float_status *status)
-{
-    return float128_compare_internal(a, b, 1, status);
-}
-
 floatx80 floatx80_scalbn(floatx80 a, int n, float_status *status)
 {
     bool aSign;
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)
+{
+    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;
+    }
+
+    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;
+        }
+    } 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;
+}