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

[andrzej zaborowski]

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.

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

This would work but it's weird.  The thing is it's currently a correct
code and the check may be useless but it's the optimiser's task to
remove it, not ours.  The compiler is not able to tell whether the
check makes sense or nott, because the compiler only has access to
preprocessed code.  So why should you let the compiler have anything
to say on it.

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

That's why it may make sense to enable the warning to check for
errors.  But enabled by default it causes other types of bugs as
people have to work around their compiler to avoid language constructs
that are not kosher in the given compiler version.

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

Cheers