From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=33225 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Orvr4-000843-QX for qemu-devel@nongnu.org; Sat, 04 Sep 2010 12:44:40 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1Orvr3-0003MS-Bi for qemu-devel@nongnu.org; Sat, 04 Sep 2010 12:44:38 -0400 Received: from mail-vw0-f45.google.com ([209.85.212.45]:60244) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1Orvr3-0003MO-7S for qemu-devel@nongnu.org; Sat, 04 Sep 2010 12:44:37 -0400 Received: by vws19 with SMTP id 19so2244585vws.4 for ; Sat, 04 Sep 2010 09:44:36 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: References: Date: Sat, 4 Sep 2010 18:44:35 +0200 Message-ID: Subject: Re: [Qemu-devel] [PATCH 1/5] Suppress some gcc warnings with -Wtype-limits From: andrzej zaborowski Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Blue Swirl Cc: qemu-devel On 4 September 2010 18:14, Blue Swirl wrote: > On Sat, Sep 4, 2010 at 3:40 PM, andrzej zaborowski wr= ote: >> On 4 September 2010 16:17, Blue Swirl wrote: >>> Add various casts, adjust types etc. to make the warnings with >>> gcc flag -Wtype-limits disappear. >>> >>> Signed-off-by: Blue Swirl >>> --- >>> =C2=A0block/blkdebug.c =C2=A0 =C2=A0 =C2=A0| =C2=A0 =C2=A02 +- >>> =C2=A0hw/mips_fulong2e.c =C2=A0 =C2=A0| =C2=A0 =C2=A02 +- >>> =C2=A0hw/omap1.c =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0| =C2=A0 =C2= =A02 +- >>> =C2=A0hw/ppc405_boards.c =C2=A0 =C2=A0| =C2=A0 23 +++++++++++++--------= -- >>> =C2=A0hw/ppc_newworld.c =C2=A0 =C2=A0 | =C2=A0 =C2=A03 ++- >>> =C2=A0hw/ppc_prep.c =C2=A0 =C2=A0 =C2=A0 =C2=A0 | =C2=A0 =C2=A03 ++- >>> =C2=A0hw/pxa2xx.c =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 | =C2=A0 =C2=A02 += - >>> =C2=A0hw/sm501.c =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0| =C2=A0 =C2= =A04 ++-- >>> =C2=A0linux-user/flatload.c | =C2=A0 =C2=A03 ++- >>> =C2=A0linux-user/mmap.c =C2=A0 =C2=A0 | =C2=A0 =C2=A02 +- >>> =C2=A0linux-user/syscall.c =C2=A0| =C2=A0 20 +++++++++++++------- >>> =C2=A011 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) >>> =C2=A0 =C2=A0 struct BlkdebugRule *rule; >>> =C2=A0 =C2=A0 BlkdebugVars old_vars =3D s->vars; >>> >>> - =C2=A0 =C2=A0if (event < 0 || event >=3D BLKDBG_EVENT_MAX) { >>> + =C2=A0 =C2=A0if ((unsigned int)event >=3D BLKDBG_EVENT_MAX) { >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 return; >>> =C2=A0 =C2=A0 } >>> >>> 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, >>> =C2=A0{ >>> =C2=A0 =C2=A0 char *filename; >>> =C2=A0 =C2=A0 unsigned long ram_offset, bios_offset; >>> - =C2=A0 =C2=A0unsigned long bios_size; >>> + =C2=A0 =C2=A0long bios_size; >>> =C2=A0 =C2=A0 int64_t kernel_entry; >>> =C2=A0 =C2=A0 qemu_irq *i8259; >>> =C2=A0 =C2=A0 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, >>> =C2=A0static int omap_validate_emifs_addr(struct omap_mpu_state_s *s, >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 target_phys_add= r_t addr) >>> =C2=A0{ >>> - =C2=A0 =C2=A0return addr >=3D OMAP_EMIFS_BASE && addr < OMAP_EMIFF_BA= SE; >>> + =C2=A0 =C2=A0return 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. =C2=A0Same applies to other modifications in t= his >> 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 =3D=3D 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. =C2=A0They make you do some really strange things i= n >> 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. =C2=A0It's probably ok to enable them once to see if the= y >> 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 >=3D 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