From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=42914 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OshGz-0006DV-VQ for qemu-devel@nongnu.org; Mon, 06 Sep 2010 15:25:18 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OshG9-0004m1-3I for qemu-devel@nongnu.org; Mon, 06 Sep 2010 15:21:42 -0400 Received: from mail-qw0-f45.google.com ([209.85.216.45]:50447) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OshG8-0004lv-VN for qemu-devel@nongnu.org; Mon, 06 Sep 2010 15:21:41 -0400 Received: by qwh5 with SMTP id 5so4209060qwh.4 for ; Mon, 06 Sep 2010 12:21:40 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <4C84E6E4.6030909@redhat.com> References: <4C84E6E4.6030909@redhat.com> From: Blue Swirl Date: Mon, 6 Sep 2010 19:21:20 +0000 Message-ID: Subject: Re: [Qemu-devel] [PATCH 1/5] Suppress some gcc warnings with -Wtype-limits Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: qemu-devel On Mon, Sep 6, 2010 at 1:04 PM, Kevin Wolf wrote: > Am 04.09.2010 23:07, schrieb Blue Swirl: >> On Sat, Sep 4, 2010 at 8:30 PM, andrzej zaborowski w= rote: >>> Hi, >>> >>> On 4 September 2010 21:45, Blue Swirl wrote: >>>> On Sat, Sep 4, 2010 at 5:57 PM, andrzej zaborowski = wrote: >>>>>> =C2=A0- =C2=A0 =C2=A0if (event < 0 || event >=3D BLKDBG_EVENT_MAX) { >>>>>> =C2=A0+ =C2=A0 =C2=A0if ((int)event < 0 || event >=3D BLKDBG_EVENT_M= AX) { >>>>> Is the behaviour incorrect for some values, or is it not correct C? >>>>> As far as I know it is correct in c99 because of type promotions >>>>> between enum, int and unsigned int. >>>> >>>> The problem is that the type of an enum may be signed or unsigned, >>>> which affects the comparison. For example, the following program >>>> produces different results when it's compiled with -DUNSIGNED and >>>> without: >>>> $ cat enum.c >>>> #include >>>> >>>> int main(int argc, const char **argv) >>>> { >>>> =C2=A0 =C2=A0enum { >>>> #ifdef UNSIGNED >>>> =C2=A0 =C2=A0 =C2=A0 =C2=A0A =3D 0, >>>> #else >>>> =C2=A0 =C2=A0 =C2=A0 =C2=A0A =3D -1, >>>> #endif >>>> =C2=A0 =C2=A0} a; >>>> >>>> =C2=A0 =C2=A0a =3D atoi(argv[1]); >>>> =C2=A0 =C2=A0if (a < 0) { >>>> =C2=A0 =C2=A0 =C2=A0 =C2=A0puts("1: smaller"); >>>> =C2=A0 =C2=A0} else { >>>> =C2=A0 =C2=A0 =C2=A0 =C2=A0puts("1: not smaller"); >>>> =C2=A0 =C2=A0} >>>> >>>> =C2=A0 =C2=A0if ((int)a < 0) { >>>> =C2=A0 =C2=A0 =C2=A0 =C2=A0puts("2: smaller"); >>>> =C2=A0 =C2=A0} else { >>>> =C2=A0 =C2=A0 =C2=A0 =C2=A0puts("2: not smaller"); >>>> =C2=A0 =C2=A0} >>>> >>>> =C2=A0 =C2=A0return 0; >>>> } >>>> $ gcc -DUNSIGNED enum.c -o enum-unsigned >>>> $ gcc enum.c -o enum-signed >>>> $ ./enum-signed 0 >>>> 1: not smaller >>>> 2: not smaller >>>> $ ./enum-signed -1 >>>> 1: smaller >>>> 2: smaller >>>> $ ./enum-unsigned 0 >>>> 1: not smaller >>>> 2: not smaller >>>> $ ./enum-unsigned -1 >>>> 1: not smaller >>>> 2: smaller >>> >>> Ah, that's a good example, however.. >>> >>>> >>>> This is only how GCC uses enums, other compilers have other rules. So >>>> it's better to avoid any assumptions about signedness of enums. >>>> >>>> In this specific case, because the enum does not have any negative >>>> items, it is unsigned if compiled with GCC. Unsigned items cannot be >>>> negative, thus the check is useless. >>> >>> I agree it's useless, but saying that it is wrong is a bit of a >>> stretch in my opinion. =C2=A0It actually depends on what the intent was= -- >>> if the intent was to be able to use the value as an array index, then >>> I think the check does the job independent of the signedness of the >>> operands. >> >> No. >> >> If BLKDBG_EVENT_MAX is < 0x80000000, it does not matter if there is >> the check or not. Because of unsigned arithmetic, only one comparison >> is enough. With a non-GCC compiler (or if there were negative enum >> items), the check may have the desired effect, just like with int >> cast. >> If BLKDBG_EVENT_MAX is >=3D 0x80000000, the first check is wrong, >> because you want to allow array access beyond 0x80000000, which will >> be blocked by the first check. A non-GCC compiler may actually reject >> an enum bigger than 0x80000000. Then the checks should be rewritten. >> >> The version with int cast is correct in more cases than the version >> which relies on enum signedness. > > If the value isn't explicitly specified, it's defined to start at 0 and > increment by 1 for each enum constant. So it's very unlikely to hit an > interesting case - we would have to add some billions of events first. The discussion about BLKDBG_EVENT_MAX being absurdly high value was theoretical. In reality, even the check for < 0 is useless at the moment, since those values can't be produced. It may be useful to catch internal inconsistencies later if the code changes. > And isn't it the int cast that would lead to (event < 0) =3D=3D true if > BLKDBG_EVENT_MAX was >=3D 0x8000000 and falsely return an error? The old > version should do this right. No, because if you want to allow event >=3D 0x80000000, you shouldn't also check for event < 0 when using 32 bit integers on modern hardware. > Anyway, even though this specific code shouldn't misbehave today, I'm > all for silencing warnings and enabling -Wtype-limits. We regularly have > real bugs of this type. Thank you!