From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=59759 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OsHtE-0002AK-5C for qemu-devel@nongnu.org; Sun, 05 Sep 2010 12:16:21 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OsHtC-0002qJ-8U for qemu-devel@nongnu.org; Sun, 05 Sep 2010 12:16:20 -0400 Received: from mail-qw0-f45.google.com ([209.85.216.45]:38651) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OsHtC-0002qA-39 for qemu-devel@nongnu.org; Sun, 05 Sep 2010 12:16:18 -0400 Received: by qwh5 with SMTP id 5so3390094qwh.4 for ; Sun, 05 Sep 2010 09:16:17 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: References: <20100905075403.GC16215@redhat.com> <20100905092636.GA17394@redhat.com> From: Blue Swirl Date: Sun, 5 Sep 2010 16:15:57 +0000 Message-ID: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: [Qemu-devel] Re: [PATCH 1/5] Suppress some gcc warnings with -Wtype-limits List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: andrzej zaborowski Cc: qemu-devel , "Michael S. Tsirkin" On Sun, Sep 5, 2010 at 3:26 PM, andrzej zaborowski wrot= e: > On 5 September 2010 11:44, Blue Swirl wrote: >> On Sun, Sep 5, 2010 at 9:26 AM, Michael S. Tsirkin wrot= e: >>> 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 wr= ote: >>>> > 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 w= e >>>> >> could have: >>>> >> =C2=A0- =C2=A0 =C2=A0if (event < 0 || event >=3D BLKDBG_EVENT_MAX) = { >>>> >> =C2=A0+ =C2=A0 =C2=A0if ((int)event < 0 || event >=3D BLKDBG_EVENT_= MAX) { >>>> >> >>>> >> This would also implement the check that the writer of this code wa= s >>>> >> trying to make. >>>> >> The important thing to note is however is that the check as it is n= ow >>>> >> is not correct. > > I agree, assuming that an enum can reach 0x80000000 different values, > perhaps the current code is not ideal. =C2=A0Still 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 =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. =C2=A0The thing is it's currently= a correct >>>> >> > code and the check may be useless but it's the optimiser's task t= o >>>> >> > remove it, not ours. =C2=A0The compiler is not able to tell wheth= er the >>>> >> > check makes sense or nott, because the compiler only has access t= o >>>> >> > preprocessed code. =C2=A0So 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 ma= cro .. >>>> >>>> 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_B= ASE) >>> 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 ge= t >>> 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. =C2=A0At 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.