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

[Lois Foltan]

Thank you Ron for your review!
Lois

On 9/24/2013 5:13 PM, Ron Durbin wrote:
>
> I am not an official open source code reviewer, but I did review the 
> code and it looks good.
>
> Ron
>
> *From:*Lois Foltan
> *Sent:* Thursday, September 19, 2013 9:11 AM
> *To:* hotspot-runtime-dev at openjdk.java.net; build-dev at openjdk.java.net
> *Subject:* RFR (L) JDK-7195622: CheckUnhandledOops has limited 
> usefulness now
>
> Please review the following fix:
>
>     Webrev:
> http://cr.openjdk.java.net/~hseigel/bug_jdk7195622.0/ 
> <http://cr.openjdk.java.net/%7Ehseigel/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
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/attachments/20130924/98248466/attachment-0001.html