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