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

[Kim Barrett]

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 }

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!