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

[Blue Swirl]

On Sat, Sep 4, 2010 at 5:57 PM, andrzej zaborowski <address@hidden> wrote:
> On 4 September 2010 19:21, Blue Swirl <address@hidden> wrote:
>> On Sat, Sep 4, 2010 at 4:44 PM, andrzej zaborowski <address@hidden> wrote:
>>> On 4 September 2010 18:14, Blue Swirl <address@hidden> wrote:
>>>> On Sat, Sep 4, 2010 at 3:40 PM, andrzej zaborowski <address@hidden> wrote:
>>>>> On 4 September 2010 16:17, Blue Swirl <address@hidden> wrote:
>>>>>> Add various casts, adjust types etc. to make the warnings with
>>>>>> gcc flag -Wtype-limits disappear.
>>>>>>
>>>>>> Signed-off-by: Blue Swirl <address@hidden>
>>>>>> ---
>>>>>>  block/blkdebug.c      |    2 +-
>>>>>>  hw/mips_fulong2e.c    |    2 +-
>>>>>>  hw/omap1.c            |    2 +-
>>>>>>  hw/ppc405_boards.c    |   23 +++++++++++++----------
>>>>>>  hw/ppc_newworld.c     |    3 ++-
>>>>>>  hw/ppc_prep.c         |    3 ++-
>>>>>>  hw/pxa2xx.c           |    2 +-
>>>>>>  hw/sm501.c            |    4 ++--
>>>>>>  linux-user/flatload.c |    3 ++-
>>>>>>  linux-user/mmap.c     |    2 +-
>>>>>>  linux-user/syscall.c  |   20 +++++++++++++-------
>>>>>>  11 files changed, 39 insertions(+), 27 deletions(-)
>>>>>>
>>>>>> diff --git a/block/blkdebug.c b/block/blkdebug.c
>>>>>> index 2a63df9..b5083bc 100644
>>>>>> --- a/block/blkdebug.c
>>>>>> +++ b/block/blkdebug.c
>>>>>> @@ -439,7 +439,7 @@ static void blkdebug_debug_event(BlockDriverState
>>>>>> *bs, BlkDebugEvent event)
>>>>>>     struct BlkdebugRule *rule;
>>>>>>     BlkdebugVars old_vars = s->vars;
>>>>>>
>>>>>> -    if (event < 0 || event >= BLKDBG_EVENT_MAX) {
>>>>>> +    if ((unsigned int)event >= BLKDBG_EVENT_MAX) {
>>>>>>         return;
>>>>>>     }
>>>>>>
>>>>>> diff --git a/hw/mips_fulong2e.c b/hw/mips_fulong2e.c
>>>>>> index cbe7156..ac82067 100644
>>>>>> --- a/hw/mips_fulong2e.c
>>>>>> +++ b/hw/mips_fulong2e.c
>>>>>> @@ -258,7 +258,7 @@ static void mips_fulong2e_init(ram_addr_t
>>>>>> ram_size, const char *boot_device,
>>>>>>  {
>>>>>>     char *filename;
>>>>>>     unsigned long ram_offset, bios_offset;
>>>>>> -    unsigned long bios_size;
>>>>>> +    long bios_size;
>>>>>>     int64_t kernel_entry;
>>>>>>     qemu_irq *i8259;
>>>>>>     qemu_irq *cpu_exit_irq;
>>>>>> diff --git a/hw/omap1.c b/hw/omap1.c
>>>>>> index 06370b6..b04aa80 100644
>>>>>> --- a/hw/omap1.c
>>>>>> +++ b/hw/omap1.c
>>>>>> @@ -3675,7 +3675,7 @@ static int omap_validate_emiff_addr(struct
>>>>>> omap_mpu_state_s *s,
>>>>>>  static int omap_validate_emifs_addr(struct omap_mpu_state_s *s,
>>>>>>                 target_phys_addr_t addr)
>>>>>>  {
>>>>>> -    return addr >= OMAP_EMIFS_BASE && addr < OMAP_EMIFF_BASE;
>>>>>> +    return addr < OMAP_EMIFF_BASE;
>>>>>
>>>>> I think this only doesn't break the code because it relies on a
>>>>> specific value for that #define, and if the value is changed, the
>>>>> check has to be re-added.  Same applies to other modifications in this
>>>>> patch.
>>>>
>>>> I think you mean other modifications like this, that is: this change
>>>> and the pxa2xx.c one. I'll try to invent a better solution.
>>>
>>> Well the other ones are probably just pointless, not wrong.  For
>>> example the first change in this patch really makes you wonder, until
>>> you think of the famous gcc warnings.  There can't be any better
>>> reason for such a change.
>>
>> In the unsigned number space, the checks can be merged into one,
>> assuming that BLKDBG_EVEN_MAX is less than INT_MAX. Alternatively we
>> could have:
>>  -    if (event < 0 || event >= BLKDBG_EVENT_MAX) {
>>  +    if ((int)event < 0 || event >= BLKDBG_EVENT_MAX) {
>>
>> This would also implement the check that the writer of this code was
>> trying to make.
>>
>> The important thing to note is however is that the check as it is now
>> is not correct.
>
> 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

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.

If the enum included also negative values (or compiled with a compiler
using different rules), the check would succeed. Though then the check
against 0 would be wrong because the author probably wanted to check
against the smallest possible value.

In both cases, the cast to int makes sure that the comparison is meaningful.

>>>>> There are some projects that avoid using strcat for example, because,
>>>>> if misused, it can cause crashes.
>>>>
>>>> We also do that, there's pstrcat() and others.
>>>
>>> I don't believe we avoid strcat everywhere.  strcat and pstrcat are
>>> different functions to be used on different occasions.  It'd be the
>>> same level of ridiculous as not using malloc or not using the number 5
>>> (because pstrcat(NULL, 5, "a") causes a crash) with an added
>>> performance penalty.
>>
>> There's no difference in using using strcat vs. pstrcat, or sprintf
>> vs. snprintf. If the caller of strcat doesn't know the buffer size,
>> someone down the call chain must know it, so it can be passed. The
>> major benefit is that buffer overflows will be avoided, at the
>> possible cost of extra parameter passing. Again, the benefit exceeds
>> cost.
>
> Usually you'll allocate the buffer of the size that is needed, so you
> can do for exmple
>
> buffer = qemu_malloc(strlen(this) + strlen(that) + 1);
> and then call strcpy and strcat

But then you can easily add
buflen = strlen(this) + strlen(that) + 1;
and use that for malloc and pstrcat. Cost: one additional variable.

> You know the size of the buffer, but there is no risk of an overflow.
> Yet, some projects prohibit code like this.  Using pstrcpy, pstrcat,
> snprintf unavoidably add the overhead of looping over the input to
> find the length again.

Only if you don't store the length.

> On other ocasions your input buffer and output buffer both have
> constant length and if the input is smaller, then there's no need to
> truncate.  On other occasions you want to return an error if the input
> is too long.  pstrcpy is used when the input should be truncated in
> case it's too long, and no error returned.

I don't see why you wouldn't want to use pstrcpy even for the error
check case given the small additional overhead.

Anyway, this has very little to do with the patch.