Warnings Cleanup in Hotspot
Dean Long
dean.long at oracle.com
Fri May 17 22:00:25 PDT 2013
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?
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
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/attachments/20130517/7910aca9/attachment-0001.html
More information about the hotspot-runtime-dev
mailing list