RFR(M): 8073315: Enable gcc -Wtype-limits and fix upcoming issues.

Fri Mar 6 10:52:45 UTC 2015

Yep, I'll get right on it. It seems to be based on hs-comp(?)

Thanks for fixing this!!
/Jesper

Lindenmaier, Goetz skrev den 6/3/15 10:36:
> Hi Jesper,
>
> You offered to sponsor this change.  I think it's ok to be pushed.
> Kim and Volker are ok with it, too, and I ran it through our nightly tests.
> These cover quite a range of compilers and platforms.
>
> This is the final webrev:
> http://cr.openjdk.java.net/~goetz/webrevs/8073315-wUnsign/webrev.06/
>
> I would appreciate a lot if you could sponsor this!
>
> Best regards,
>    Goetz.
>
> -----Original Message-----
> From: Jesper Wilhelmsson [mailto:jesper.wilhelmsson at oracle.com]
> Sent: Mittwoch, 18. Februar 2015 17:29
> To: Lindenmaier, Goetz; Kim Barrett
> Cc: hotspot-dev Source Developers
> Subject: Re: RFR(M): 8073315: Enable gcc -Wtype-limits and fix upcoming issues.
>
> Hi Goetz,
>
> Lindenmaier, Goetz skrev den 18/2/15 11:53:
>> Hi Kim,
>>
>> thanks for looking at this!
>>
>>> As Jesper noted, there are cases where, yes, the
>>> warning is correct today, but future maintenance might change that.
>> You mean, somebody could change the type to signed, and then again
>> the assertion makes sense?
>> I thought about that.  One could add some function
>>      is_unsigned(uint64_t x) { return true; }
>>      is_unsigned(int64_t x) { return false; }
>> in the assertions.  It might even be possible to get a warning if the
>> second is left out, so that it's a compile time error (-Werror).
>> But it seemed a bit overengineered to me, as there's sure lots of
>> cases where changing signedness would causes similar problems
>> with current code, but there is no assertion to catch it.
>
> This approach seems, as you say, over engineered. If someone wants to change the
> type of a variable all usages of that variable should be examined anyway. There
> are several things that can go wrong with a change like that. Things the
> developers never even thought about since they had a reason to choose the type
> they used in the first place.
>
> Anyway, I think the latest webrev looks good. I can sponsor the change if Kim
> agrees that this is an OK change to do. Please note that you need a Reviewer as
> well. Both me and Kim are committers only.
> /Jesper
>
>>
>>> This warning can also lead to serious ugliness with generic code
>>> (e.g. templates), where only some values of a type parameter trigger
>>> warnings.
>> There is currently no such code in the VM.  We could again remove
>> the flag if such a case comes up.  Until then, the warning will avoid
>> future errors. And maybe, until then, openJDK uses a compiler with
>> that issue fixed.
>> Should I add a respective comment in the makefile?
>>
>> I fixed last_active_index() as you proposed.  Yes, that's better.
>> http://cr.openjdk.java.net/~goetz/webrevs/8073315-wUnsign/webrev.02/
>>
>> Best regards,
>>     Goetz.
>>
>>
>> -----Original Message-----
>> From: Kim Barrett [mailto:kim.barrett at oracle.com]
>> Sent: Mittwoch, 18. Februar 2015 00:28
>> To: Lindenmaier, Goetz
>> Cc: hotspot-dev Source Developers
>> Subject: Re: RFR(M): 8073315: Enable gcc -Wtype-limits and fix upcoming issues.
>>
>> On Feb 17, 2015, at 6:42 AM, Lindenmaier, Goetz <goetz.lindenmaier at sap.com> wrote:
>>>
>>> We fixed all the warnings showing with -Wtype-limits and gcc in our VM.
>>> I would like to contribute these small changes.
>>>
>>> This warning detects unnecessary compares for >= 0 of unsigned values.
>>>
>>> At least one warning unveils a real bug. In memory/blockOffsetTable.cpp,
>>> BlockOffsetArrayContigSpace::last_active_index(), the compare to >=0
>>> tries to prevent an underflow, but the value is unsigned.
>>>
>>> Further useless compares in real code are in os_linux.cpp,
>>> opto/graphKit.cpp, prims/jvmtiRedefineClasses.cpp
>>>
>>> In some places debug code with constant debug flags caused
>>> warnings in the opt build (g1/concurrentMark.cpp, classfile/systemDictionary.cpp)
>>>
>>> The remaining warnings occur in asserts, which is also the biggest part.
>>>
>>> The change also removes an empty #ifdef ASSERT .
>>>
>>> Please review this change.  I please need a sponsor.
>>> https://bugs.openjdk.java.net/browse/JDK-8073315
>>> http://cr.openjdk.java.net/~goetz/webrevs/8073315-wUnsign/webrev.01/
>>>
>>> I tested this on linuxx86_64 with gcc 4.1.2, 4.3 and 4.8.  Further I ran the patch
>>> with our nightly builds and tests of jdk9,see
>>> http://cr.openjdk.java.net/~simonis/ppc-aix-port/index.html
>>
>> -Wtype-limits is one of those somewhat questionable warnings that are
>> enabled by -Wextra.  As Jesper noted, there are cases where, yes, the
>> warning is correct today, but future maintenance might change that.
>> And the information that one needs in order to make that determination
>> might be far away from the expression generating the warning.
>>
>> This warning can also lead to serious ugliness with generic code
>> (e.g. templates), where only some values of a type parameter trigger
>> warnings. I've seen real code that used template metapogramming to
>> select different expressions in order to dodge this specific warning.
>>
>> In other words, while this warning does sometimes find real bugs, it
>> also tends to generate a lot of false positives.  Because of that, I'm
>> not sure it's actually a good idea to enable it.
>>
>> For the specific bug mentioned, I would like to just see that fixed,
>> but not in the way that was done in the webrev, e.g.
>>
>> incorrect old:
>>    794 size_t BlockOffsetArrayContigSpace::last_active_index() const {
>>    795   size_t result = _next_offset_index - 1;
>>    796   return result >= 0 ? result : 0;
>>    797 }mem
>>
>> webrev:
>>    794 size_t BlockOffsetArrayContigSpace::last_active_index() const {
>>    795   size_t result = _next_offset_index - 1;
>>    796   return ((intptr_t)result) >= 0 ? result : 0;
>>    797 }
>>
>> better:
>> size_t BlockOffsetArrayContigSpace::last_active_index() const {
>>     size_t next = _next_offset_index;
>>     return next == 0 ? 0 : next - 1;
>> }
>>
>> Eschew unnecessary casts!
>>