Warnings Cleanup in Hotspot
Calvin Cheung
calvin.cheung at oracle.com
Fri May 17 15:08:00 PDT 2013
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/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/3c1e4f5d/attachment-0001.html
More information about the hotspot-runtime-dev
mailing list