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

[Lois Foltan]

On 9/20/2013 9:16 AM, Stefan Karlsson wrote:
> 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;
>  }
Hi Stefan,
Thank you, good catch.  I will fix this and send it out for a second 
round of review with my changes to do away of the CAST* macros in favor 
of inline template methods based on feedback from Christian Thalinger.
Lois

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