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

Erik Helin erik.helin at oracle.com
Fri Oct 18 03:56:44 PDT 2013 

Hi David and Serguei,

thanks for having a look at this change!

On 2013-10-18, David Holmes wrote:
> On 18/10/2013 5:58 PM, serguei.spitsyn at oracle.com wrote:
> >On 10/18/13 12:24 AM, serguei.spitsyn at oracle.com wrote:
> >>On 10/17/13 11:03 PM, David Holmes wrote:
> >>>On 18/10/2013 3:49 PM, serguei.spitsyn at oracle.com wrote:
> >>>>Hi Erik,
> >>>>
> >>>>The fix looks good in general.
> >>>>
> >>>>But one thing is confusing in the fix.
> >>>>Why do you keep both _class_loader and _class_loader_handle in the
> >>>>JvmtiBreakpoint class?
> >>>
> >>>Even more confusing to me is the fact the neither of these seem to
> >>>actually be used anywhere ???
> >>
> >>Nice catch, David.
> >>I do not see too any of them is really used.
> >>Is it a leftover after the permgen elimination?
> >
> >Maybe this is a rush judging.
> >It depends on the closure->do_oop() that is used for traversing
> >I thought that the KeepAliveClosure is used below (basing on the comment).
> >
> >class JvmtiBreakpoint : public GrowableElement {
> >   . . .
> >   void oops_do(OopClosure* f)     {
> >     // Mark the method loader as live
> >     f->do_oop(&_class_loader);
> >   }
> >
> >This oops_do() is not needed if we have handle instead of oop.
> 
> Ah! Maybe the only purpose of keeping the class_loader (whether oop
> or Handle) is that it is kept alive outside of its normal lifecycle.
> 
> But still we should only need the Handle or the Oop, not both. And
> if there is no oop we should not need an oops_do.

I will try to explain the problem, the current implementation and then
my change.

The problem
-----------

The problem is that JvmtiEnv::SetBreakpoint and
JvmtiEnv::ClearBreakpoint are passed a Method*. We must ensure that the
class for this Method is kept alive by the GC. We do this by informing
the GC that it must visit the class loader for the Method's class when
marking. This can be done in two ways:
- Putting a Handle on the stack containing an oop to the class loader
- Have an oops_do method that we ensure will be called by the GC during
  marking

A third option is to make sure (by inspecting the code) that no GC can
occur that might cause problems, but this is a very brittle solution
once a new programmer comes along and adds/removes code.

The current implementation
--------------------------

In JvmtiEnv::SetBreakpoint and JvmtiEnv::ClearBreakpoint, we create a
JvmtiBreakpoint on the stack. This JvmtiBreakpoint contains an
"unhandled" oop to the class loader of the Method's class.

A pointer to the JvmtiBreakpoint allocated on the stack will be passed
to VM_ChangeBreakpoint via JvmtiBreakpoints::set/clear.
In the doit() method of VM_ChangeBreakpoint,
JvmtiBreakpoints::set_at_safepoint will be called. A new JvmtiBreakpoint
will be allocated on the heap by JvmtiBreakpoints::set_at_safepoint.

The JvmtiBreakpoint on the heap will contain the same oop as the one in
the JvmtiBreakpoint allocated on the stack earlier. The oop in the
JvmtiBreakpoint allocated on the heap will be found by the GC, because
VM_ChangeBreakpoint has an oops_do method (see first email for how this
oops_do method is called).

Once the VM_ChangeBreakpoint operation is done, then the JvmtiBreakpoint
allocated on the heap will either be cleared due to a clear operation
(and we do not care about the oop any longer) or it will be placed in
JvmtiBreakpoints. JvmtiBreakpoints also has an oops_do which ensures
that the oop now will be found by the GC.

We will now pop the call stack and return to JvmtiEnv::SetBreakpoint or
JvmtiEnv::ClearBreakpoint. No code in JvmtiEnv::SetBreakpoint or
JvmtiEnv::ClearBreakpoint is touching the JvmtiBreakpoint after the
VM_ChangeBreakpoint operation has been run.

Furthermore, no GC can today occur before the call to VMThread::execute
in JvmtiBreakpoints::set/clear.

This means that the current implementation is safe, but understanding
this is quite tricky.

The change
----------

My proposed solution was the following:
- When a JvmtiBreakpoint is allocated on the stack, place an oop to the
  Method's class loader in a Handle.
- When a JvmtiBreakpoint is allocated on the heap, just store the oop to
  Method's class lodear in a field. The JvmtiBreakpoint will be placed
  in _jvmti_breakpoints which will be visited by the GC, which makes the
  GC call JvmtiBreakpoint::oops_do.

Of course, this means that that if a GC occurs before the
JvmtiBreakpoint in constructed in JvmtiEnv::SetBreakpoint or
JvmtiEnv::ClearBreakpoint, the Method's class might be garbage
collected. Ideally, we should wrap the Method* in jvmti_SetBreakpoint
and jvmti_ClearBreakpoint in jvmtiEnter.cpp (a cpp file generated from
an XML file via an XSL tranformation).

As icing on the cake, if someone redefines the method that we are
receiving a pointer to, then this code will probably not work at all. I
believe (I am not sure) that this should be solved by having a
MethodHandle wrapping the Method*.

This change is just a first step towards safer code.

I've gotten feedback internally that it is hard to understand the
new code. I will try to see if I can update the change to make this
clearer.

Thanks for getting all the way to the end of this email :)

Erik

> David
> 
> >
> >>
> >>>
> >>>But if we have the Handle then the oop is redundant AFAICS.
> >>
> >>Right.
> >>The issue is that the oop version is used in the oops_do.
> >>Not sure if we can get rid of oops_do.
> >>It may have an empty body though.
> >
> >Getting rid of the oops_do will require more cleanup that needs to be
> >accurate.
> >
> >
> >Thanks,
> >Serguei
> >
> >>
> >>
> >>Thanks,
> >>Serguei
> >>
> >>>
> >>>David
> >>>
> >>>>Also, you need to run the nsk.jdi.testlist and nsk.jdwp.testlist test
> >>>>suites as well.
> >>>>
> >>>>Thanks,
> >>>>Serguei
> >>>>
> >>>>
> >>>>On 10/17/13 2:28 PM, Erik Helin wrote:
> >>>>>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
> >>>>>>>
> >>>>
> >>
> >

More information about the hotspot-dev mailing list