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

[Richard Henderson]

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~