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

[Richard Henderson <richard.henderson@linaro.org>]

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~