RFR: 8025834: NPE in Parallel Scavenge with -XX:+CheckUnhandledOops

[Mikael Gerdin]

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
>