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

["Alex Bennée" <alex.bennee@linaro.org>]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 27 April 2018 at 14:49, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>
>>> On 21 February 2018 at 11:05, Alex Bennée <alex.bennee@linaro.org> 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.

Good point - it is the source of the FloatParts in this case. I'll init
it.

>
> thanks
> -- PMM


--
Alex Bennée