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