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

[Stefan Karlsson]

On 09/20/2013 02:46 PM, Stefan Karlsson wrote:
> On 09/19/2013 05:13 PM, Lois Foltan wrote:
>>
>>
>> Please review the following fix:
>>     Webrev:
>> http://cr.openjdk.java.net/~hseigel/bug_jdk7195622.0/
>
> The changes looks good.

I forgot this rather benign bug:

http://cr.openjdk.java.net/~hseigel/bug_jdk7195622.0/src/share/vm/oops/klass.inline.hpp.patch

  inline bool check_klass_alignment(Klass* obj) {
-  return (intptr_t)obj % KlassAlignmentInBytes == 0;
+  return CAST_FROM_OOP(intptr_t, obj) % KlassAlignmentInBytes == 0;
  }


obj is not an oop.

thanks,
StefanK

>
> There are some changes that I don't fully agree with, but I will not 
> block the push because of them. I'm just going to mention them here 
> for the record:
>
> 1) I would personally prefer not to have the CAST_TO_OOP/CAST_FROM_OOP 
> macros and instead explicitly add (void *) casts.
>
> 2) I don't understand the need make the code harder to read by adding 
> a  SOLAR_ONLY((void*)) instead of just a plain (void*).
>
> I'm looking forward to be able to use this feature on Linux!
>
> thanks,
> StefanK
>
>>
>> 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
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>