Warnings Cleanup in Hotspot

Calvin Cheung calvin.cheung at oracle.com
Tue May 21 13:39:31 PDT 2013


On 5/20/2013 7:48 PM, David Holmes wrote:
> On 18/05/2013 3:00 PM, Dean Long wrote:
>> Does this mean the recent change (8007037) at lines 1066-1068 should be
>> re-examined?  It wasn't doing what it was meant to do,
>> but no tests caught it, so don't we need a better test?
>
> Yes to both questions! I've added this info to 8007037. I would prefer 
> to see that fixed explicitly not hidden inside the warnings cleanup 
> changes.
Without the change in constantPool.cpp, the build will fail after 
enabling -Wunused-value.
I think we should just file a follow-up bug on 8007037 if it requires 
more attentions such as testing coverage.

thanks,
Calvin

>
> David
> -----
>
>> dl
>>
>> On 5/17/2013 7:06 PM, Christian Thalinger wrote:
>>>
>>> On May 17, 2013, at 4:18 PM, Jeremy Manson <jeremymanson at google.com
>>> <mailto:jeremymanson at google.com>> wrote:
>>>
>>>> Nice catch - I guess before, that was expanding to
>>>>
>>>> bool match = compare_entry_to(k1, cp2, k2, THREAD);
>>>> if (HAS_PENDING_EXCEPTION) return false;
>>>> (0) && compare_operand_to(i1, cp2, i2, THREAD);
>>>> if (HAS_PENDING_EXCEPTION) return false;
>>>> (0);
>>>>
>>>> which is perfectly legal, but booogus…
>>>
>>> Wow.  This almost makes you want to have macros in Java…
>>>
>>> -- Chris
>>>
>>>>
>>>> Jeremy
>>>>
>>>>
>>>>
>>>>
>>>> On Fri, May 17, 2013 at 3:08 PM, Calvin Cheung
>>>> <calvin.cheung at oracle.com <mailto:calvin.cheung at oracle.com>> wrote:
>>>>
>>>>     On 5/17/2013 10:32 AM, Jeremy Manson wrote:
>>>>>     I wonder why that didn't pop up when I compiled it.
>>>>     I cloned my repo from the following:
>>>>     http://hg.openjdk.java.net/hsx/hotspot-rt/hotspot
>>>>
>>>>     g++ version is 4.4.6
>>>>
>>>>     The file list I mentioned was for fastdebug build on linux_x64.
>>>>     When I switched to product build, I found couple more files need
>>>>     to be changed.
>>>>     Then I needed to change more files for the 32-bit build.
>>>>     So there will be few more files added to the list.
>>>>
>>>>>
>>>>>     I disagree with David.  Casting to void is the accepted strategy
>>>>>     for getting rid of unused value warnings.  It makes perfect
>>>>>     sense to me; if you cast to void, it isn't a value anymore.
>>>>>      It's also good documentation: it says you know you aren't using
>>>>>     the value, and that that is not a mistake.  I did it several
>>>>>     times in my patch.
>>>>>
>>>>>     Furthermore, turning on the warning will prevent mistakes like
>>>>>     the strcmp / strcpy one I fixed in my patch.  It seems worth it
>>>>>     to have a few scattered (void) casts in the VM.
>>>>     After adding the (void) cast in CHECK_false, I'm seeing the
>>>>     following error:
>>>>     constantPool.cpp:1067: error: void value not ignored as it ought
>>>>     to be
>>>>     1066     bool match = compare_entry_to(k1, cp2, k2, 
>>>> CHECK_false) &&
>>>>     1067                  compare_operand_to(i1, cp2, i2, 
>>>> CHECK_false);
>>>>     1068     return match;
>>>>
>>>>     the way I fixed it is to break up 1066 - 1067 into 2 statements
>>>>     such as follows:
>>>>     1066     bool match_entry = compare_entry_to(k1, cp2, k2,
>>>>     CHECK_false);
>>>>     1067     bool match_operand = compare_operand_to(i1, cp2, i2,
>>>>     CHECK_false);
>>>>     1068     return (match_entry && match_operand);
>>>>
>>>>     Calvin
>>>>
>>>>>
>>>>>     Jeremy
>>>>>
>>>>>
>>>>>     On Thu, May 16, 2013 at 6:59 PM, David Holmes
>>>>>     <david.holmes at oracle.com <mailto:david.holmes at oracle.com>> wrote:
>>>>>
>>>>>         Calvin,
>>>>>
>>>>>
>>>>>         On 17/05/2013 4:55 AM, Calvin Cheung wrote:
>>>>>
>>>>>             Jeremy,
>>>>>
>>>>>             It seems your suggested patch is incomplete as the build
>>>>>             on linux was
>>>>>             failing.
>>>>>             Recall that warnings are treated as errors as in 
>>>>> gcc.make:
>>>>>                # Compiler warnings are treated as errors
>>>>>                WARNINGS_ARE_ERRORS = -Werror
>>>>>
>>>>>             Below is the list of files I needed to change for the
>>>>>             build to go
>>>>>             through on linux:
>>>>>             make/linux/makefiles/gcc.make
>>>>>             src/share/vm/c1/c1_IR.cpp
>>>>>             src/share/vm/ci/ciUtilities.hpp
>>>>>             src/share/vm/classfile/genericSignatures.cpp
>>>>>             src/share/vm/classfile/verifier.hpp
>>>>>             src/share/vm/code/nmethod.cpp
>>>>>             src/share/vm/memory/cardTableModRefBS.cpp
>>>>>             src/share/vm/memory/universe.cpp
>>>>>             src/share/vm/oops/constantPool.cpp
>>>>>             src/share/vm/prims/forte.cpp
>>>>>             src/share/vm/runtime/perfData.hpp
>>>>>             src/share/vm/runtime/sharedRuntime.cpp
>>>>>             src/share/vm/services/diagnosticArgument.cpp
>>>>>             src/share/vm/utilities/exceptions.hpp
>>>>>             src/share/vm/utilities/taskqueue.hpp
>>>>>
>>>>>             Most of the warnings are related to: "statement has no
>>>>>             effect"
>>>>>             e.g. in exceptions.hpp, I needed to change
>>>>>             #define CHECK              THREAD); if
>>>>>             (HAS_PENDING_EXCEPTION) return ; (0
>>>>>             to
>>>>>             #define CHECK              THREAD); if
>>>>>             (HAS_PENDING_EXCEPTION) return ; (void)(0
>>>>>
>>>>>             I don't want to share webrev yet since I haven't built
>>>>>             on other
>>>>>             platforms and haven't done any testing on the change.
>>>>>
>>>>>             2 ways I'm thinking of fixing it:
>>>>>             1) include the -Wunused-value warning flag
>>>>>                  this means changes are required on the files listed
>>>>>             above
>>>>>
>>>>>             2) not include the -Wunused-value warning flag but only
>>>>>             fixing the
>>>>>             potential coding errors which would impact functionality
>>>>>             as you pointed
>>>>>             out in your email. This should result in a smaller
>>>>>             changeset.
>>>>>
>>>>>
>>>>>         I think I prefer #2 mainly because:
>>>>>
>>>>>         0;
>>>>>
>>>>>         and
>>>>>
>>>>>         (void)0;
>>>>>
>>>>>         are equally bereft of any effect so I don't see why the
>>>>>         compiler complains about one and accepts the other. As this
>>>>>         seems completely arbitrary different compilers may have
>>>>>         different views, or change their view over time. So I
>>>>>         suggest not changing this and only fix the real issues that
>>>>>         -Wunused-value produces.
>>>>>
>>>>>         Just my 2c.
>>>>>
>>>>>         David
>>>>>         ------
>>>>>
>>>>>
>>>>>             Calvin
>>>>>
>>>>>             On 5/16/2013 10:55 AM, Jeremy Manson wrote:
>>>>>
>>>>>                 Okay.  Is there anything you want me to do?
>>>>>
>>>>>                 Jeremy
>>>>>
>>>>>
>>>>>                 On Wed, May 15, 2013 at 2:37 PM, Calvin Cheung
>>>>>                 <calvin.cheung at oracle.com
>>>>>                 <mailto:calvin.cheung at oracle.com>
>>>>>                 <mailto:calvin.cheung at oracle.com
>>>>> <mailto:calvin.cheung at oracle.com>>> wrote:
>>>>>
>>>>>                     Jeremy,
>>>>>
>>>>>                     We'll fix it in hs25 first and then backport it
>>>>>                 to hs24 if the fix
>>>>>                     is applicable there.
>>>>>
>>>>>                     Calvin
>>>>>
>>>>>
>>>>>                     On 5/15/2013 2:14 PM, Jeremy Manson wrote:
>>>>>
>>>>>                         Thanks, Calvin.  What does followup look 
>>>>> like?
>>>>>
>>>>>                         Jeremy
>>>>>
>>>>>
>>>>>                         On Mon, May 13, 2013 at 9:59 AM, Calvin 
>>>>> Cheung
>>>>>                         <calvin.cheung at oracle.com
>>>>> <mailto:calvin.cheung at oracle.com>
>>>>>                     <mailto:calvin.cheung at oracle.com
>>>>> <mailto:calvin.cheung at oracle.com>>> wrote:
>>>>>
>>>>>                             Hi Jeremy,
>>>>>
>>>>>                             I've created 8014431 (cleanup warnings
>>>>>                     indicated by the
>>>>>                             -Wunused-value compiler option on linux)
>>>>>                     to track this. It'll
>>>>>                             take a few hours for it to show up in
>>>>>                     the bugs.sun.com <http://bugs.sun.com/>
>>>>>                             <http://bugs.sun.com 
>>>>> <http://bugs.sun.com/>>
>>>>> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8014431
>>>>>
>>>>>                             Calvin
>>>>>
>>>>>
>>>>>                             On 5/10/2013 11:38 AM, Jeremy Manson 
>>>>> wrote:
>>>>>
>>>>>                                 Okay, I turned on -Wunused-value in
>>>>>                     the gcc makefile for
>>>>>                                 Linux.  I also ran this on a more
>>>>>                     recent build of HS (I
>>>>>                                 was using something very, very old),
>>>>>                     and I got a few more
>>>>>                                 warnings (including a couple that
>>>>>                     actually looked as if
>>>>>                                 they would impact functionality).
>>>>>                      Thoughts on this?
>>>>>
>>>>>                                 diff --git
>>>>>                     a/make/linux/makefiles/gcc.make
>>>>>                     b/make/linux/makefiles/gcc.make
>>>>>                                 --- a/make/linux/makefiles/gcc.make
>>>>>                                 +++ b/make/linux/makefiles/gcc.make
>>>>>                                 @@ -126,7 +126,7 @@
>>>>>                                  # Compiler warnings are treated as
>>>>>                     errors
>>>>>                                  WARNINGS_ARE_ERRORS = -Werror
>>>>>                                 -WARNING_FLAGS = -Wpointer-arith
>>>>>                     -Wsign-compare -Wundef
>>>>>                                 -Wunused-function
>>>>>                                 +WARNING_FLAGS = -Wpointer-arith
>>>>>                     -Wsign-compare -Wundef
>>>>>                                 -Wunused-function -Wunused-value
>>>>>                                  # Since GCC 4.3, -Wconversion has
>>>>>                     changed its meanings
>>>>>                                 to warn these implicit
>>>>>                                  # conversions which might affect
>>>>>                     the values. Only enable
>>>>>                                 it in earlier versions.
>>>>>                                 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/memory/cardTableModRefBS.cpp
>>>>> b/src/share/vm/memory/cardTableModRefBS.cpp
>>>>>                                 ---
>>>>> a/src/share/vm/memory/cardTableModRefBS.cpp
>>>>>                                 +++
>>>>> b/src/share/vm/memory/cardTableModRefBS.cpp
>>>>>                                 @@ -395,7 +395,7 @@
>>>>>                                    }
>>>>>                                    // Touch the last card of the
>>>>>                     covered region to show
>>>>>                                 that it
>>>>>                                    // is committed (or SEGV).
>>>>>                                 -
>>>>> debug_only(*byte_for(_covered[ind].last());)
>>>>>                                 +  debug_only((void)
>>>>>                     (*byte_for(_covered[ind].last()));)
>>>>>                      debug_only(verify_guard();)
>>>>>                                  }
>>>>>                                 diff --git
>>>>>                     a/src/share/vm/memory/universe.cpp
>>>>>                     b/src/share/vm/memory/universe.cpp
>>>>>                                 --- 
>>>>> a/src/share/vm/memory/universe.cpp
>>>>>                                 +++ 
>>>>> b/src/share/vm/memory/universe.cpp
>>>>>                                 @@ -532,7 +532,9 @@
>>>>>                                    if (vt)
>>>>>                     vt->initialize_vtable(false, CHECK);
>>>>>                                    if (ko->oop_is_instance()) {
>>>>>                                      InstanceKlass* ik =
>>>>>                     (InstanceKlass*)ko;
>>>>>                                 -    for (KlassHandle s_h(THREAD,
>>>>>                     ik->subklass()); s_h()
>>>>>                                 != NULL; s_h = (THREAD,
>>>>>                     s_h()->next_sibling())) {
>>>>>                                 +    for (KlassHandle s_h(THREAD,
>>>>>                     ik->subklass());
>>>>>                                 +         s_h() != NULL;
>>>>>                                 +         s_h = KlassHandle(THREAD,
>>>>>                     s_h()->next_sibling())) {
>>>>>                      reinitialize_vtable_of(s_h, CHECK);
>>>>>                                      }
>>>>>                                    }
>>>>>                                 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
>>>>>                                 diff --git
>>>>> a/src/share/vm/services/diagnosticArgument.cpp
>>>>> b/src/share/vm/services/diagnosticArgument.cpp
>>>>>                                 ---
>>>>> a/src/share/vm/services/diagnosticArgument.cpp
>>>>>                                 +++
>>>>> b/src/share/vm/services/diagnosticArgument.cpp
>>>>>                                 @@ -232,7 +232,7 @@
>>>>>                                    } else {
>>>>>                                      _value._time = 0;
>>>>>                      _value._nanotime = 0;
>>>>>                                 -  strcmp(_value._unit, "ns");
>>>>>                                 +  strcpy(_value._unit, "ns");
>>>>>                                    }
>>>>>                                  }
>>>>>                                 diff --git
>>>>>                     a/src/share/vm/utilities/taskqueue.hpp
>>>>>                     b/src/share/vm/utilities/taskqueue.hpp
>>>>>                                 ---
>>>>>                     a/src/share/vm/utilities/taskqueue.hpp
>>>>>                                 +++
>>>>>                     b/src/share/vm/utilities/taskqueue.hpp
>>>>>                                 @@ -340,8 +340,12 @@
>>>>>                                    if (dirty_n_elems == N - 1) {
>>>>>                                      // Actually means 0, so do the
>>>>>                     push.
>>>>>                                      uint localBot = _bottom;
>>>>>                                 -    // g++ complains if the
>>>>>                     volatile result of the
>>>>>                                 assignment is unused.
>>>>>                                 - const_cast<E&>(_elems[localBot] 
>>>>> = t);
>>>>>                                 +    // g++ complains if the
>>>>>                     volatile result of the
>>>>>                                 assignment is
>>>>>                                 +    // unused, so we cast the
>>>>>                     volatile away.  We cannot
>>>>>                                 cast directly
>>>>>                                 +    // to void, because gcc treats
>>>>>                     that as not using the
>>>>>                                 result of the
>>>>>                                 +    // assignment. However,
>>>>>                     casting to E& means that we
>>>>>                                 trigger an
>>>>>                                 +    // unused-value warning.  So,
>>>>>                     we cast the E& to void.
>>>>>                                 +    (void)
>>>>> const_cast<E&>(_elems[localBot] = t);
>>>>> OrderAccess::release_store(&_bottom,
>>>>>                     increment_index(localBot));
>>>>> TASKQUEUE_STATS_ONLY(stats.record_push());
>>>>>                                      return true;
>>>>>                                 @@ -397,7 +401,12 @@
>>>>>                                      return false;
>>>>>                                    }
>>>>>                                 - const_cast<E&>(t =
>>>>>                     _elems[oldAge.top()]);
>>>>>                                 +  // g++ complains if the volatile
>>>>>                     result of the
>>>>>                                 assignment is
>>>>>                                 +  // unused, so we cast the
>>>>>                     volatile away.  We cannot
>>>>>                                 cast directly
>>>>>                                 +  // to void, because gcc treats
>>>>>                     that as not using the
>>>>>                                 result of the
>>>>>                                 +  // assignment. However, casting
>>>>>                     to E& means that we
>>>>>                                 trigger an
>>>>>                                 +  // unused-value warning.  So, we
>>>>>                     cast the E& to void.
>>>>>                                 +  (void) const_cast<E&>(t =
>>>>>                     _elems[oldAge.top()]);
>>>>>                                    Age newAge(oldAge);
>>>>>                      newAge.increment();
>>>>>                                    Age resAge = _age.cmpxchg(newAge,
>>>>>                     oldAge);
>>>>>                                 @@ -640,8 +649,12 @@
>>>>>                                    uint dirty_n_elems =
>>>>>                     dirty_size(localBot, top);
>>>>>                      assert(dirty_n_elems < N, "n_elems out of 
>>>>> range.");
>>>>>                                    if (dirty_n_elems < max_elems()) {
>>>>>                                 -    // g++ complains if the
>>>>>                     volatile result of the
>>>>>                                 assignment is unused.
>>>>>                                 - const_cast<E&>(_elems[localBot] 
>>>>> = t);
>>>>>                                 +    // g++ complains if the
>>>>>                     volatile result of the
>>>>>                                 assignment is
>>>>>                                 +    // unused, so we cast the
>>>>>                     volatile away.  We cannot
>>>>>                                 cast directly
>>>>>                                 +    // to void, because gcc treats
>>>>>                     that as not using the
>>>>>                                 result of the
>>>>>                                 +    // assignment. However,
>>>>>                     casting to E& means that we
>>>>>                                 trigger an
>>>>>                                 +    // unused-value warning.  So,
>>>>>                     we cast the E& to void.
>>>>>                                 +    (void)
>>>>> const_cast<E&>(_elems[localBot] = t);
>>>>> OrderAccess::release_store(&_bottom,
>>>>>                     increment_index(localBot));
>>>>> TASKQUEUE_STATS_ONLY(stats.record_push());
>>>>>                                      return true;
>>>>>                                 @@ -665,7 +678,12 @@
>>>>>                                    // This is necessary to prevent
>>>>>                     any read below from
>>>>>                                 being reordered
>>>>>                                    // before the store just above.
>>>>>                      OrderAccess::fence();
>>>>>                                 - const_cast<E&>(t = 
>>>>> _elems[localBot]);
>>>>>                                 +  // g++ complains if the volatile
>>>>>                     result of the
>>>>>                                 assignment is
>>>>>                                 +  // unused, so we cast the
>>>>>                     volatile away.  We cannot
>>>>>                                 cast directly
>>>>>                                 +  // to void, because gcc treats
>>>>>                     that as not using the
>>>>>                                 result of the
>>>>>                                 +  // assignment. However, casting
>>>>>                     to E& means that we
>>>>>                                 trigger an
>>>>>                                 +  // unused-value warning.  So, we
>>>>>                     cast the E& to void.
>>>>>                                 +  (void) const_cast<E&>(t =
>>>>>                     _elems[localBot]);
>>>>>                                    // This is a second read of
>>>>>                     "age"; the "size()" above
>>>>>                                 is the first.
>>>>>                                    // If there's still at least one
>>>>>                     element in the queue,
>>>>>                                 based on the
>>>>>                                    // "_bottom" and "age" we've
>>>>>                     read, then there can be
>>>>>                                 no interference with
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>
>>



More information about the hotspot-runtime-dev mailing list