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

[andrzej zaborowski]

On 5 September 2010 21:16, Blue Swirl <address@hidden> wrote:
> 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.

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