Hi Mikael and Coleen,
thanks for your reviews!
On 2013-10-16, Mikael Gerdin wrote:
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);
Agree, I've updated the patch. A new webrev is located at:
http://cr.openjdk.java.net/~ehelin/8025834/webrev.01/
On 2013-10-16, Mikael Gerdin wrote:
jvmtiEnv.cpp:
Have you verified that the generated JVMTI entry point contains a
ResourceMark or is it just not needed?
- ResourceMark rm;
+ HandleMark hm;
The JVMTI entry point does not contain a ResourceMark. However, I have
verified that a ResourceMark is not needed for jvmtiEnv::SetBreakpoint
nor for jvmtiEnv::ClearBreapoint. The only codes that needs a
ResourceMark is JvmtiBreakpoints::prints, but that method already has a
ResourceMark.
On 2013-10-16, Coleen Phillimore wrote:
Did you run the nsk.jvmti.testlist tests too though?
No, I had not run the nsk.jvmti.testlist test, but I have now :)
I run both with and without the patch on the latest hsx/hotspot-gc. I
also run with and without -XX:+CheckUnhandledOops. The results were all
the same: 598 passed an 11 failed (the same tests for all combinations).
So, the patch does not introduce any regressions for this test suite.
Thanks,
Erik
On 2013-10-16, Mikael Gerdin wrote:
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
