Warnings Cleanup in Hotspot

[Jeremy Manson]

These were warnings from a relatively recent build of Clang.  Not sure what
the current status of Clang support is (I had to make some other Clang
compatibility hacks for it to compile at all).

-Wunsequenced gave the warning in nmethod.cpp. (I believe this is
Clang-only)
-Wunused-value gave the warnings in c1_IR.cpp and in sharedRuntime.cpp.
(g++ seems to have this)

I could conceivably add a patch to the makefiles to compile with
unused-value.

Jeremy



On Thu, May 2, 2013 at 12:53 AM, Mikael Gerdin <mikael.gerdin at oracle.com>wrote:

> Jeremy,
>
>
> On 2013-05-01 22:31, Jeremy Manson wrote:
>
>> Hi folks,
>>
>> On a lark, I decided to turn a bunch of warnings on in my compiler and
>> see what happens to Hotspot.  Unsurprisingly, a lot of warnings came out.
>>
>> The largest class of real errors (which I chose to ignore for the
>> moment, under the assumption that it would be tricky to convince people
>> to accept a patch) came from having a class with a non-virtual
>> destructor, but with virtual methods.  If people are interested in the
>> results of that, I can push on it some more.
>>
>> I ended up finding a handful of "real" errors, none of which are likely
>> to be real problems, but all of which are probably worth checking in to
>> improve general code hygiene.
>>
>
> +1 for general code hygiene, but I'm afraid that if we don't enable the
> warnings which found these problems in the build we will simply
> re-introduce these subtle bugs.
>
> What warnings did you enable to find these?
>
> Another problem with warnings is that since we compile with -Werror we
> should aim to have roughly the same warnings across compilers, otherwise we
> will waste people's time when they do multi-platform builds and run into
> warnings that didn't show up on their development platform of choice.
>
> In general I'm all for enabling more warnings and cleaning up the code.
>
> /Mikael
>
>
>
>> The following patch fixes the five most trivially obviously wrong
>> things.  None of them are serious in the slightest.  Three of them are
>> statements that were probably supposed to be print statements; one of
>> them is a bug where someone assumed that arguments to printf were
>> evaluated left-to-right (instead of in an implementation-specific
>> order); and the final one is a pointer dereference that doesn't do
>> anything.  Here it is; you can feel free to tell me that you have no
>> interest:
>>
>> diff --git a/src/share/vm/c1/c1_IR.cpp b/src/share/vm/c1/c1_IR.cpp
>> --- a/src/share/vm/c1/c1_IR.cpp
>> +++ b/src/share/vm/c1/c1_IR.cpp
>> @@ -506,7 +506,7 @@
>>     _loop_map(0, 0),          // initialized later with correct size
>>     _compilation(c)
>>   {
>> -  TRACE_LINEAR_SCAN(2, "***** computing linear-scan block order");
>> +  TRACE_LINEAR_SCAN(2, tty->print_cr("***** computing linear-scan block
>> order"));
>>     init_visited();
>>     count_edges(start_block, NULL);
>> @@ -683,7 +683,7 @@
>>   }
>>   void ComputeLinearScanOrder::**assign_loop_depth(BlockBegin*
>> start_block) {
>> -  TRACE_LINEAR_SCAN(3, "----- computing loop-depth and weight");
>> +  TRACE_LINEAR_SCAN(3, tty->print_cr("----- computing loop-depth and
>> weight"));
>>     init_visited();
>>     assert(_work_list.is_empty(), "work list must be empty before
>> processing");
>> @@ -868,7 +868,7 @@
>>   }
>>   void ComputeLinearScanOrder::**compute_order(BlockBegin* start_block) {
>> -  TRACE_LINEAR_SCAN(3, "----- computing final block order");
>> +  TRACE_LINEAR_SCAN(3, tty->print_cr("----- computing final block
>> order"));
>>     // the start block is always the first block in the linear scan order
>>     _linear_scan_order = new BlockList(_num_blocks);
>> diff --git a/src/share/vm/code/nmethod.**cpp b/src/share/vm/code/nmethod.
>> **cpp
>> --- a/src/share/vm/code/nmethod.**cpp
>> +++ b/src/share/vm/code/nmethod.**cpp
>> @@ -2602,7 +2602,8 @@
>>                         relocation_begin()-1+ip[1]);
>>         for (; ip < index_end; ip++)
>>           tty->print_cr("  (%d ?)", ip[0]);
>> -      tty->print_cr("          @" INTPTR_FORMAT ": index_size=%d", ip,
>> *ip++);
>> +      tty->print_cr("          @" INTPTR_FORMAT ": index_size=%d", ip,
>> *ip);
>> +      ip++;
>>         tty->print_cr("reloc_end @" INTPTR_FORMAT ":", ip);
>>       }
>>     }
>> diff --git a/src/share/vm/runtime/**sharedRuntime.cpp
>> b/src/share/vm/runtime/**sharedRuntime.cpp
>> --- a/src/share/vm/runtime/**sharedRuntime.cpp
>> +++ b/src/share/vm/runtime/**sharedRuntime.cpp
>> @@ -2733,7 +2733,7 @@
>>     // ResourceObject, so do not put any ResourceMarks in here.
>>     char *s = sig->as_C_string();
>>     int len = (int)strlen(s);
>> -  *s++; len--;                  // Skip opening paren
>> +  s++; len--;                   // Skip opening paren
>>     char *t = s+len;
>>     while( *(--t) != ')' ) ;      // Find close paren
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/attachments/20130502/9fd81ac5/attachment.html