From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45491) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fG0GU-0001Cg-Aj for qemu-devel@nongnu.org; Tue, 08 May 2018 06:54:39 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1fG0GR-0007mi-6F for qemu-devel@nongnu.org; Tue, 08 May 2018 06:54:38 -0400 Received: from mail-wm0-x234.google.com ([2a00:1450:400c:c09::234]:38878) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1fG0GQ-0007mT-VT for qemu-devel@nongnu.org; Tue, 08 May 2018 06:54:35 -0400 Received: by mail-wm0-x234.google.com with SMTP id y189-v6so2500918wmc.3 for ; Tue, 08 May 2018 03:54:34 -0700 (PDT) References: <20180221110523.859-1-alex.bennee@linaro.org> <20180221110523.859-19-alex.bennee@linaro.org> <87zi1okbdo.fsf@linaro.org> From: Alex =?utf-8?Q?Benn=C3=A9e?= In-reply-to: Date: Tue, 08 May 2018 11:54:31 +0100 Message-ID: <87vabyv2mw.fsf@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PULL 18/22] fpu/softfloat: re-factor int/uint to float List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: QEMU Developers , Aurelien Jarno , Richard Henderson Peter Maydell writes: > On 27 April 2018 at 14:49, Alex Benn=C3=A9e wrot= e: >> >> Peter Maydell writes: >> >>> On 21 February 2018 at 11:05, Alex Benn=C3=A9e = 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 =3D=3D 0) { >>>> + r.cls =3D float_class_zero; >>>> + r.sign =3D false; >>>> + } else if (a =3D=3D (1ULL << 63)) { >>>> + r.cls =3D float_class_normal; >>>> + r.sign =3D true; >>>> + r.frac =3D DECOMPOSED_IMPLICIT_BIT; >>>> + r.exp =3D 63; >>>> + } else { >>>> + uint64_t f; >>>> + if (a < 0) { >>>> + f =3D -a; >>>> + r.sign =3D true; >>>> + } else { >>>> + f =3D a; >>>> + r.sign =3D false; >>>> + } >>>> + int shift =3D clz64(f) - 1; >>>> + r.cls =3D float_class_normal; >>>> + r.exp =3D (DECOMPOSED_BINARY_POINT - shift); >>>> + r.frac =3D 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 =3D=3D 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 =3D { .sign =3D 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 =3D {};" 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=C3=A9e