Warnings Cleanup in Hotspot
Volker Simonis
volker.simonis at gmail.com
Tue May 21 12:03:40 PDT 2013
On Sat, May 18, 2013 at 12:08 AM, Calvin Cheung <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;
>
>
Clang gives a nice and clear error here (even without -Werror):
/share/software/Java/OpenJDK/hsx/hotspot-rt/hotspot/src/share/vm/oops/constantPool.cpp:1066:48:
error: value of type 'void' is not
contextually convertible to 'bool'
bool match = compare_entry_to(k1, cp2, k2, CHECK_false) &&
^~~~~~~~~~~~
/share/software/Java/OpenJDK/hsx/hotspot-rt/hotspot/src/share/vm/utilities/exceptions.hpp:202:50:
note: expanded from:
#define CHECK_false CHECK_(false)
^
/share/software/Java/OpenJDK/hsx/hotspot-rt/hotspot/src/share/vm/utilities/exceptions.hpp:198:101:
note: expanded from:
#define CHECK_(result) THREAD); if
(HAS_PENDING_EXCEPTION) return result; (void)(0
^
/share/software/Java/OpenJDK/hsx/hotspot-rt/hotspot/src/share/vm/oops/constantPool.cpp:1066:61:
error: invalid operands to binary
expression ('void' and 'bool')
bool match = compare_entry_to(k1, cp2, k2, CHECK_false) &&
~~~~~~~~~~~~ ^
I have a nice Clang-patch for the current hotspot-rt repo which I could
provide. All it requires is setting USE_CLANG=true on the hotspot build
command line. Is there already a Bug ID for an Clang-enabled HotSpot build?
> 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>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>> 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>>
>>>>> 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/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/20130521/aea8d507/attachment-0001.html
More information about the hotspot-runtime-dev
mailing list