On Sep 19, 2013, at 4:22 PM, Lois Foltan <lois.fol...@oracle.com> wrote:
> > On 9/19/2013 6:27 PM, Christian Thalinger wrote: >> 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 in the case of CAST_TO_OOP, where the type to cast value to > is known, an oop. In the case of CAST_FROM_OOP, it wouldn't work as nicely > without having to introduce many different inline methods based on the > different types that an oop must be cast to. How about a template method? > > Lois >> >>> 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 >>>>> >>>>> >>>>> >>>>> >>>>> >>>>> >>>>> >>>>> >>>>> >>>>> >
