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

[Lindenmaier, Goetz]

Hi Kim, 

I can follow your argumentation about problems that could arise.
Nevertheless I think this already unveiled a sufficient number
of problematic compares to legitimate enabling the warning.
And there is no false positive in the code.  If so, the warning still
can be disabled again.

Best regards,
  Goetz.

-----Original Message-----
From: Kim Barrett [mailto:kim.barrett at oracle.com] 
Sent: Mittwoch, 18. Februar 2015 23:22
To: Lindenmaier, Goetz
Cc: hotspot-dev Source Developers
Subject: Re: RFR(M): 8073315: Enable gcc -Wtype-limits and fix upcoming issues.

On Feb 18, 2015, at 5:53 AM, Lindenmaier, Goetz <goetz.lindenmaier at sap.com> wrote:
> You mean, somebody could change the type to signed, and then again
> the assertion makes sense? 

Yes.  That sort of thing happens.  In my experience it happens more
frequently in the signed to unsigned direction, e.g. the original code
"casually" used int but later changed to use size_t.  Such a change
could trip the warning in lots of otherwise uninteresting far away
code that isn't actually broken at all; it just contains expressions
that any competent compiler (particularly one capable of generating
this warning) will now optimize away.

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

That doesn't work except for uint64_t or int64_t argument types.  For
any other argument type, this will fail to compile due to ambiguous
overloads.

For complete coverage, portability, and correctness, this kind of
thing is generally done using template metaprogramming, typically
using std::numeric_limits<>.

> It might even be possible to get a warning if the
> second is left out, so that it's a compile time error (-Werror).

That doesn't work, due to integer conversion rules.

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

The VM code barely uses templates at all at present, though that seems
to be changing.  This situation also arises in certain kinds of
macros.  It might also arise with code dealing with enums.

And as it happens, I have a change set out for review right now that
should have exactly this problem. [It presently doesn't because I had
to tweak and uglify the code because the code I wanted failed to
compile on MacOSX in a way that looks like (some part of?)
-Wtype-limits is turned on? I should probably try harder to figure out
what's really going wrong, but that changeset has been hanging around
for a while and is blocking other work.]

> And maybe, until then, openJDK uses a compiler with
> that issue fixed.

What compiler issue?  There's no compiler issue here.  The warning
does exactly what it says, which is warn about code that is correct
according to the language but is going to be optimized away, because
*sometimes* that indicates the code is not doing what the programmer
thought it was doing.

I'm a big fan of warning flags that pick up a significant number of
real mistakes with few false positives.  I really wish hotspot could
be built with -Wall, and think it would be worth some effort to get
there.  But there are good reasons why -Wtype-limits is in the -Wextra
set rather than the -Wall set.