On 9/25/2013 3:06 PM, Christian Thalinger wrote:
This looks good.  Thank you for changing it to template methods instead of 
using macros.
Great, thank you for taking the time to look at this given your week at JavaOne.
Lois


On Sep 23, 2013, at 2:17 PM, Lois Foltan <[email protected]> wrote:

Please review the following updated fix which has removed the CAST_*_OOP macro
implementation in favor of two inlined template methods, cast_to_oop() & 
cast_from_oop().

    Webrev:
     http://cr.openjdk.java.net/~hseigel/bug_jdk7195622.1/

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
                inlined template methods, cast_*_oop(), were introduced in 
oops/oopsHierarchy.hpp

            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