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

[Lindenmaier, Goetz]

Hi,

I additionally updated the copyrights in this webrev ;)

Best regards,
  Goetz.

-----Original Message-----
From: Lindenmaier, Goetz 
Sent: Mittwoch, 18. Februar 2015 11:53
To: 'Kim Barrett'
Cc: hotspot-dev Source Developers
Subject: RE: RFR(M): 8073315: Enable gcc -Wtype-limits and fix upcoming issues.

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