On Sep 19, 2013, at 3:19 PM, Lois Foltan <lois.fol...@oracle.com> wrote:
> > On 9/19/2013 6:09 PM, Christian Thalinger wrote: >> + #define CAST_TO_OOP(value) ((oop)(CHECK_UNHANDLED_OOPS_ONLY((void >> *))(value))) >> + #define CAST_FROM_OOP(new_type, value) >> ((new_type)(CHECK_UNHANDLED_OOPS_ONLY((void *))(value))) >> >> Could these two macros also be a method? > Hi Christian, > I assume by method you are implying methods within the oop class itself? Not necessarily. We already have a couple of inline methods is globalDefinitions.hpp. Would that work? > That would work only in the case of fastdebug builds where an oop is defined > as a class. In non-fastdebug builds, an oop is a (oopDesc *). The macros > provided a way to preserve the existing cast to & from an oop to a numerical > type in all builds, even non-fastdebug ones. > > Thanks for the initial review, > Lois > >> >> On Sep 19, 2013, at 8:13 AM, Lois Foltan <lois.fol...@oracle.com> wrote: >> >>> >>> Please review the following fix: >>> Webrev: >>> 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 >>> >>> >>> >>> >>> >>> >>> >>> >>> >>> >
