Patchwork [1/5] Suppress some gcc warnings with -Wtype-limits

login
register
mail settings
Submitter Blue Swirl
Date Sept. 4, 2010, 2:17 p.m.
Message ID <AANLkTimrAJ7fJzOhJRXLVpRjc8F+CoxWsETHss6tmZG3@mail.gmail.com>
Download mbox | patch
Permalink /patch/63777/
State New
Headers show

Comments

Blue Swirl - Sept. 4, 2010, 2:17 p.m.
Add various casts, adjust types etc. to make the warnings with
gcc flag -Wtype-limits disappear.

Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
---
 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(-)


@@ -1656,8 +1658,9 @@ static abi_long do_accept(int fd, abi_ulong target_addr,
     if (get_user_u32(addrlen, target_addrlen_addr))
         return -TARGET_EINVAL;

-    if (addrlen < 0)
+    if ((int)addrlen < 0) {
         return -TARGET_EINVAL;
+    }

     if (!access_ok(VERIFY_WRITE, target_addr, addrlen))
         return -TARGET_EINVAL;
@@ -1684,8 +1687,9 @@ static abi_long do_getpeername(int fd, abi_ulong
target_addr,
     if (get_user_u32(addrlen, target_addrlen_addr))
         return -TARGET_EFAULT;

-    if (addrlen < 0)
+    if ((int)addrlen < 0) {
         return -TARGET_EINVAL;
+    }

     if (!access_ok(VERIFY_WRITE, target_addr, addrlen))
         return -TARGET_EFAULT;
@@ -1712,8 +1716,9 @@ static abi_long do_getsockname(int fd, abi_ulong
target_addr,
     if (get_user_u32(addrlen, target_addrlen_addr))
         return -TARGET_EFAULT;

-    if (addrlen < 0)
+    if ((int)addrlen < 0) {
         return -TARGET_EINVAL;
+    }

     if (!access_ok(VERIFY_WRITE, target_addr, addrlen))
         return -TARGET_EFAULT;
@@ -1753,8 +1758,9 @@ static abi_long do_sendto(int fd, abi_ulong msg,
size_t len, int flags,
     void *host_msg;
     abi_long ret;

-    if (addrlen < 0)
+    if ((int)addrlen < 0) {
         return -TARGET_EINVAL;
+    }

     host_msg = lock_user(VERIFY_READ, msg, len, 1);
     if (!host_msg)
@@ -1792,7 +1798,7 @@ static abi_long do_recvfrom(int fd, abi_ulong
msg, size_t len, int flags,
             ret = -TARGET_EFAULT;
             goto fail;
         }
-        if (addrlen < 0) {
+        if ((int)addrlen < 0) {
             ret = -TARGET_EINVAL;
             goto fail;
         }
andrzej zaborowski - Sept. 4, 2010, 3:40 p.m.
On 4 September 2010 16:17, Blue Swirl <blauwirbel@gmail.com> wrote:
> Add various casts, adjust types etc. to make the warnings with
> gcc flag -Wtype-limits disappear.
>
> Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
> ---
>  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.

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.

There are some projects that avoid using strcat for example, because,
if misused, it can cause crashes.  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.

Cheers
Blue Swirl - Sept. 4, 2010, 4:14 p.m.
On Sat, Sep 4, 2010 at 3:40 PM, andrzej zaborowski <balrogg@gmail.com> wrote:
> On 4 September 2010 16:17, Blue Swirl <blauwirbel@gmail.com> wrote:
>> Add various casts, adjust types etc. to make the warnings with
>> gcc flag -Wtype-limits disappear.
>>
>> Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
>> ---
>>  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.
andrzej zaborowski - Sept. 4, 2010, 4:44 p.m.
On 4 September 2010 18:14, Blue Swirl <blauwirbel@gmail.com> wrote:
> On Sat, Sep 4, 2010 at 3:40 PM, andrzej zaborowski <balrogg@gmail.com> wrote:
>> On 4 September 2010 16:17, Blue Swirl <blauwirbel@gmail.com> wrote:
>>> Add various casts, adjust types etc. to make the warnings with
>>> gcc flag -Wtype-limits disappear.
>>>
>>> Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
>>> ---
>>>  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
Blue Swirl - Sept. 4, 2010, 5:21 p.m.
On Sat, Sep 4, 2010 at 4:44 PM, andrzej zaborowski <balrogg@gmail.com> wrote:
> On 4 September 2010 18:14, Blue Swirl <blauwirbel@gmail.com> wrote:
>> On Sat, Sep 4, 2010 at 3:40 PM, andrzej zaborowski <balrogg@gmail.com> wrote:
>>> On 4 September 2010 16:17, Blue Swirl <blauwirbel@gmail.com> wrote:
>>>> Add various casts, adjust types etc. to make the warnings with
>>>> gcc flag -Wtype-limits disappear.
>>>>
>>>> Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
>>>> ---
>>>>  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.
andrzej zaborowski - Sept. 4, 2010, 5:57 p.m.
On 4 September 2010 19:21, Blue Swirl <blauwirbel@gmail.com> wrote:
> On Sat, Sep 4, 2010 at 4:44 PM, andrzej zaborowski <balrogg@gmail.com> wrote:
>> On 4 September 2010 18:14, Blue Swirl <blauwirbel@gmail.com> wrote:
>>> On Sat, Sep 4, 2010 at 3:40 PM, andrzej zaborowski <balrogg@gmail.com> wrote:
>>>> On 4 September 2010 16:17, Blue Swirl <blauwirbel@gmail.com> wrote:
>>>>> Add various casts, adjust types etc. to make the warnings with
>>>>> gcc flag -Wtype-limits disappear.
>>>>>
>>>>> Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
>>>>> ---
>>>>>  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
Blue Swirl - Sept. 4, 2010, 7:45 p.m.
On Sat, Sep 4, 2010 at 5:57 PM, andrzej zaborowski <balrogg@gmail.com> wrote:
> On 4 September 2010 19:21, Blue Swirl <blauwirbel@gmail.com> wrote:
>> On Sat, Sep 4, 2010 at 4:44 PM, andrzej zaborowski <balrogg@gmail.com> wrote:
>>> On 4 September 2010 18:14, Blue Swirl <blauwirbel@gmail.com> wrote:
>>>> On Sat, Sep 4, 2010 at 3:40 PM, andrzej zaborowski <balrogg@gmail.com> wrote:
>>>>> On 4 September 2010 16:17, Blue Swirl <blauwirbel@gmail.com> wrote:
>>>>>> Add various casts, adjust types etc. to make the warnings with
>>>>>> gcc flag -Wtype-limits disappear.
>>>>>>
>>>>>> Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
>>>>>> ---
>>>>>>  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.
andrzej zaborowski - Sept. 4, 2010, 8:30 p.m.
Hi,

On 4 September 2010 21:45, Blue Swirl <blauwirbel@gmail.com> wrote:
> On Sat, Sep 4, 2010 at 5:57 PM, andrzej zaborowski <balrogg@gmail.com> wrote:
>>>  -    if (event < 0 || event >= BLKDBG_EVENT_MAX) {
>>>  +    if ((int)event < 0 || event >= BLKDBG_EVENT_MAX) {
>> 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

Ah, that's a good example, however..

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

I agree it's useless, but saying that it is wrong is a bit of a
stretch in my opinion.  It actually depends on what the intent was --
if the intent was to be able to use the value as an array index, then
I think the check does the job independent of the signedness of the
operands.

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

Plus the cost of the strlen inside pstrcat.  pstrcat has to either
check the length of source string against the buffer size first, or do
it at the same time it copies the string from source to destination
character by character, but either way the penalty is of linear cost.
If it was an inline function, then perhaps the compiler could optimise
the second strlen away in some situations, specially since strcat,
strlen etc are now builtins.

So the reason I dislike using it blindly is that often you already
know that a buffer overflow is out of question, and secondly if
misused, it can hide a real bug where the string gets unintentionally
truncated and as a result something worse than an overflow happens
(e.g. a file on disk is overwritten) without noticing whereas a
segfault would be noticed.

Cheers
Blue Swirl - Sept. 4, 2010, 9:07 p.m.
On Sat, Sep 4, 2010 at 8:30 PM, andrzej zaborowski <balrogg@gmail.com> wrote:
> Hi,
>
> On 4 September 2010 21:45, Blue Swirl <blauwirbel@gmail.com> wrote:
>> On Sat, Sep 4, 2010 at 5:57 PM, andrzej zaborowski <balrogg@gmail.com> wrote:
>>>>  -    if (event < 0 || event >= BLKDBG_EVENT_MAX) {
>>>>  +    if ((int)event < 0 || event >= BLKDBG_EVENT_MAX) {
>>> 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
>
> Ah, that's a good example, however..
>
>>
>> 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.
>
> I agree it's useless, but saying that it is wrong is a bit of a
> stretch in my opinion.  It actually depends on what the intent was --
> if the intent was to be able to use the value as an array index, then
> I think the check does the job independent of the signedness of the
> operands.

No.

If BLKDBG_EVENT_MAX is < 0x80000000, it does not matter if there is
the check or not. Because of unsigned arithmetic, only one comparison
is enough. With a non-GCC compiler (or if there were negative enum
items), the check may have the desired effect, just like with int
cast.

If BLKDBG_EVENT_MAX is >= 0x80000000, the first check is wrong,
because you want to allow array access beyond 0x80000000, which will
be blocked by the first check. A non-GCC compiler may actually reject
an enum bigger than 0x80000000. Then the checks should be rewritten.

The version with int cast is correct in more cases than the version
which relies on enum signedness.

>
>>
>> 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.
>
> Plus the cost of the strlen inside pstrcat.  pstrcat has to either
> check the length of source string against the buffer size first, or do
> it at the same time it copies the string from source to destination
> character by character, but either way the penalty is of linear cost.
> If it was an inline function, then perhaps the compiler could optimise
> the second strlen away in some situations, specially since strcat,
> strlen etc are now builtins.

void pstrcpy(char *buf, int buf_size, const char *str)
{
    int c;
    char *q = buf;

    if (buf_size <= 0)
        return;

    for(;;) {
        c = *str++;
        if (c == 0 || q >= buf + buf_size - 1)
            break;
        *q++ = c;
    }
    *q = '\0';
}

There is one extra check, q >= buf + buf_size - 1. No strlen().

0000000000000000 <pstrcpy>:
   0:   48 83 ec 18             sub    $0x18,%rsp
   4:   64 48 8b 04 25 28 00    mov    %fs:0x28,%rax
   b:   00 00
   d:   48 89 44 24 10          mov    %rax,0x10(%rsp)
  12:   31 c0                   xor    %eax,%eax
  14:   85 f6                   test   %esi,%esi
  16:   7e 39                   jle    51 <pstrcpy+0x51>
  18:   0f b6 0a                movzbl (%rdx),%ecx
  1b:   84 c9                   test   %cl,%cl
  1d:   74 2f                   je     4e <pstrcpy+0x4e>
  1f:   48 63 c6                movslq %esi,%rax
  22:   48 8d 44 07 ff          lea    -0x1(%rdi,%rax,1),%rax
  27:   48 39 c7                cmp    %rax,%rdi
  2a:   73 22                   jae    4e <pstrcpy+0x4e>
  2c:   48 83 c2 01             add    $0x1,%rdx
  30:   eb 0b                   jmp    3d <pstrcpy+0x3d>
  32:   66 0f 1f 44 00 00       nopw   0x0(%rax,%rax,1)

The copying loop follows until 4c:
  38:   48 39 c7                cmp    %rax,%rdi
  3b:   73 11                   jae    4e <pstrcpy+0x4e>
  3d:   88 0f                   mov    %cl,(%rdi)
  3f:   0f b6 0a                movzbl (%rdx),%ecx
  42:   48 83 c7 01             add    $0x1,%rdi
  46:   48 83 c2 01             add    $0x1,%rdx
  4a:   84 c9                   test   %cl,%cl
  4c:   75 ea                   jne    38 <pstrcpy+0x38>

  4e:   c6 07 00                movb   $0x0,(%rdi)
  51:   48 8b 44 24 10          mov    0x10(%rsp),%rax
  56:   64 48 33 04 25 28 00    xor    %fs:0x28,%rax
  5d:   00 00
  5f:   75 05                   jne    66 <pstrcpy+0x66>
  61:   48 83 c4 18             add    $0x18,%rsp
  65:   c3                      retq
  66:   66 90                   xchg   %ax,%ax
  68:   e8 00 00 00 00          callq  6d <pstrcpy+0x6d>
  6d:   0f 1f 00                nopl   (%rax)

The extra check in the loop adds the instructions at 38 and 3b. Those
are not memory accesses, so the cost should be marginal.

> So the reason I dislike using it blindly is that often you already
> know that a buffer overflow is out of question, and secondly if
> misused, it can hide a real bug where the string gets unintentionally
> truncated and as a result something worse than an overflow happens
> (e.g. a file on disk is overwritten) without noticing whereas a
> segfault would be noticed.

The 'segfault' can be much, much worse:
http://cve.mitre.org/cgi-bin/cvekey.cgi?keyword=buffer+overflow

It's true that truncation can also cause bugs without proper checks,
but still there shouldn't be a direct route to 'allow remote attackers
to execute arbitrary code'.
malc - Sept. 4, 2010, 9:26 p.m.
On Sat, 4 Sep 2010, Blue Swirl wrote:

> On Sat, Sep 4, 2010 at 5:57 PM, andrzej zaborowski <balrogg@gmail.com> wrote:
> > On 4 September 2010 19:21, Blue Swirl <blauwirbel@gmail.com> wrote:
> >> On Sat, Sep 4, 2010 at 4:44 PM, andrzej zaborowski <balrogg@gmail.com> wrote:
> >>> On 4 September 2010 18:14, Blue Swirl <blauwirbel@gmail.com> wrote:
> >>>> On Sat, Sep 4, 2010 at 3:40 PM, andrzej zaborowski <balrogg@gmail.com> wrote:
> >>>>> On 4 September 2010 16:17, Blue Swirl <blauwirbel@gmail.com> wrote:
> >>>>>> Add various casts, adjust types etc. to make the warnings with
> >>>>>> gcc flag -Wtype-limits disappear.

[..snip..]

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

While enumerated types are indeed implementation defined (6.7.2.2),
enumeration constants themselves are of type int (6.4.4.3), so all one
has to do is not to declare variables with enumeration type but use
plain int instead (Which i guess goes against the desire to be gentle
with the reader of the code)

[..snip..]
Michael S. Tsirkin - Sept. 5, 2010, 7:54 a.m.
On Sat, Sep 04, 2010 at 05:21:24PM +0000, Blue Swirl wrote:
> 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.

I agree. But it seems to indicate a bigger problem.

If we are trying to pass in a negative value, which is not one
of enum values, using BlkDebugEvent as type is just confusing,
we should just pass int instead.


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

Use #pragma to supress the warning? Maybe we could wrap this in a macro ..
Blue Swirl - Sept. 5, 2010, 9:06 a.m.
On Sun, Sep 5, 2010 at 7:54 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Sat, Sep 04, 2010 at 05:21:24PM +0000, Blue Swirl wrote:
>> 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.
>
> I agree. But it seems to indicate a bigger problem.
>
> If we are trying to pass in a negative value, which is not one
> of enum values, using BlkDebugEvent as type is just confusing,
> we should just pass int instead.

AFAICT it's only possible to use the values listed in event_names in
blkdebug.c, other values are rejected. So the check should actually be
an assert() or it could even be removed.

>> >> 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.
>
> Use #pragma to supress the warning? Maybe we could wrap this in a macro ..

Those lines may also desynch silently with changes to OMAP_EMIFS_BASE.

I think the assertion is still the best way, it ensures that something
will happen if OMAP_EMIFS_BASE changes. We could for example remove
OMAP_EMIFS_BASE entirely (it's only used for the check), but someone
adding a new define could still forget to adjust the check anyway.
Michael S. Tsirkin - Sept. 5, 2010, 9:26 a.m.
On Sun, Sep 05, 2010 at 09:06:10AM +0000, Blue Swirl wrote:
> On Sun, Sep 5, 2010 at 7:54 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Sat, Sep 04, 2010 at 05:21:24PM +0000, Blue Swirl wrote:
> >> 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.
> >
> > I agree. But it seems to indicate a bigger problem.
> >
> > If we are trying to pass in a negative value, which is not one
> > of enum values, using BlkDebugEvent as type is just confusing,
> > we should just pass int instead.
> 
> AFAICT it's only possible to use the values listed in event_names in
> blkdebug.c, other values are rejected. So the check should actually be
> an assert() or it could even be removed.

Sounds good.

> >> >> 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.
> >
> > Use #pragma to supress the warning? Maybe we could wrap this in a macro ..
> 
> Those lines may also desynch silently with changes to OMAP_EMIFS_BASE.
> 
> I think the assertion is still the best way, it ensures that something
> will happen if OMAP_EMIFS_BASE changes. We could for example remove
> OMAP_EMIFS_BASE entirely (it's only used for the check), but someone
> adding a new define could still forget to adjust the check anyway.

We could replace it with a macro
#define OMAP_EMIFS_VALID(addr) ((target_phys_addr_t)addr < OMAP_EMIFF_BASE) 
but all this does look artificial. And of course using type casts
is always scary ...

Would it help to have some inline functions that do the range checking correctly?
We have a couple of range helpers in pci.h, these could be moved out
to range.h and we could add some more. As there act on u64 this will get
the type limits mostly automatically right.
Kevin Wolf - Sept. 6, 2010, 1:04 p.m.
Am 04.09.2010 23:07, schrieb Blue Swirl:
> On Sat, Sep 4, 2010 at 8:30 PM, andrzej zaborowski <balrogg@gmail.com> wrote:
>> Hi,
>>
>> On 4 September 2010 21:45, Blue Swirl <blauwirbel@gmail.com> wrote:
>>> On Sat, Sep 4, 2010 at 5:57 PM, andrzej zaborowski <balrogg@gmail.com> wrote:
>>>>>  -    if (event < 0 || event >= BLKDBG_EVENT_MAX) {
>>>>>  +    if ((int)event < 0 || event >= BLKDBG_EVENT_MAX) {
>>>> 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
>>
>> Ah, that's a good example, however..
>>
>>>
>>> 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.
>>
>> I agree it's useless, but saying that it is wrong is a bit of a
>> stretch in my opinion.  It actually depends on what the intent was --
>> if the intent was to be able to use the value as an array index, then
>> I think the check does the job independent of the signedness of the
>> operands.
> 
> No.
> 
> If BLKDBG_EVENT_MAX is < 0x80000000, it does not matter if there is
> the check or not. Because of unsigned arithmetic, only one comparison
> is enough. With a non-GCC compiler (or if there were negative enum
> items), the check may have the desired effect, just like with int
> cast.
> If BLKDBG_EVENT_MAX is >= 0x80000000, the first check is wrong,
> because you want to allow array access beyond 0x80000000, which will
> be blocked by the first check. A non-GCC compiler may actually reject
> an enum bigger than 0x80000000. Then the checks should be rewritten.
> 
> The version with int cast is correct in more cases than the version
> which relies on enum signedness.

If the value isn't explicitly specified, it's defined to start at 0 and
increment by 1 for each enum constant. So it's very unlikely to hit an
interesting case - we would have to add some billions of events first.

And isn't it the int cast that would lead to (event < 0) == true if
BLKDBG_EVENT_MAX was >= 0x8000000 and falsely return an error? The old
version should do this right.

Anyway, even though this specific code shouldn't misbehave today, I'm
all for silencing warnings and enabling -Wtype-limits. We regularly have
real bugs of this type.

Kevin
Blue Swirl - Sept. 6, 2010, 7:21 p.m.
On Mon, Sep 6, 2010 at 1:04 PM, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 04.09.2010 23:07, schrieb Blue Swirl:
>> On Sat, Sep 4, 2010 at 8:30 PM, andrzej zaborowski <balrogg@gmail.com> wrote:
>>> Hi,
>>>
>>> On 4 September 2010 21:45, Blue Swirl <blauwirbel@gmail.com> wrote:
>>>> On Sat, Sep 4, 2010 at 5:57 PM, andrzej zaborowski <balrogg@gmail.com> wrote:
>>>>>>  -    if (event < 0 || event >= BLKDBG_EVENT_MAX) {
>>>>>>  +    if ((int)event < 0 || event >= BLKDBG_EVENT_MAX) {
>>>>> 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
>>>
>>> Ah, that's a good example, however..
>>>
>>>>
>>>> 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.
>>>
>>> I agree it's useless, but saying that it is wrong is a bit of a
>>> stretch in my opinion.  It actually depends on what the intent was --
>>> if the intent was to be able to use the value as an array index, then
>>> I think the check does the job independent of the signedness of the
>>> operands.
>>
>> No.
>>
>> If BLKDBG_EVENT_MAX is < 0x80000000, it does not matter if there is
>> the check or not. Because of unsigned arithmetic, only one comparison
>> is enough. With a non-GCC compiler (or if there were negative enum
>> items), the check may have the desired effect, just like with int
>> cast.
>> If BLKDBG_EVENT_MAX is >= 0x80000000, the first check is wrong,
>> because you want to allow array access beyond 0x80000000, which will
>> be blocked by the first check. A non-GCC compiler may actually reject
>> an enum bigger than 0x80000000. Then the checks should be rewritten.
>>
>> The version with int cast is correct in more cases than the version
>> which relies on enum signedness.
>
> If the value isn't explicitly specified, it's defined to start at 0 and
> increment by 1 for each enum constant. So it's very unlikely to hit an
> interesting case - we would have to add some billions of events first.

The discussion about BLKDBG_EVENT_MAX being absurdly high value was
theoretical. In reality, even the check for < 0 is useless at the
moment, since those values can't be produced. It may be useful to
catch internal inconsistencies later if the code changes.

> And isn't it the int cast that would lead to (event < 0) == true if
> BLKDBG_EVENT_MAX was >= 0x8000000 and falsely return an error? The old
> version should do this right.

No, because if you want to allow event >= 0x80000000, you shouldn't
also check for event < 0 when using 32 bit integers on modern
hardware.

> Anyway, even though this specific code shouldn't misbehave today, I'm
> all for silencing warnings and enabling -Wtype-limits. We regularly have
> real bugs of this type.

Thank you!

Patch

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

 static int omap_validate_imif_addr(struct omap_mpu_state_s *s,
diff --git a/hw/ppc405_boards.c b/hw/ppc405_boards.c
index 662d7c4..db8e5ec 100644
--- a/hw/ppc405_boards.c
+++ b/hw/ppc405_boards.c
@@ -182,10 +182,12 @@  static void ref405ep_init (ram_addr_t ram_size,
     qemu_irq *pic;
     ram_addr_t sram_offset, bios_offset, bdloc;
     target_phys_addr_t ram_bases[2], ram_sizes[2];
-    target_ulong sram_size, bios_size;
+    target_ulong sram_size;
+    long bios_size;
     //int phy_addr = 0;
     //static int phy_addr = 1;
-    target_ulong kernel_base, kernel_size, initrd_base, initrd_size;
+    target_ulong kernel_base, initrd_base;
+    long kernel_size, initrd_size;
     int linux_boot;
     int fl_idx, fl_sectors, len;
     DriveInfo *dinfo;
@@ -221,8 +223,8 @@  static void ref405ep_init (ram_addr_t ram_size,
         bios_offset = qemu_ram_alloc(NULL, "ef405ep.bios", bios_size);
         fl_sectors = (bios_size + 65535) >> 16;
 #ifdef DEBUG_BOARD_INIT
-        printf("Register parallel flash %d size " TARGET_FMT_lx
-               " at offset %08lx addr " TARGET_FMT_lx " '%s' %d\n",
+        printf("Register parallel flash %d size %lx"
+               " at offset %08lx addr %lx '%s' %d\n",
                fl_idx, bios_size, bios_offset, -bios_size,
                bdrv_get_device_name(dinfo->bdrv), fl_sectors);
 #endif
@@ -308,7 +310,7 @@  static void ref405ep_init (ram_addr_t ram_size,
                     kernel_filename);
             exit(1);
         }
-        printf("Load kernel size " TARGET_FMT_ld " at " TARGET_FMT_lx,
+        printf("Load kernel size %ld at " TARGET_FMT_lx,
                kernel_size, kernel_base);
         /* load initrd */
         if (initrd_filename) {
@@ -503,8 +505,9 @@  static void taihu_405ep_init(ram_addr_t ram_size,
     qemu_irq *pic;
     ram_addr_t bios_offset;
     target_phys_addr_t ram_bases[2], ram_sizes[2];
-    target_ulong bios_size;
-    target_ulong kernel_base, kernel_size, initrd_base, initrd_size;
+    long bios_size;
+    target_ulong kernel_base, initrd_base;
+    long kernel_size, initrd_size;
     int linux_boot;
     int fl_idx, fl_sectors;
     DriveInfo *dinfo;
@@ -534,8 +537,8 @@  static void taihu_405ep_init(ram_addr_t ram_size,
         fl_sectors = (bios_size + 65535) >> 16;
         bios_offset = qemu_ram_alloc(NULL, "taihu_405ep.bios", bios_size);
 #ifdef DEBUG_BOARD_INIT
-        printf("Register parallel flash %d size " TARGET_FMT_lx
-               " at offset %08lx addr " TARGET_FMT_lx " '%s' %d\n",
+        printf("Register parallel flash %d size %lx"
+               " at offset %08lx addr %lx '%s' %d\n",
                fl_idx, bios_size, bios_offset, -bios_size,
                bdrv_get_device_name(dinfo->bdrv), fl_sectors);
 #endif
@@ -576,7 +579,7 @@  static void taihu_405ep_init(ram_addr_t ram_size,
         bios_size = 32 * 1024 * 1024;
         fl_sectors = (bios_size + 65535) >> 16;
 #ifdef DEBUG_BOARD_INIT
-        printf("Register parallel flash %d size " TARGET_FMT_lx
+        printf("Register parallel flash %d size %lx"
                " at offset %08lx  addr " TARGET_FMT_lx " '%s'\n",
                fl_idx, bios_size, bios_offset, (target_ulong)0xfc000000,
                bdrv_get_device_name(dinfo->bdrv));
diff --git a/hw/ppc_newworld.c b/hw/ppc_newworld.c
index 809a1cf..fb07c83 100644
--- a/hw/ppc_newworld.c
+++ b/hw/ppc_newworld.c
@@ -135,7 +135,8 @@  static void ppc_core99_init (ram_addr_t ram_size,
     int unin_memory;
     int linux_boot, i;
     ram_addr_t ram_offset, bios_offset, vga_bios_offset;
-    uint32_t kernel_base, kernel_size, initrd_base, initrd_size;
+    uint32_t kernel_base, initrd_base;
+    long kernel_size, initrd_size;
     PCIBus *pci_bus;
     MacIONVRAMState *nvr;
     int nvram_mem_index;
diff --git a/hw/ppc_prep.c b/hw/ppc_prep.c
index 52fa9b6..0e5b88c 100644
--- a/hw/ppc_prep.c
+++ b/hw/ppc_prep.c
@@ -572,7 +572,8 @@  static void ppc_prep_init (ram_addr_t ram_size,
     int PPC_io_memory;
     int linux_boot, i, nb_nics1, bios_size;
     ram_addr_t ram_offset, bios_offset;
-    uint32_t kernel_base, kernel_size, initrd_base, initrd_size;
+    uint32_t kernel_base, initrd_base;
+    long kernel_size, initrd_size;
     PCIBus *pci_bus;
     qemu_irq *i8259;
     qemu_irq *cpu_exit_irq;
diff --git a/hw/pxa2xx.c b/hw/pxa2xx.c
index 26b9205..3c06bf9 100644
--- a/hw/pxa2xx.c
+++ b/hw/pxa2xx.c
@@ -125,7 +125,7 @@  static void pxa2xx_pm_write(void *opaque,
target_phys_addr_t addr,
         break;

     default:	/* Read-write registers */
-        if (addr >= PMCR && addr <= PCMD31 && !(addr & 3)) {
+        if (addr <= PCMD31 && !(addr & 3)) {
             s->pm_regs[addr >> 2] = value;
             break;
         }
diff --git a/hw/sm501.c b/hw/sm501.c
index 8e6932d..1eb6b63 100644
--- a/hw/sm501.c
+++ b/hw/sm501.c
@@ -814,7 +814,7 @@  static uint32_t sm501_palette_read(void *opaque,
target_phys_addr_t addr)
     /* TODO : consider BYTE/WORD access */
     /* TODO : consider endian */

-    assert(0 <= addr && addr < 0x400 * 3);
+    assert(addr < 0x400 * 3);
     return *(uint32_t*)&s->dc_palette[addr];
 }

@@ -828,7 +828,7 @@  static void sm501_palette_write(void *opaque,
     /* TODO : consider BYTE/WORD access */
     /* TODO : consider endian */

-    assert(0 <= addr && addr < 0x400 * 3);
+    assert(addr < 0x400 * 3);
     *(uint32_t*)&s->dc_palette[addr] = value;
 }

diff --git a/linux-user/flatload.c b/linux-user/flatload.c
index 8ad130a..8f9f4a5 100644
--- a/linux-user/flatload.c
+++ b/linux-user/flatload.c
@@ -383,7 +383,8 @@  static int load_flat_file(struct linux_binprm * bprm,
 		struct lib_info *libinfo, int id, abi_ulong *extra_stack)
 {
     struct flat_hdr * hdr;
-    abi_ulong textpos = 0, datapos = 0, result;
+    abi_ulong textpos = 0, datapos = 0;
+    abi_long result;
     abi_ulong realdatastart = 0;
     abi_ulong text_len, data_len, bss_len, stack_len, flags;
     abi_ulong memp = 0; /* for finding the brk area */
diff --git a/linux-user/mmap.c b/linux-user/mmap.c
index e10a6ef..67d020d 100644
--- a/linux-user/mmap.c
+++ b/linux-user/mmap.c
@@ -342,7 +342,7 @@  abi_ulong mmap_find_vma(abi_ulong start, abi_ulong size)
         munmap(ptr, size);

         /* ENOMEM if we checked the whole of the target address space.  */
-        if (addr == -1ul) {
+        if (addr == (abi_ulong)-1ul) {
             return (abi_ulong)-1;
         } else if (addr == 0) {
             if (wrapped) {
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 0ebe7e1..d44f512 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -1551,8 +1551,9 @@  static abi_long do_bind(int sockfd, abi_ulong target_addr,
     void *addr;
     abi_long ret;

-    if (addrlen < 0)
+    if ((int)addrlen < 0) {
         return -TARGET_EINVAL;
+    }

     addr = alloca(addrlen+1);

@@ -1570,8 +1571,9 @@  static abi_long do_connect(int sockfd, abi_ulong
target_addr,
     void *addr;
     abi_long ret;

-    if (addrlen < 0)
+    if ((int)addrlen < 0) {
         return -TARGET_EINVAL;
+    }

     addr = alloca(addrlen);