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

[andrzej zaborowski]

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.

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

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.

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.

Cheers