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

[Lindenmaier, Goetz]

Hi Kim,

Do you have any more comments on this?

Best regards,
  Goetz.

-----Original Message-----
From: hotspot-dev [mailto:hotspot-dev-bounces at openjdk.java.net] On Behalf Of Lindenmaier, Goetz
Sent: Donnerstag, 19. Februar 2015 11:35
To: Kim Barrett
Cc: hotspot-dev Source Developers
Subject: RE: RFR(M): 8073315: Enable gcc -Wtype-limits and fix upcoming issues.

Hi Kim,

os_linux.cpp:
  - I adapted the type to ptrdiff_t.
  - I implemented error handling as where current directory can not be read.

g1/concurrentMark.cpp
  You're right, that #define is not nice.  And yes, the code you propose is better.
  But here I don't feel like changing it, as I don't know what was intended by the
  code.  Is it used at all? Does Oracle have a test that depends on it?
  Anyways, if the default is 0, it's off per default if somebody enables
  G1StressConcRegionFreeing, which I also consider a strange behaviour.
  The code was added with 6977804: G1: remove the zero-filling thread:
  http://hg.openjdk.java.net/jdk9/hs-rt/hotspot/rev/0fa27f37d4d4
  I can't find the review thread in the gc mail archive.  There is only the submit:
  http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2011-January/002243.html

jvmtiRedefineClasses.cpp
  Changed to u1 and removed comment.
  The value never exceeds 255.  u1 is used at other places for frame_type, too.
  So I think it's good to change it to u1.

The new webrev is here:
http://cr.openjdk.java.net/~goetz/webrevs/8073315-wUnsign/webrev.03/

Thanks for the detailed review!
Best regards,
  Goetz.


-----Original Message-----
From: Kim Barrett [mailto:kim.barrett at oracle.com] 
Sent: Donnerstag, 19. Februar 2015 02:21
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:
> 
> http://cr.openjdk.java.net/~goetz/webrevs/8073315-wUnsign/webrev.02/

I wonder how many of the changes being made here are the result of
code that originally used int for some counters, indices, or size
values, and was later changed to use an appropriate unsigned type.

------------------------------------------------------------------------------ 
src/os/linux/vm/os_linux.cpp  
3751       size_t top_overlap = requested_addr + (bytes + gap) - base[i];
3752       if (top_overlap >= 0 && top_overlap < bytes) {
=>
3751       size_t top_overlap = requested_addr + (bytes + gap) - base[i];
3752       if (requested_addr + (bytes + gap) >= base[i] && // Check underflow.
3753           top_overlap < bytes) {

I believe the real problem here is the type of top_overlap, which
should be ptrdiff_t rather than size_t. I *think* that if just that
type change was made, that the rest would be fine.

Similarly a few lines later for bottom_overlap.

------------------------------------------------------------------------------  
src/os/linux/vm/os_linux.cpp  
6028   int written;
...
6050   if ((written >= 0) && ((size_t)written < bufferSize) &&
6051       (pid_pos == NULL) && (core_pattern[0] != '|')) {

The real problem here is the value of "written", which is the result
of a call to jio_snprintf, is not being checked for failure. Clearly
"written" should be of signed type because of its value, but the other
changes here just continue to mask the lack of error checking.

------------------------------------------------------------------------------ 
src/share/vm/classfile/systemDictionary.cpp
1371   for (uintx it = 0; it < GCExpandToAllocateDelayMillis; it++){}

I'll grant you that one as a nice find and deletion.  (Although the
code is "harmless", since the compiler should optimize it away.)

------------------------------------------------------------------------------
src/share/vm/gc_implementation/g1/concurrentMark.cpp
2173 #ifndef PRODUCT
2174       if (G1StressConcRegionFreeing) {
2175         for (uintx i = 0; i < G1StressConcRegionFreeingDelayMillis; ++i) {
2176           os::sleep(Thread::current(), (jlong) 1, false);
2177         }
2178       }
2179 #endif

The #ifndef here is the kind of workaround for the warning that I
really dislike.

But why isn't this just

  if (G1StressConcRegionFreeing) {
    os::sleep(Thread::current(), G1StressConcRegionFreeingDelayMillis, false);
  }     

or maybe even "true" for the third argument to sleep, to allow it to
be interruptable.

-----------------------------------------------------------------------------
src/share/vm/prims/jvmtiRedefineClasses.cpp
2908     if (frame_type >= 0 && frame_type <= 63) {
=>
2908     if (frame_type <= 63) {

There is a comment a few lines back:

2897     // The Linux compiler does not like frame_type to be u1 or u2. It
2898     // issues the following warning for the first if-statement below:
2899     //
2900     // "warning: comparison is always true due to limited range of data type"

It looks like someone got that warning and tried to work around it in
a different way, without really understanding what was going on.

Eliminating the (frame >= 0) expression is ok, but the comment should
be eliminated and perhaps consider changing the type for frame_type to
u1?  I'm less sure about the type change.

------------------------------------------------------------------------------