On Sep 19, 2013, at 6:06 PM, Lois Foltan <lois.fol...@oracle.com> wrote:
>
> On 9/19/2013 7:25 PM, Christian Thalinger wrote:
>> On Sep 19, 2013, at 4:22 PM, Lois Foltan <lois.fol...@oracle.com> wrote:
>>
>>> On 9/19/2013 6:27 PM, Christian Thalinger wrote:
>>>> On Sep 19, 2013, at 3:19 PM, Lois Foltan <lois.fol...@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.
>
> 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.fol...@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
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>
