[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PULL 18/22] fpu/softfloat: re-factor int/uint to float
From: |
Alex Bennée |
Subject: |
Re: [Qemu-devel] [PULL 18/22] fpu/softfloat: re-factor int/uint to float |
Date: |
Tue, 08 May 2018 11:54:31 +0100 |
User-agent: |
mu4e 1.1.0; emacs 26.1 |
Peter Maydell <address@hidden> writes:
> 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.
Good point - it is the source of the FloatParts in this case. I'll init
it.
>
> thanks
> -- PMM
--
Alex Bennée