On 9/19/2013 9:34 PM, Christian Thalinger wrote:

On Sep 19, 2013, at 6:06 PM, Lois Foltan <lois.fol...@oracle.com <mailto: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.
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.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













Reply via email to