From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=46098 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OsbN5-0004KZ-Se for qemu-devel@nongnu.org; Mon, 06 Sep 2010 09:04:32 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OsbN1-0007p7-60 for qemu-devel@nongnu.org; Mon, 06 Sep 2010 09:04:27 -0400 Received: from mx1.redhat.com ([209.132.183.28]:32884) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OsbN0-0007p1-Tm for qemu-devel@nongnu.org; Mon, 06 Sep 2010 09:04:23 -0400 Message-ID: <4C84E6E4.6030909@redhat.com> Date: Mon, 06 Sep 2010 15:04:36 +0200 From: Kevin Wolf MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH 1/5] Suppress some gcc warnings with -Wtype-limits References: In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Blue Swirl Cc: qemu-devel Am 04.09.2010 23:07, schrieb Blue Swirl: > On Sat, Sep 4, 2010 at 8:30 PM, andrzej zaborowski wrote: >> Hi, >> >> On 4 September 2010 21:45, Blue Swirl wrote: >>> On Sat, Sep 4, 2010 at 5:57 PM, andrzej zaborowski wrote: >>>>> - if (event < 0 || event >= BLKDBG_EVENT_MAX) { >>>>> + if ((int)event < 0 || event >= BLKDBG_EVENT_MAX) { >>>> 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) >>> { >>> enum { >>> #ifdef UNSIGNED >>> A = 0, >>> #else >>> A = -1, >>> #endif >>> } a; >>> >>> a = atoi(argv[1]); >>> if (a < 0) { >>> puts("1: smaller"); >>> } else { >>> puts("1: not smaller"); >>> } >>> >>> if ((int)a < 0) { >>> puts("2: smaller"); >>> } else { >>> puts("2: not smaller"); >>> } >>> >>> return 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. It 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 >= 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. And isn't it the int cast that would lead to (event < 0) == true if BLKDBG_EVENT_MAX was >= 0x8000000 and falsely return an error? The old version should do this right. 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. Kevin