FR (L) JDK-7195622 (round 2): CheckUnhandledOops has limited usefulness now

[Christian Thalinger]

This looks good.  Thank you for changing it to template methods instead of using macros.

On Sep 23, 2013, at 2:17 PM, Lois Foltan <lois.foltan at oracle.com> wrote:

> 
> Please review the following updated fix which has removed the CAST_*_OOP macro
> implementation in favor of two inlined template methods, cast_to_oop() & cast_from_oop().
> 
>    Webrev:
>     http://cr.openjdk.java.net/~hseigel/bug_jdk7195622.1/
> 
> 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
>                inlined template methods, cast_*_oop(), were introduced in oops/oopsHierarchy.hpp
> 
>            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
> 
> 
> 
> 
> 
> 
> 
>