Warnings Cleanup in Hotspot
Calvin Cheung
calvin.cheung at oracle.com
Tue May 21 12:28:49 PDT 2013
On 5/21/2013 12:03 PM, Volker Simonis wrote:
>
>
>
> On Sat, May 18, 2013 at 12:08 AM, 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;
>
>
> 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.
I'm about to send a review request for
http://bugs.sun.com/view_bug.do?bug_id=8014431
> 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?
I'm not sure if there's one.
Calvin
>
> 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/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/ac67d380/attachment-0001.html
More information about the hotspot-runtime-dev
mailing list