Warnings Cleanup in Hotspot
David Holmes
david.holmes at oracle.com
Wed May 22 04:40:06 PDT 2013
On 22/05/2013 6:39 AM, Calvin Cheung wrote:
> 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.
We can file a separate bug for it now and still fix it as part of this
set of changes. Just requires its own commit. But both changesets can be
pushed through together.
David
> 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