RFR (L) JDK-7195622: CheckUnhandledOops has limited usefulness now
Lois Foltan
lois.foltan at oracle.com
Fri Sep 20 11:22:50 UTC 2013
On 9/19/2013 9:34 PM, Christian Thalinger wrote:
>
> On Sep 19, 2013, at 6:06 PM, Lois Foltan <lois.foltan at oracle.com
> <mailto:lois.foltan at oracle.com>> wrote:
>
>>
>> On 9/19/2013 7:25 PM, Christian Thalinger wrote:
>>> On Sep 19, 2013, at 4:22 PM, Lois Foltan<lois.foltan at oracle.com> wrote:
>>>
>>>> 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.
>>> How about a template method?
>> Hi Christian,
>> I had to prototype this idea, here's the implementation I came up with
>>
>> template <class T> inline oop cast_to_oop(T value) {
>> return (oop)(CHECK_UNHANDLED_OOPS_ONLY((void *))value);
>> }
>> template <class T> inline T cast_from_oop(oop& o) {
>> return (T)(CHECK_UNHANDLED_OOPS_ONLY((void*))o);
>> }
>>
>> The cast_from_oop() template method vs. the CAST_FROM_OOP() macro is
>> a wash, in that no extra copy construction was incurred. The
>> cast_to_oop() template method vs. the CAST_TO_OOP() macro was not.
>> There was one extra call to the (void *) conversion operator and an
>> extra copy construction. I believe this can be attributed to the
>> return of the oop since the temporary oop that was constructed could
>> not be returned by reference since it is a temporary, thus an extra
>> copy construction occurred to return it by value. Given the extra
>> copy construction, it is better to stick with the macros.
>
> But this is only when CHECK_UNHANDLED_OOPS is on, right? In a product
> there can't be a copy construction. If that's the case I'd say we go
> with the template method because the tiny overhead in a fastdebug
> build is negligible.
Hi Christian,
Okay, I concur. Seems like Coleen is okay with the template approach as
well. I will switch over to implement the template and send this out
for another review once I'm done.
Thanks,
Lois
>
>>
>> Thanks,
>> Lois
>>>> 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
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>
>
More information about the build-dev
mailing list