[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 <address@hidden> wrote:
> On 5 September 2010 18:15, Blue Swirl <address@hidden> wrote:
>> On Sun, Sep 5, 2010 at 3:26 PM, andrzej zaborowski <address@hidden> wrote:
>>> On 5 September 2010 11:44, Blue Swirl <address@hidden> wrote:
>>>> On Sun, Sep 5, 2010 at 9:26 AM, Michael S. Tsirkin <address@hidden> 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 <address@hidden> 
>>>>>> 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.