RFR (L) JDK-7195622: CheckUnhandledOops has limited usefulness now
stefan.karlsson at oracle.com
Fri Sep 20 05:46:44 PDT 2013
On 09/19/2013 05:13 PM, Lois Foltan wrote:
> Please review the following fix:
The changes looks good.
There are some changes that I don't fully agree with, but I will not
block the push because of them. I'm just going to mention them here for
1) I would personally prefer not to have the CAST_TO_OOP/CAST_FROM_OOP
macros and instead explicitly add (void *) casts.
2) I don't understand the need make the code harder to read by adding a
SOLAR_ONLY((void*)) instead of just a plain (void*).
I'm looking forward to be able to use this feature on Linux!
> Bug: JDK8 b44 hotspot:src/share/vm/oops/klass.hpp: Error:Initializing
> const volatile oop& requires ... &
> CheckUnhandledOops has limited usefulness now bug links at:
> Summary of fix:
> Update the C++ oop structure definition in oopsHierarchy.hpp to solve
> several problems with the current definition when compiled with
> various C++ compilers across supported platforms. These changes
> initially address the problem reported in JDK-7180556 and continue
> with additional improvements to allow CHECK_UNHANDLED_OOPS to be
> defined in all fastdebug builds on all platforms as suggested in
> JDK-7195622. Several notes concerning this fix:
> 1. A review should start at understanding the changes made
> to oopsHierarchy.hpp
> a. Addition of a non-volatile copy constructor to
> address compile time errors
> reported in JDK-7180556 and also currently by
> g++ compilers on Linux.
> b. Addition of member wise assignment operators
> to handle many instances
> of [non]volatile to [non]volatile oops within
> the JVM.
> Note: Solaris compilers would not allow for
> the member wise assignment operators
> of every flavor of non-volatile to
> volatile and vice versa. However, unlike g++ compilers,
> Solaris compilers had no issue
> passing a volatile "this" pointer to a non-volatile
> assignment operator. So the g++
> compilers needed these different flavors
> of the assignment operator and
> Solaris did not.
> d. For similar reasons as 1b, addition of a
> volatile explicit conversion from oop -> void *.
> g++ specifically complained when trying to
> pass a volatile "this" pointer.
> e. Removal of the ambiguous behavior of having
> overloaded copy constructor and
> explicit user conversion member functions
> defined of both integral and void *.
> All C++ compilers, except Solaris, issue a
> compile time error concerning this ambiguity.
> 2. Change #1e had the consequence of C++ compilers now
> generating compile time
> errors where the practice has been to cast an oop to
> and from an integral type such
> as intptr_t. To allow for this practice to continue,
> when oop is a structure and not
> a OopDesc *, as is the case when CHECK_UNHANDLED_OOPS
> is defined, two new
> macros were introduced within globalDefinitions.hpp,
> CAST_TO_OOP and CAST_FROM_OOP.
> 3. Due to the change in #1a & #1b, the oop structure in no
> longer considered a POD (plain old data)
> by the g++ compilers on Linux and MacOS. This caused
> several changes to be needed
> throughout the JVM to add an (void *) cast of an oop
> when invoking print_cr().
> 4. Many instances of an assignment to a volatile oop
> required a "const_cast<oop&>" to
> cast away the volatileness of the lvalue. There is
> already precedence for this
> type of change within utilities/taskqueue.hpp. The
> following comment was in place:
> // g++ complains if the volatile result of the assignment is
> // unused, so we cast the volatile away. We cannot cast
> // 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.
> 5. The addition of the volatile keyword to the
> GenericTaskQueue's pop_local() & pop_global()
> member functions was to accommodate the Solaris C++
> compiler's complaint about the assignment
> of the volatile elements of the private member data
> _elems when GenericTaskQueue is instantiated
> with a non-volatile oop. Line #723 in pop_local().
> This was a result of #1b, Solaris' lack of
> allowing for all flavors of volatile/non-volatile
> member wise assignment operators.
> Per Liden of the GC group did pre-review this specific
> change with regards to performance
> implications and was not concerned.
> 6. In utilities/hashtable.cpp, required
> CHECK_UNHANDLED_OOPS conditional for the instantiation
> of "template class Hashtable<oop, mtSymbol>". With
> CHECK_UHANDLED_OOPS specified for a
> fastdebug build, an unresolved symbol occurred.
> philli:% nm --demangle
> build//linux_amd64_compiler2/fastdebug/libjvm.so | grep Hashtable |
> grep seed
> U Hashtable<oop, (unsigned
> 0000000000848890 t Hashtable<oop, (unsigned
> Making these improvements allows for CHECK_UNHANDLED_OOPS to be
> defined in all fastdebug builds across the supported platforms.
> Solaris (12u1 & 12u3 C++ compilers),
> MacOS (llvm-g++ & clang++ compilers),
> Linux (g++ v4.4.3 & g++ v4.7.3),
> JTREG on MacOS,
> vm.quick.testlist on LInux,
> nsk.regression.testlist, nsk.stress.testlist on LInux,
> JCK vm on Windows
> Thank you, Lois
More information about the hotspot-dev