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

[Blue Swirl]

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.

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

Good point. I'll try to invent something better.

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

This extra effort should be balanced against the benefits. I still
think the benefits in the bug detection outweigh the efforts to work
around the (somewhat artificial, I agree) problems.

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