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

[Blue Swirl]

On Mon, Sep 6, 2010 at 1:04 PM, Kevin Wolf <address@hidden> wrote:
> 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.

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) == true if
> BLKDBG_EVENT_MAX was >= 0x8000000 and falsely return an error? The old
> version should do this right.

No, because if you want to allow event >= 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!