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