Warnings Cleanup in Hotspot

[Mikael Gerdin]

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
>