I am not an official open source code reviewer, but I did review the code and it looks good.
Ron From: Lois Foltan Sent: Thursday, September 19, 2013 9:11 AM To: hotspot-runtime-...@openjdk.java.net; build-dev@openjdk.java.net Subject: RFR (L) JDK-7195622: CheckUnhandledOops has limited usefulness now Please review the following fix: Webrev: HYPERLINK "http://cr.openjdk.java.net/%7Ehseigel/bug_jdk7195622.0/"http://cr.openjdk.java.net/~hseigel/bug_jdk7195622.0/ Bug: JDK8 b44 hotspot:src/share/vm/oops/klass.hpp: Error:Initializing const volatile oop& requires ... & CheckUnhandledOops has limited usefulness now bug links at: https://bugs.openjdk.java.net/browse/JDK-7180556 https://bugs.openjdk.java.net/browse/JDK-7195622 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 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. 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 short)2304>::_seed 0000000000848890 t Hashtable<oop, (unsigned short)256>::seed() ... Making these improvements allows for CHECK_UNHANDLED_OOPS to be defined in all fastdebug builds across the supported platforms. Builds: Solaris (12u1 & 12u3 C++ compilers), MacOS (llvm-g++ & clang++ compilers), Linux (g++ v4.4.3 & g++ v4.7.3), VS2010 Tests: JTREG on MacOS, vm.quick.testlist on LInux, nsk.regression.testlist, nsk.stress.testlist on LInux, JCK vm on Windows Thank you, Lois
