Warnings Cleanup in Hotspot
David Holmes
david.holmes at oracle.com
Mon May 20 19:48:45 PDT 2013
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.
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