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

[Blue Swirl]

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 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. 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.