Re: [Qemu-devel] [PATCH 1/5] Suppress some gcc warnings with -Wtype-limits

[Kevin Wolf]

Am 04.09.2010 23:07, schrieb Blue Swirl:
> On Sat, Sep 4, 2010 at 8:30 PM, andrzej zaborowski <address@hidden> wrote:
>> Hi,
>>
>> On 4 September 2010 21:45, Blue Swirl <address@hidden> wrote:
>>> On Sat, Sep 4, 2010 at 5:57 PM, andrzej zaborowski <address@hidden> 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 <stdio.h>
>>>
>>> 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