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

[Jesper Wilhelmsson]

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