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