Erik,
(it's not necessary to cross-post between hotspot-dev and
hotspot-gc-dev, so I removed hotspot-gc from the CC list)
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/
jvmtiImpl.hpp:
Since clone() uses unhandled oops, and is only supposed to be used by
the VM operation, would it make sense to
assert(SafepointSynchronize::is_at_safepoint())?
196 GrowableElement *clone() {
197 return new JvmtiBreakpoint(_method, _bci, _class_loader_handle);
jvmtiImpl.cpp:
No comments.
jvmtiEnv.cpp:
Have you verified that the generated JVMTI entry point contains a
ResourceMark or is it just not needed?
- ResourceMark rm;
+ HandleMark hm;
Otherwise the code change looks good.
One thing that you didn't describe here, but which was related to the
bug (which we discussed) was the fact that the old code tried to "do the
right thing" WRT CheckUnhandledOops, but it incorrectly added a Method*:
thread->allow_unhandled_oop((oop*)&_method);
We should take care to find other such places where we try to put a
non-oop in allow_unhandled_oop(), perhaps checking is_oop_or_null in the
unhandled oops code.
/Mikael
Testing:
- JPRT
- The four tests that were failing are now passing
Thanks,
Erik
