From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=53174 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OsN0l-0006WP-M2 for qemu-devel@nongnu.org; Sun, 05 Sep 2010 17:44:29 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OsN0j-0005Nj-VF for qemu-devel@nongnu.org; Sun, 05 Sep 2010 17:44:27 -0400 Received: from mail-qy0-f180.google.com ([209.85.216.180]:40438) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OsN0j-0005Nf-Ow for qemu-devel@nongnu.org; Sun, 05 Sep 2010 17:44:25 -0400 Received: by qyk31 with SMTP id 31so4085092qyk.4 for ; Sun, 05 Sep 2010 14:44:25 -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 21:44:05 +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 8:32 PM, andrzej zaborowski wrot= e: > On 5 September 2010 21:16, Blue Swirl wrote: >> On Sun, Sep 5, 2010 at 5:02 PM, andrzej zaborowski w= rote: >>> On 5 September 2010 18:15, Blue Swirl wrote: >>>> On Sun, Sep 5, 2010 at 3:26 PM, andrzej zaborowski = wrote: >>>>> On 5 September 2010 11:44, Blue Swirl wrote: >>>>>> On Sun, Sep 5, 2010 at 9:26 AM, Michael S. Tsirkin = 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 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. Alternative= ly we >>>>>>>> >> could have: >>>>>>>> >> =C2=A0- =C2=A0 =C2=A0if (event < 0 || event >=3D BLKDBG_EVENT_M= AX) { >>>>>>>> >> =C2=A0+ =C2=A0 =C2=A0if ((int)event < 0 || event >=3D BLKDBG_EV= ENT_MAX) { >>>>>>>> >> >>>>>>>> >> This would also implement the check that the writer of this cod= e 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. =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 wanted to ask the same question. =C2=A0Without constants in the >>> definition, the values of an enum range from 0 to N-1. =C2=A0You explai= ned >>> 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 >=3D 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 =3D 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). =C2=A0Can an enum even have INT_MAX values? =C2=A0It for = sure >>> can't have UINT_MAX values. =C2=A0You failed to give an example value w= hich >>> would make any difference in the result of the check. =C2=A0Perhaps 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. =C2=A0I'm trying to understand why you call > this code buggy. > > The line is if (event < 0 || event >=3D 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 actuall= y be >>>>>>>> an assert() or it could even be removed. >>>>>>> >>>>>>> Sounds good. >>>>>>> >>>>>>>> >> >> How about adding assert(OMAP_EMIFS_BASE =3D=3D 0) and commen= ting out the >>>>>>>> >> >> check? Then if the value changes, the need to add the compar= ison back >>>>>>>> >> >> will be obvious. >>>>>>>> >> > >>>>>>>> >> > This would work but it's weird. =C2=A0The thing is it's curre= ntly a correct >>>>>>>> >> > code and the check may be useless but it's the optimiser's ta= sk to >>>>>>>> >> > remove it, not ours. =C2=A0The compiler is not able to tell w= hether the >>>>>>>> >> > check makes sense or nott, because the compiler only has acce= ss to >>>>>>>> >> > preprocessed code. =C2=A0So why should you let the compiler h= ave 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_B= ASE. >>>>>>>> >>>>>>>> I think the assertion is still the best way, it ensures that somet= hing >>>>>>>> will happen if OMAP_EMIFS_BASE changes. We could for example remov= e >>>>>>>> OMAP_EMIFS_BASE entirely (it's only used for the check), but someo= ne >>>>>>>> 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_EMI= FF_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 check= ing correctly? >>>>>>> We have a couple of range helpers in pci.h, these could be moved ou= t >>>>>>> to range.h and we could add some more. As there act on u64 this wil= l 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. =C2=A0At thi= s >>>>> 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*? =C2=A0Looking = 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. =C2=A0Enable a differen= t >>> warning and you get a different subset. =C2=A0Grep for any given keywor= d 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", rig= ht? 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"? =C2=A0What 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. >>> =C2=A0The always-false >>> or always-true comparisons should be and are handled by the compiler >>> and it is a completely normal thing to write them. =C2=A0Take for examp= le >>> how KVM was compile-time disabled or enabled at one point, these were >>> all comparisons with a constant result. =C2=A0I'm not sure if the warni= ng >>> would have triggered because they were behind a static inline function >>> (do we want this to make a difference? =C2=A0This 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. =C2=A0Knowing 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.