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

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

[Blue Swirl]

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

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

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

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

[andrzej zaborowski]

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

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

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

[andrzej zaborowski]

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

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

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

[andrzej zaborowski]

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

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

[Blue Swirl]

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.

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

[andrzej zaborowski]

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

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

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

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

[malc]

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

-- 
mailto:av1474@comtv.ru

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

[Michael S. Tsirkin]

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

-- 
MST

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

[Blue Swirl]

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.

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

[Michael S. Tsirkin]

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.

-- 
MST

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

[Blue Swirl]

On Sun, Sep 5, 2010 at 9:26 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> 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.

That seems to be the best solution, I get no warnings with this:

diff --git a/hw/omap1.c b/hw/omap1.c
index b00f870..8bf88e7 100644
--- a/hw/omap1.c
+++ b/hw/omap1.c
@@ -3672,14 +3672,25 @@ static int omap_validate_emiff_addr(struct
omap_mpu_state_s *s,
     return addr >= OMAP_EMIFF_BASE && addr < OMAP_EMIFF_BASE + s->sdram_size;
 }

+/* Get last byte of a range from offset + length.
+ * Undefined for ranges that wrap around 0. */
+static inline uint64_t range_get_last(uint64_t offset, uint64_t len)
+{
+    return offset + len - 1;
+}
+
+/* Check whether a given range covers a given byte. */
+static inline int range_covers_byte(uint64_t offset, uint64_t len,
+                                    uint64_t byte)
+{
+    return offset <= byte && byte <= range_get_last(offset, len);
+}
+
 static int omap_validate_emifs_addr(struct omap_mpu_state_s *s,
                 target_phys_addr_t addr)
 {
-    /* If OMAP_EMIFS_BASE ever becomes nonzero, adjust the check below
-       to also include the lower bound check like
-       addr >= OMAP_EMIFS_BASE && addr < OMAP_EMIFF_BASE */
-    assert(OMAP_EMIFS_BASE == 0);
-    return addr < OMAP_EMIFF_BASE;
+    return range_covers_byte(OMAP_EMIFS_BASE,
+                             OMAP_EMIFF_BASE - OMAP_EMIFS_BASE, addr);
 }

 static int omap_validate_imif_addr(struct omap_mpu_state_s *s,

I'll add range.h and respin the patches.

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

[Michael S. Tsirkin]

On Sun, Sep 05, 2010 at 09:44:01AM +0000, Blue Swirl wrote:
> On Sun, Sep 5, 2010 at 9:26 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > 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.
> 
> That seems to be the best solution, I get no warnings with this:
> 
> diff --git a/hw/omap1.c b/hw/omap1.c
> index b00f870..8bf88e7 100644
> --- a/hw/omap1.c
> +++ b/hw/omap1.c
> @@ -3672,14 +3672,25 @@ static int omap_validate_emiff_addr(struct
> omap_mpu_state_s *s,
>      return addr >= OMAP_EMIFF_BASE && addr < OMAP_EMIFF_BASE + s->sdram_size;
>  }
> 
> +/* Get last byte of a range from offset + length.
> + * Undefined for ranges that wrap around 0. */
> +static inline uint64_t range_get_last(uint64_t offset, uint64_t len)
> +{
> +    return offset + len - 1;
> +}
> +
> +/* Check whether a given range covers a given byte. */
> +static inline int range_covers_byte(uint64_t offset, uint64_t len,
> +                                    uint64_t byte)
> +{
> +    return offset <= byte && byte <= range_get_last(offset, len);
> +}
> +
>  static int omap_validate_emifs_addr(struct omap_mpu_state_s *s,
>                  target_phys_addr_t addr)
>  {
> -    /* If OMAP_EMIFS_BASE ever becomes nonzero, adjust the check below
> -       to also include the lower bound check like
> -       addr >= OMAP_EMIFS_BASE && addr < OMAP_EMIFF_BASE */
> -    assert(OMAP_EMIFS_BASE == 0);
> -    return addr < OMAP_EMIFF_BASE;
> +    return range_covers_byte(OMAP_EMIFS_BASE,
> +                             OMAP_EMIFF_BASE - OMAP_EMIFS_BASE, addr);
>  }
> 
>  static int omap_validate_imif_addr(struct omap_mpu_state_s *s,
> 
> I'll add range.h and respin the patches.

BTW, maybe we want a variant of range_covers_byte that gets
first and after last byte values, but so far we could not
come up with good name for that function, and 1 after last
semantics might be confusing.

One small comment: these are currenly wrong if the range
wraps around to 0 - and there's a comment that says so there.
This was never a problem for pci, but it might be
if we are making them generic.

-- 
MST

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

[andrzej zaborowski]

On 5 September 2010 11:44, Blue Swirl <blauwirbel@gmail.com> wrote:
> On Sun, Sep 5, 2010 at 9:26 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> 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, assuming that an enum can reach 0x80000000 different values,
perhaps the current code is not ideal.  Still I think calling it
"wrong" is wrong, and calling your patch a "fix" is wrong. (Same as
calling patches that remove a warning a "fix", they are workarounds)

>>> >
>>> > 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.
>
> That seems to be the best solution, I get no warnings with this:

While the resulting code is clean (just as the current code), I think
it really shows that this warning should not be enabled.  At this
point you find yourself working around your compiler and potentially
forcing other write some really strange code to work around the
problem caused by this.

There are many warnings that should not be enabled by default for this
reason (like the uninitialised variable warning) unless they are fixed
to be really intelligent (which is unlikely in this case).

Cheers

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

[Blue Swirl]

On Sun, Sep 5, 2010 at 3:26 PM, andrzej zaborowski <balrogg@gmail.com> wrote:
> On 5 September 2010 11:44, Blue Swirl <blauwirbel@gmail.com> wrote:
>> On Sun, Sep 5, 2010 at 9:26 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>> 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, assuming that an enum can reach 0x80000000 different values,
> perhaps the current code is not ideal.  Still I think calling it
> "wrong" is wrong, and calling your patch a "fix" is wrong. (Same as
> calling patches that remove a warning a "fix", they are workarounds)

On what basis do you still claim that? I think I explained the problem
at detail. There is a bug. I have a fix for the bug. The fix is not a
workaround, except maybe for committee-induced stupidity which created
the enum signedness ambiguity in the first place.

>>>> > 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.
>>
>> That seems to be the best solution, I get no warnings with this:
>
> While the resulting code is clean (just as the current code), I think
> it really shows that this warning should not be enabled.  At this
> point you find yourself working around your compiler and potentially
> forcing other write some really strange code to work around the
> problem caused by this.

The warnings generated by -Wtype-limits are very useful, because with
it I have found several bugs in the code. Even the patches that are
not bugs fixes are cleanups, not 'some really strange code'. Please
take a look at the 15 piece patch set I sent last, the patches
identify the problems better than this one you are replying to. Which
ones do you still think are only workarounds? Please be more specific.

> There are many warnings that should not be enabled by default for this
> reason (like the uninitialised variable warning) unless they are fixed
> to be really intelligent (which is unlikely in this case).

Please review the latest set of patches and provide hard facts to
support your claims.

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

[andrzej zaborowski]

On 5 September 2010 18:15, Blue Swirl <blauwirbel@gmail.com> wrote:
> On Sun, Sep 5, 2010 at 3:26 PM, andrzej zaborowski <balrogg@gmail.com> wrote:
>> On 5 September 2010 11:44, Blue Swirl <blauwirbel@gmail.com> wrote:
>>> On Sun, Sep 5, 2010 at 9:26 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>> 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, assuming that an enum can reach 0x80000000 different values,
>> perhaps the current code is not ideal.  Still I think calling it
>> "wrong" is wrong, and calling your patch a "fix" is wrong. (Same as
>> calling patches that remove a warning a "fix", they are workarounds)
>
> On what basis do you still claim that?

I wanted to ask the same question.  Without constants in the
definition, the values of an enum range from 0 to N-1.  You explained
that if the enum had INT_MAX different values, then the signedness of
the values would matter (but for it to be signed would require it to
have constants again, which is not the case for enumerations of types
of an event).  Can an enum even have INT_MAX values?  It for sure
can't have UINT_MAX values.  You failed to give an example value which
would make any difference in the result of the check.  Perhaps I'm
misunderstanding where you see the bug.

> I think I explained the problem
> at detail. There is a bug. I have a fix for the bug. The fix is not a
> workaround, except maybe for committee-induced stupidity which created
> the enum signedness ambiguity in the first place.
>
>>>>> > 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.
>>>
>>> That seems to be the best solution, I get no warnings with this:
>>
>> While the resulting code is clean (just as the current code), I think
>> it really shows that this warning should not be enabled.  At this
>> point you find yourself working around your compiler and potentially
>> forcing other write some really strange code to work around the
>> problem caused by this.
>
> The warnings generated by -Wtype-limits are very useful, because with
> it I have found several bugs in the code.

Is that an argument for enabling a warning *by default*?  Looking at
any specific part of the code you'll find bugs. If you enable some
warning, it'll hint on a given subset of the places in the code, some
of which are bugs and some are false-positives.  Enable a different
warning and you get a different subset.  Grep for any given keyword or
constant and you get a different subset.

> Even the patches that are
> not bugs fixes are cleanups, not 'some really strange code'. Please
> take a look at the 15 piece patch set I sent last, the patches
> identify the problems better than this one you are replying to. Which
> ones do you still think are only workarounds? Please be more specific.

Patches 05, 06, 07, 09, 11, 14, 15 all replace one version of the code
with a different that achieves the exact same functionality for all
input values, what do they "fix"?   What is the scenario in which they
perform better.  Some of the new code has worse self-documenting
function after the change, some are actual clean-up.  The always-false
or always-true comparisons should be and are handled by the compiler
and it is a completely normal thing to write them.  Take for example
how KVM was compile-time disabled or enabled at one point, these were
all comparisons with a constant result.  I'm not sure if the warning
would have triggered because they were behind a static inline function
(do we want this to make a difference?  This is what forces the
'really strange code')

Cheers

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

[andrzej zaborowski]

On 5 September 2010 19:02, andrzej zaborowski <balrogg@gmail.com> wrote:
> Patches 05, 06, 07, 09, 11, 14, 15 all replace one version of the code
> with a different that achieves the exact same functionality for all

Sorry, patch 11 is a fix (unrelated to the warning though).

Cheers

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

[Blue Swirl]

On Sun, Sep 5, 2010 at 5:02 PM, andrzej zaborowski <balrogg@gmail.com> wrote:
> On 5 September 2010 18:15, Blue Swirl <blauwirbel@gmail.com> wrote:
>> On Sun, Sep 5, 2010 at 3:26 PM, andrzej zaborowski <balrogg@gmail.com> wrote:
>>> On 5 September 2010 11:44, Blue Swirl <blauwirbel@gmail.com> wrote:
>>>> On Sun, Sep 5, 2010 at 9:26 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>> 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, assuming that an enum can reach 0x80000000 different values,
>>> perhaps the current code is not ideal.  Still I think calling it
>>> "wrong" is wrong, and calling your patch a "fix" is wrong. (Same as
>>> calling patches that remove a warning a "fix", they are workarounds)
>>
>> On what basis do you still claim that?
>
> I wanted to ask the same question.  Without constants in the
> definition, the values of an enum range from 0 to N-1.  You explained
> that if the enum had INT_MAX different values, then the signedness of
> the values would matter

I never said anything about INT_MAX different values, you did.

> (but for it to be signed would require it to
> have constants again, which is not the case for enumerations of types
> of an event).  Can an enum even have INT_MAX values?  It for sure
> can't have UINT_MAX values.  You failed to give an example value which
> would make any difference in the result of the check.  Perhaps I'm
> misunderstanding where you see the bug.

Yes, please read the discussion again. Especially my message with the
example program.

>> I think I explained the problem
>> at detail. There is a bug. I have a fix for the bug. The fix is not a
>> workaround, except maybe for committee-induced stupidity which created
>> the enum signedness ambiguity in the first place.
>>
>>>>>> > 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.
>>>>
>>>> That seems to be the best solution, I get no warnings with this:
>>>
>>> While the resulting code is clean (just as the current code), I think
>>> it really shows that this warning should not be enabled.  At this
>>> point you find yourself working around your compiler and potentially
>>> forcing other write some really strange code to work around the
>>> problem caused by this.
>>
>> The warnings generated by -Wtype-limits are very useful, because with
>> it I have found several bugs in the code.
>
> Is that an argument for enabling a warning *by default*?  Looking at
> any specific part of the code you'll find bugs. If you enable some
> warning, it'll hint on a given subset of the places in the code, some
> of which are bugs and some are false-positives.  Enable a different
> warning and you get a different subset.  Grep for any given keyword or
> constant and you get a different subset.

Right, so when we enable *by default* the warning, buggy code (and
unfortunately the false positives, if any) will not be committed.

>> Even the patches that are
>> not bugs fixes are cleanups, not 'some really strange code'. Please
>> take a look at the 15 piece patch set I sent last, the patches
>> identify the problems better than this one you are replying to. Which
>> ones do you still think are only workarounds? Please be more specific.
>
> Patches 05, 06, 07, 09, 11, 14, 15 all replace one version of the code
> with a different that achieves the exact same functionality for all
> input values, what do they "fix"?

5: refactoring, as noted in pci.h, the code does not belong there.
6: refactoring and cleanup using the range functions.
7: cleanup leftover code.
9: cleanup. We already had a hack in place because of mingw32
compiler, replace that with a cleaner approach.
11: bug fix.
14: cleanup. Hiding semicolons after comments is asking for trouble,
this is not obfuscated C contest. I already fixed a bug because of
that, remember?
15: cleanup, declarations belong to header files, not to .c files.

>  What is the scenario in which they
> perform better.

There is no change in performance.

> Some of the new code has worse self-documenting
> function after the change, some are actual clean-up.

Which ones? I really don't see what you are complaining about.

>  The always-false
> or always-true comparisons should be and are handled by the compiler
> and it is a completely normal thing to write them.  Take for example
> how KVM was compile-time disabled or enabled at one point, these were
> all comparisons with a constant result.  I'm not sure if the warning
> would have triggered because they were behind a static inline function
> (do we want this to make a difference?  This is what forces the
> 'really strange code')

Please identify clearly any 'really strange code' which the changes
introduce in your opinion. 14 actually removes some strange
obfuscations which can be demonstrated to have caused a bug.

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

[andrzej zaborowski]

On 5 September 2010 21:16, Blue Swirl <blauwirbel@gmail.com> wrote:
> On Sun, Sep 5, 2010 at 5:02 PM, andrzej zaborowski <balrogg@gmail.com> wrote:
>> On 5 September 2010 18:15, Blue Swirl <blauwirbel@gmail.com> wrote:
>>> On Sun, Sep 5, 2010 at 3:26 PM, andrzej zaborowski <balrogg@gmail.com> wrote:
>>>> On 5 September 2010 11:44, Blue Swirl <blauwirbel@gmail.com> wrote:
>>>>> On Sun, Sep 5, 2010 at 9:26 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>>> 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, assuming that an enum can reach 0x80000000 different values,
>>>> perhaps the current code is not ideal.  Still I think calling it
>>>> "wrong" is wrong, and calling your patch a "fix" is wrong. (Same as
>>>> calling patches that remove a warning a "fix", they are workarounds)
>>>
>>> On what basis do you still claim that?
>>
>> I wanted to ask the same question.  Without constants in the
>> definition, the values of an enum range from 0 to N-1.  You explained
>> that if the enum had INT_MAX different values, then the signedness of
>> the values would matter
>
> I never said anything about INT_MAX different values, you did.

You said a BLKDBG_EVENT_MAX >= 0x80000000 and that the enum has to be
signed, how will that happen?

>
>> (but for it to be signed would require it to
>> have constants again, which is not the case for enumerations of types
>> of an event).  Can an enum even have INT_MAX values?  It for sure
>> can't have UINT_MAX values.  You failed to give an example value which
>> would make any difference in the result of the check.  Perhaps I'm
>> misunderstanding where you see the bug.
>
> Yes, please read the discussion again. Especially my message with the
> example program.

I've re-read it and confirmed that you failed to show a scenario that
the check does not address.  I'm trying to understand why you call
this code buggy.

The line is if (event < 0 || event >= BLKDBG_EVENT_MAX) { and it
assures that an out-of-range value of "event" will not get used.

>
>>> I think I explained the problem
>>> at detail. There is a bug. I have a fix for the bug. The fix is not a
>>> workaround, except maybe for committee-induced stupidity which created
>>> the enum signedness ambiguity in the first place.
>>>
>>>>>>> > 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.
>>>>>
>>>>> That seems to be the best solution, I get no warnings with this:
>>>>
>>>> While the resulting code is clean (just as the current code), I think
>>>> it really shows that this warning should not be enabled.  At this
>>>> point you find yourself working around your compiler and potentially
>>>> forcing other write some really strange code to work around the
>>>> problem caused by this.
>>>
>>> The warnings generated by -Wtype-limits are very useful, because with
>>> it I have found several bugs in the code.
>>
>> Is that an argument for enabling a warning *by default*?  Looking at
>> any specific part of the code you'll find bugs. If you enable some
>> warning, it'll hint on a given subset of the places in the code, some
>> of which are bugs and some are false-positives.  Enable a different
>> warning and you get a different subset.  Grep for any given keyword or
>> constant and you get a different subset.
>
> Right, so when we enable *by default* the warning, buggy code (and
> unfortunately the false positives, if any) will not be committed.

Ok, so "malloc causes memory leeks, let's forbid dynamic allocation", right?

>
>>> Even the patches that are
>>> not bugs fixes are cleanups, not 'some really strange code'. Please
>>> take a look at the 15 piece patch set I sent last, the patches
>>> identify the problems better than this one you are replying to. Which
>>> ones do you still think are only workarounds? Please be more specific.
>>
>> Patches 05, 06, 07, 09, 11, 14, 15 all replace one version of the code
>> with a different that achieves the exact same functionality for all
>> input values, what do they "fix"?
>
> 5: refactoring, as noted in pci.h, the code does not belong there.
> 6: refactoring and cleanup using the range functions.
> 7: cleanup leftover code.
> 9: cleanup. We already had a hack in place because of mingw32
> compiler, replace that with a cleaner approach.
> 11: bug fix.
> 14: cleanup. Hiding semicolons after comments is asking for trouble,
> this is not obfuscated C contest.

If you're used to reading one code style, other styles look like IOCCC
to you, there's no hiding anything.

> 15: cleanup, declarations belong to header files, not to .c files.

So, skipping 11 (bugfix unrelated to the warnings), where are those
"fixes"?  What is the improvement in behaviour?

>
>>  The always-false
>> or always-true comparisons should be and are handled by the compiler
>> and it is a completely normal thing to write them.  Take for example
>> how KVM was compile-time disabled or enabled at one point, these were
>> all comparisons with a constant result.  I'm not sure if the warning
>> would have triggered because they were behind a static inline function
>> (do we want this to make a difference?  This is what forces the
>> 'really strange code')
>
> Please identify clearly any 'really strange code' which the changes
> introduce in your opinion. 14 actually removes some strange
> obfuscations which can be demonstrated to have caused a bug.

I didn't say you introduced any, I'm saying that you're trying to
force new code to be written with workarounds, such as forcing hiding
a constant value behind an inline function if it is to be used as a
condition.  Knowing C syntax and the qemu documentation will not be
enough, you'll have to fight the humours of the compiler.

Cheers

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

[Blue Swirl]

On Sun, Sep 5, 2010 at 8:32 PM, andrzej zaborowski <balrogg@gmail.com> wrote:
> On 5 September 2010 21:16, Blue Swirl <blauwirbel@gmail.com> wrote:
>> On Sun, Sep 5, 2010 at 5:02 PM, andrzej zaborowski <balrogg@gmail.com> wrote:
>>> On 5 September 2010 18:15, Blue Swirl <blauwirbel@gmail.com> wrote:
>>>> On Sun, Sep 5, 2010 at 3:26 PM, andrzej zaborowski <balrogg@gmail.com> wrote:
>>>>> On 5 September 2010 11:44, Blue Swirl <blauwirbel@gmail.com> wrote:
>>>>>> On Sun, Sep 5, 2010 at 9:26 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>>>> 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, assuming that an enum can reach 0x80000000 different values,
>>>>> perhaps the current code is not ideal.  Still I think calling it
>>>>> "wrong" is wrong, and calling your patch a "fix" is wrong. (Same as
>>>>> calling patches that remove a warning a "fix", they are workarounds)
>>>>
>>>> On what basis do you still claim that?
>>>
>>> I wanted to ask the same question.  Without constants in the
>>> definition, the values of an enum range from 0 to N-1.  You explained
>>> that if the enum had INT_MAX different values, then the signedness of
>>> the values would matter
>>
>> I never said anything about INT_MAX different values, you did.
>
> You said a BLKDBG_EVENT_MAX >= 0x80000000 and that the enum has to be
> signed, how will that happen?

Please be more careful with your attributions. I did not say that either.

The problem case is when BLKDBG_EVENT_MAX > 0x80000000 and the type of
enum is unsigned. This can happen easily by

typedef enum {
    BLKDBG_EVENT_MAX = 0x80000001,
} BlkDebugEvent;

>>> (but for it to be signed would require it to
>>> have constants again, which is not the case for enumerations of types
>>> of an event).  Can an enum even have INT_MAX values?  It for sure
>>> can't have UINT_MAX values.  You failed to give an example value which
>>> would make any difference in the result of the check.  Perhaps I'm
>>> misunderstanding where you see the bug.
>>
>> Yes, please read the discussion again. Especially my message with the
>> example program.
>
> I've re-read it and confirmed that you failed to show a scenario that
> the check does not address.  I'm trying to understand why you call
> this code buggy.
>
> The line is if (event < 0 || event >= BLKDBG_EVENT_MAX) { and it
> assures that an out-of-range value of "event" will not get used.

The problem case is when BLKDBG_EVENT_MAX > 0x80000000 and the type of
enum is unsigned. Then the first check is ignored by the compiler and
the second does not catch values which are between 0x80000000 and
BLKDBG_EVENT_MAX. This may not be what was desired by the check,
though.

Those values will be caught with the int cast, or if the compiler
still happens to make the enum signed (for example, because
BLKDBG_EVENT_MAX was changed to a #define in order to support
compilers which don't allow too large enum values).

>>>> I think I explained the problem
>>>> at detail. There is a bug. I have a fix for the bug. The fix is not a
>>>> workaround, except maybe for committee-induced stupidity which created
>>>> the enum signedness ambiguity in the first place.
>>>>
>>>>>>>> > 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.
>>>>>>
>>>>>> That seems to be the best solution, I get no warnings with this:
>>>>>
>>>>> While the resulting code is clean (just as the current code), I think
>>>>> it really shows that this warning should not be enabled.  At this
>>>>> point you find yourself working around your compiler and potentially
>>>>> forcing other write some really strange code to work around the
>>>>> problem caused by this.
>>>>
>>>> The warnings generated by -Wtype-limits are very useful, because with
>>>> it I have found several bugs in the code.
>>>
>>> Is that an argument for enabling a warning *by default*?  Looking at
>>> any specific part of the code you'll find bugs. If you enable some
>>> warning, it'll hint on a given subset of the places in the code, some
>>> of which are bugs and some are false-positives.  Enable a different
>>> warning and you get a different subset.  Grep for any given keyword or
>>> constant and you get a different subset.
>>
>> Right, so when we enable *by default* the warning, buggy code (and
>> unfortunately the false positives, if any) will not be committed.
>
> Ok, so "malloc causes memory leeks, let's forbid dynamic allocation", right?

The questionable malloc policies of your employer have nothing to do
with this. If you don't agree with them, you can argue for a change in
the rules or seek employment in a company without those rules.

>>>> Even the patches that are
>>>> not bugs fixes are cleanups, not 'some really strange code'. Please
>>>> take a look at the 15 piece patch set I sent last, the patches
>>>> identify the problems better than this one you are replying to. Which
>>>> ones do you still think are only workarounds? Please be more specific.
>>>
>>> Patches 05, 06, 07, 09, 11, 14, 15 all replace one version of the code
>>> with a different that achieves the exact same functionality for all
>>> input values, what do they "fix"?
>>
>> 5: refactoring, as noted in pci.h, the code does not belong there.
>> 6: refactoring and cleanup using the range functions.
>> 7: cleanup leftover code.
>> 9: cleanup. We already had a hack in place because of mingw32
>> compiler, replace that with a cleaner approach.
>> 11: bug fix.
>> 14: cleanup. Hiding semicolons after comments is asking for trouble,
>> this is not obfuscated C contest.
>
> If you're used to reading one code style, other styles look like IOCCC
> to you

Faulty generalization.

>, there's no hiding anything.

Even if you didn't intend to hide anything, putting the semicolons
immediately after comments certainly makes them less visible. I can't
see what could ever be the benefit of doing that, for example putting
the semicolon on a separate line would make much better sense to me.

>> 15: cleanup, declarations belong to header files, not to .c files.
>
> So, skipping 11 (bugfix unrelated to the warnings), where are those
> "fixes"?  What is the improvement in behaviour?

I did not claim all of the changes are bug fixes (again, please try be
more careful), some of them are cleanups. It's OK to also clean up
code, not only fix bugs.

>>>  The always-false
>>> or always-true comparisons should be and are handled by the compiler
>>> and it is a completely normal thing to write them.  Take for example
>>> how KVM was compile-time disabled or enabled at one point, these were
>>> all comparisons with a constant result.  I'm not sure if the warning
>>> would have triggered because they were behind a static inline function
>>> (do we want this to make a difference?  This is what forces the
>>> 'really strange code')
>>
>> Please identify clearly any 'really strange code' which the changes
>> introduce in your opinion. 14 actually removes some strange
>> obfuscations which can be demonstrated to have caused a bug.

I must admit that #6 was not written as clearly as it should have.
It's easy to fix though.

> I didn't say you introduced any, I'm saying that you're trying to
> force new code to be written with workarounds, such as forcing hiding
> a constant value behind an inline function if it is to be used as a
> condition.  Knowing C syntax and the qemu documentation will not be
> enough, you'll have to fight the humours of the compiler.

Considering patches #1 to #12 (-Wtype-limits), 7 of those (1 to 4, 8,
10 and 11) fix bugs. #5 is unrelated change and #12 only changes the
flags. Even if the remaining three were considered to exist only to
please the compiler (ignoring the cleanup value), I think the ratio of
those changes to bug fixes still justifies adding the flag to catch
future bugs.

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

[andrzej zaborowski]

On 5 September 2010 23:44, Blue Swirl <blauwirbel@gmail.com> wrote:
> The problem case is when BLKDBG_EVENT_MAX > 0x80000000 and the type of
> enum is unsigned. Then the first check is ignored by the compiler and
> the second does not catch values which are between 0x80000000 and
> BLKDBG_EVENT_MAX. This may not be what was desired by the check,
> though.
>
> Those values will be caught with the int cast, or if the compiler
> still happens to make the enum signed (for example, because
> BLKDBG_EVENT_MAX was changed to a #define in order to support
> compilers which don't allow too large enum values).

So you're actually talking about INT_MAX + 1, not 0x80000000, the
number depends on the abi.

Quite clearly BLKDBG_EVENT_MAX is the max value in the enum so that
the values can be used as indices of an array of a known size.  I
think it's safe to say it is < INT_MAX.

>
>>>>> I think I explained the problem
>>>>> at detail. There is a bug. I have a fix for the bug. The fix is not a
>>>>> workaround, except maybe for committee-induced stupidity which created
>>>>> the enum signedness ambiguity in the first place.
>>>>>
>>>>>>>>> > 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.
>>>>>>>
>>>>>>> That seems to be the best solution, I get no warnings with this:
>>>>>>
>>>>>> While the resulting code is clean (just as the current code), I think
>>>>>> it really shows that this warning should not be enabled.  At this
>>>>>> point you find yourself working around your compiler and potentially
>>>>>> forcing other write some really strange code to work around the
>>>>>> problem caused by this.
>>>>>
>>>>> The warnings generated by -Wtype-limits are very useful, because with
>>>>> it I have found several bugs in the code.
>>>>
>>>> Is that an argument for enabling a warning *by default*?  Looking at
>>>> any specific part of the code you'll find bugs. If you enable some
>>>> warning, it'll hint on a given subset of the places in the code, some
>>>> of which are bugs and some are false-positives.  Enable a different
>>>> warning and you get a different subset.  Grep for any given keyword or
>>>> constant and you get a different subset.
>>>
>>> Right, so when we enable *by default* the warning, buggy code (and
>>> unfortunately the false positives, if any) will not be committed.
>>
>> Ok, so "malloc causes memory leeks, let's forbid dynamic allocation", right?
>
> The questionable malloc policies of your employer have nothing to do
> with this. If you don't agree with them, you can argue for a change in
> the rules or seek employment in a company without those rules.

First, the policy is almost identical to the policy you're introducing
so it has everything to do with this.  I'm pointing out what is an
actual faulty generalisation.  Avoiding malloc or avoiding strcat or
avoiding "if (0 <= 1)" is unlikely to reduce the number of bugs, quite
the opposite.  It's identical to arguing against file sharing on the
internet because illegal file sharing is possible, it's a faulty
generalisation. (Criminals use cars => cars are evil)  See your
statement above about buggy code not being committed.  (I've never
been employed by Nokia.  But I know people who wanted to submit
improvements to gpsd, which is the project that originally had this
policy.)

>
>>>>> Even the patches that are
>>>>> not bugs fixes are cleanups, not 'some really strange code'. Please
>>>>> take a look at the 15 piece patch set I sent last, the patches
>>>>> identify the problems better than this one you are replying to. Which
>>>>> ones do you still think are only workarounds? Please be more specific.
>>>>
>>>> Patches 05, 06, 07, 09, 11, 14, 15 all replace one version of the code
>>>> with a different that achieves the exact same functionality for all
>>>> input values, what do they "fix"?
>>>
>>> 5: refactoring, as noted in pci.h, the code does not belong there.
>>> 6: refactoring and cleanup using the range functions.
>>> 7: cleanup leftover code.
>>> 9: cleanup. We already had a hack in place because of mingw32
>>> compiler, replace that with a cleaner approach.
>>> 11: bug fix.
>>> 14: cleanup. Hiding semicolons after comments is asking for trouble,
>>> this is not obfuscated C contest.
>>
>> If you're used to reading one code style, other styles look like IOCCC
>> to you
>
> Faulty generalization.
>
>>, there's no hiding anything.
>
> Even if you didn't intend to hide anything, putting the semicolons
> immediately after comments certainly makes them less visible. I can't
> see what could ever be the benefit of doing that, for example putting
> the semicolon on a separate line would make much better sense to me.

They're exactly equally good.

>
>>> 15: cleanup, declarations belong to header files, not to .c files.
>>
>> So, skipping 11 (bugfix unrelated to the warnings), where are those
>> "fixes"?  What is the improvement in behaviour?
>
> I did not claim all of the changes are bug fixes (again, please try be
> more careful), some of them are cleanups.

You asked which ones I thought were workarounds.  If you have to make
an effort to satisfy the compiler warnings code then you have to
re-think what the warnings are for.  It really seems you want them
enabled just because they exist.

Cheers

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

[Kevin Wolf]

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

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

[Blue Swirl]

On Sun, Sep 5, 2010 at 10:33 PM, andrzej zaborowski <balrogg@gmail.com> wrote:
> On 5 September 2010 23:44, Blue Swirl <blauwirbel@gmail.com> wrote:
>> The problem case is when BLKDBG_EVENT_MAX > 0x80000000 and the type of
>> enum is unsigned. Then the first check is ignored by the compiler and
>> the second does not catch values which are between 0x80000000 and
>> BLKDBG_EVENT_MAX. This may not be what was desired by the check,
>> though.
>>
>> Those values will be caught with the int cast, or if the compiler
>> still happens to make the enum signed (for example, because
>> BLKDBG_EVENT_MAX was changed to a #define in order to support
>> compilers which don't allow too large enum values).
>
> So you're actually talking about INT_MAX + 1, not 0x80000000, the
> number depends on the abi.
>
> Quite clearly BLKDBG_EVENT_MAX is the max value in the enum so that
> the values can be used as indices of an array of a known size.  I
> think it's safe to say it is < INT_MAX.
>
>>
>>>>>> I think I explained the problem
>>>>>> at detail. There is a bug. I have a fix for the bug. The fix is not a
>>>>>> workaround, except maybe for committee-induced stupidity which created
>>>>>> the enum signedness ambiguity in the first place.
>>>>>>
>>>>>>>>>> > 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.
>>>>>>>>
>>>>>>>> That seems to be the best solution, I get no warnings with this:
>>>>>>>
>>>>>>> While the resulting code is clean (just as the current code), I think
>>>>>>> it really shows that this warning should not be enabled.  At this
>>>>>>> point you find yourself working around your compiler and potentially
>>>>>>> forcing other write some really strange code to work around the
>>>>>>> problem caused by this.
>>>>>>
>>>>>> The warnings generated by -Wtype-limits are very useful, because with
>>>>>> it I have found several bugs in the code.
>>>>>
>>>>> Is that an argument for enabling a warning *by default*?  Looking at
>>>>> any specific part of the code you'll find bugs. If you enable some
>>>>> warning, it'll hint on a given subset of the places in the code, some
>>>>> of which are bugs and some are false-positives.  Enable a different
>>>>> warning and you get a different subset.  Grep for any given keyword or
>>>>> constant and you get a different subset.
>>>>
>>>> Right, so when we enable *by default* the warning, buggy code (and
>>>> unfortunately the false positives, if any) will not be committed.
>>>
>>> Ok, so "malloc causes memory leeks, let's forbid dynamic allocation", right?
>>
>> The questionable malloc policies of your employer have nothing to do
>> with this. If you don't agree with them, you can argue for a change in
>> the rules or seek employment in a company without those rules.
>
> First, the policy is almost identical to the policy you're introducing
> so it has everything to do with this.  I'm pointing out what is an
> actual faulty generalisation.  Avoiding malloc or avoiding strcat or
> avoiding "if (0 <= 1)" is unlikely to reduce the number of bugs, quite
> the opposite.  It's identical to arguing against file sharing on the
> internet because illegal file sharing is possible, it's a faulty
> generalisation. (Criminals use cars => cars are evil)  See your
> statement above about buggy code not being committed.  (I've never
> been employed by Nokia.  But I know people who wanted to submit
> improvements to gpsd, which is the project that originally had this
> policy.)

I agree that the policy of using static allocations instead of using
malloc() will very likely generate more bugs, because people are not
used to think that way (uncommon policy) and the gap between the two
methods is not trivial. The cost is therefore big and benefit low.

Using pstrcat instead of strcat is much less of a problem: it's not
uncommon to avoid functions which may cause buffer overflows,
especially when writing hardened software and the conversion is often
trivial. The cost is then low but the benefit big.

I can't imagine how the conversions from avoiding -Wtype-limits
warnings can cause bugs. I then assume the cost is low but in any case
the benefit can be demonstrated to be big, see the bug fixes.

>>>>>> Even the patches that are
>>>>>> not bugs fixes are cleanups, not 'some really strange code'. Please
>>>>>> take a look at the 15 piece patch set I sent last, the patches
>>>>>> identify the problems better than this one you are replying to. Which
>>>>>> ones do you still think are only workarounds? Please be more specific.
>>>>>
>>>>> Patches 05, 06, 07, 09, 11, 14, 15 all replace one version of the code
>>>>> with a different that achieves the exact same functionality for all
>>>>> input values, what do they "fix"?
>>>>
>>>> 5: refactoring, as noted in pci.h, the code does not belong there.
>>>> 6: refactoring and cleanup using the range functions.
>>>> 7: cleanup leftover code.
>>>> 9: cleanup. We already had a hack in place because of mingw32
>>>> compiler, replace that with a cleaner approach.
>>>> 11: bug fix.
>>>> 14: cleanup. Hiding semicolons after comments is asking for trouble,
>>>> this is not obfuscated C contest.
>>>
>>> If you're used to reading one code style, other styles look like IOCCC
>>> to you
>>
>> Faulty generalization.
>>
>>>, there's no hiding anything.
>>
>> Even if you didn't intend to hide anything, putting the semicolons
>> immediately after comments certainly makes them less visible. I can't
>> see what could ever be the benefit of doing that, for example putting
>> the semicolon on a separate line would make much better sense to me.
>
> They're exactly equally good.

They are syntactically the same, but a human looking at the code will
have more problems when the semicolon is after the comment block.

But thank you, this amounts to you acking the separation of comments
and semicolons since either version is exactly equally good to you.

>>>> 15: cleanup, declarations belong to header files, not to .c files.
>>>
>>> So, skipping 11 (bugfix unrelated to the warnings), where are those
>>> "fixes"?  What is the improvement in behaviour?
>>
>> I did not claim all of the changes are bug fixes (again, please try be
>> more careful), some of them are cleanups.
>
> You asked which ones I thought were workarounds.  If you have to make
> an effort to satisfy the compiler warnings code then you have to
> re-think what the warnings are for.

To find bugs? Remember the bug fix to non bug fix ratio, which you
conveniently deleted from the discussion?

>  It really seems you want them
> enabled just because they exist.

I agree that this applies somewhat to #13, since the flags didn't
catch any bugs now. In my wishful thinking they may catch some in the
future. If any of them causes problems, it can be deleted without much
regret because we haven't seen the benefit.

I want the other flags enabled because they contribute positively to
the code quality.

It seems to me that you don't want to improve the quality of code but
want to stay in your comfort zone regardless of any improvement.

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

[Blue Swirl]

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!