On 2013-10-16 18:09, Erik Helin wrote:
Hi all,
this patch fixes an issue where an oop in JvmtiBreakpoint,
JvmtiBreakpoint::_class_loader, was found by the unhandled oop detector.
Instead of registering the oop as an unhandled oop, which would have
worked, I decided to wrap the oop in a handle as long as it is on the
stack.
A JvmtiBreakpoint is created on the stack by the two methods
JvmtiEnv::SetBreakpoint and JvmtiEnv::ClearBreakpoint. This
JvmtiBreakpoint is only created to carry the Method*, jlocation and oop
to a VM operation, VM_ChangeBreakpoints. VM_ChangeBreakpoints will, when
at a safepoint, allocate a new JvmtiBreakpoint on the native heap, copy
the values from the stack allocated JvmtiBreakpoint and then place/clear the
newly alloacted JvmtiBreakpoint in
JvmtiCurrentBreakpoints::_jvmti_breakpoints.
I have updated to the code to check that the public constructor is only
used to allocate JvmtiBreakpoints on the stack (to warn a future
programmer if he/she decides to allocate one on the heap). The
class_loader oop is now wrapped in a Handle for stack allocated
JvmtiBreakpoints.
Due to the stack allocated JvmtiBreakpoint having the oop in a handle,
the oops_do method of VM_ChangeBreakpoints can be removed. This also
makes the oop in the handle safe for use after the VM_ChangeBreakpoint
operation is finished.
The unhandled oop in the JvmtiBreakpoint allocated on the heap will be
visited by the GC via jvmtiExport::oops_do ->
JvmtiCurrentBreakpoints::oops_do -> JvmtiBreakpoints::oops_do ->
GrowableCache::oops_do -> JvmtiBreakpoint::oops_do, since it is being
added to.
I've also removed some dead code to simplify the change:
- GrowableCache::insert
- JvmtiBreakpoint::copy
- JvmtiBreakpoint::lessThan
- GrowableElement::lessThan
Finally, I also formatted JvmtiEnv::ClearBreakpoint and
Jvmti::SetBreakpoint exactly the same to highlight that they share all
code except one line. Unfortunately, I was not able to remove this code
duplication in a good way.
Webrev:
http://cr.openjdk.java.net/~ehelin/8025834/webrev.00/
Not a review.
I think there's a pre-existing bug in this code. Registering the class
loader as a way to keep the Klass alive will not work for breakpoints in
Anonymous Classes (JSR292). Anonymous classes have to register the
mirror. See the code in the ClassLoaderData::is_alive:
bool ClassLoaderData::is_alive(BoolObjectClosure* is_alive_closure) const {
bool alive =
is_anonymous() ?
is_alive_closure->do_object_b(_klasses->java_mirror()) :
class_loader() == NULL ||
is_alive_closure->do_object_b(class_loader());
assert(!alive || claimed(), "must be claimed");
return alive;
}
We probably want to add a function ClassLoaderData::keep_alive_oop()
which returns the class loader for normal classes and the mirror for the
anonymous classes, and use that in this and similar situations.
StefanK
Testing:
- JPRT
- The four tests that were failing are now passing
Thanks,
Erik
