RFR (L) JDK-7195622: CheckUnhandledOops has limited usefulness now

[Lois Foltan]

On 9/19/2013 6:27 PM, Christian Thalinger wrote:
> On Sep 19, 2013, at 3:19 PM, Lois Foltan <lois.foltan at 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.

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.foltan at 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
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>