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

[Kim Barrett]

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.

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