Re: [Qemu-devel] [PULL 18/22] fpu/softfloat: re-factor int/uint to float

[Peter Maydell]

On 27 April 2018 at 14:49, Alex Bennée <address@hidden> wrote:
>
> Peter Maydell <address@hidden> writes:
>
>> On 21 February 2018 at 11:05, Alex Bennée <address@hidden> wrote:
>>> +/*
>>> + * Integer to float conversions
>>> + *
>>> + * Returns the result of converting the two's complement integer `a'
>>> + * to the floating-point format. The conversion is performed according
>>> + * to the IEC/IEEE Standard for Binary Floating-Point Arithmetic.
>>> + */
>>> +
>>> +static FloatParts int_to_float(int64_t a, float_status *status)
>>> +{
>>> +    FloatParts r;
>>> +    if (a == 0) {
>>> +        r.cls = float_class_zero;
>>> +        r.sign = false;
>>> +    } else if (a == (1ULL << 63)) {
>>> +        r.cls = float_class_normal;
>>> +        r.sign = true;
>>> +        r.frac = DECOMPOSED_IMPLICIT_BIT;
>>> +        r.exp = 63;
>>> +    } else {
>>> +        uint64_t f;
>>> +        if (a < 0) {
>>> +            f = -a;
>>> +            r.sign = true;
>>> +        } else {
>>> +            f = a;
>>> +            r.sign = false;
>>> +        }
>>> +        int shift = clz64(f) - 1;
>>> +        r.cls = float_class_normal;
>>> +        r.exp = (DECOMPOSED_BINARY_POINT - shift);
>>> +        r.frac = f << shift;
>>> +    }
>>> +
>>> +    return r;
>>> +}
>>
>> Hi -- Coverity complains about this function (CID1390635) because
>> there's a code path through it that doesn't fully initialize
>> the struct (the a == 0 case doesn't set r.frac), and it thinks
>> that "return r" is a 'use' of all fields in the structure.
>> I don't know why it doesn't complain about r.exp.
>>
>> Should we initialize all of r's fields anyway to shut it up,
>> or mark it as a Coverity false-positive?
>
> Hmm tricky - because in some cases we don't want to mess with it. The
> fcvt patch for example manually messed with the frac portion to deal
> with conversion. But it's done outside of the main canonicalize/pack
> routines.

Hmm? The problem is only in this specific function, which
currently returns an entirely uninitialized 'frac' field,
and could be made to return a zeroed field. There aren't
many other functions that create a FloatParts struct themselves
rather than modifying an existing one:
 * unpack_raw() sets all the fields
 * uint_to_float() has "FloatParts r = { .sign = false};"
   which implicitly sets all the other parts to 0

It doesn't seem too unreasonable to me to either have this
function start "FloatParts r = {};" or to init both exp
and frac in the "zero" case.

thanks
-- PMM