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

[Lois Foltan]

On 9/25/2013 3:06 PM, Christian Thalinger wrote:
> This looks good.  Thank you for changing it to template methods instead of using macros.
Great, thank you for taking the time to look at this given your week at 
JavaOne.
Lois

>
> 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
>>
>>
>>
>>
>>
>>
>>
>>