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

[Blue Swirl]

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.

How about adding assert(OMAP_EMIFS_BASE == 0) and commenting out the
check? Then if the value changes, the need to add the comparison back
will be obvious.

> Most of the gcc warnings should not be enabled by default and I think
> this is one of them.  They make you do some really strange things in
> the code just to avoid using one construct of your programming
> language (fully valid in the language), and in the end result in bugs
> of other types.  It's probably ok to enable them once to see if they
> hint on any bugs, but if enabled by default, they'll just cause other
> types of bugs.

I don't see how comparison of an unsigned value for >= 0 or < 0 can be
useful. I found two questionable cases or bugs. Also in the
bios_size/kernel_size cases, failures (indicated by return value < 0)
were ignored. Most of other cases are just matter of mixing incorrect
types so they can be fixed easily.

> 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.

>  There's even a pretty big project
> by Nokia that does not use malloc(), because again, "malloc can cause
> memory leaks".  They just allocate all memory statically, and it
> works.. but is a pain to work with, and the program requires much more
> memory than needed so it doesn't run on some embedded devices.  I
> wouldn't want to go this way.

I agree. We have other reasons to be careful with QEMU memory
management, but this is not one of them.