RFR: 8025834: NPE in Parallel Scavenge with -XX:+CheckUnhandledOops (round 2)

[Erik Helin]

Hi Mikael,

thanks for reviewing! Based on internal feedback, I've updated the
comment slightly:

diff --git a/src/share/vm/prims/jvmtiImpl.cpp b/src/share/vm/prims/jvmtiImpl.cpp
--- a/src/share/vm/prims/jvmtiImpl.cpp
+++ b/src/share/vm/prims/jvmtiImpl.cpp
@@ -225,18 +225,20 @@
   _method = NULL;
   _bci    = 0;
   _class_loader = NULL;
-#ifdef CHECK_UNHANDLED_OOPS
-  // This one is always allocated with new, but check it just in case.
-  Thread *thread = Thread::current();
-  if (thread->is_in_stack((address)&_method)) {
-    thread->allow_unhandled_oop((oop*)&_method);
-  }
-#endif // CHECK_UNHANDLED_OOPS
 }

 JvmtiBreakpoint::JvmtiBreakpoint(Method* m_method, jlocation location) {
   _method        = m_method;
   _class_loader  = _method->method_holder()->class_loader_data()->class_loader();
+#ifdef CHECK_UNHANDLED_OOPS
+  // _class_loader can't be wrapped in a Handle, because JvmtiBreakpoint:s are
+  // eventually allocated on the heap.
+  //
+  // The code handling JvmtiBreakpoint:s allocated on the stack can't be
+  // interrupted by a GC until _class_loader is reachable by the GC via the
+  // oops_do method.
+  Thread::current()->allow_unhandled_oop(&_class_loader);
+#endif // CHECK_UNHANDLED_OOPS
   assert(_method != NULL, "_method != NULL");
   _bci           = (int) location;
   assert(_bci >= 0, "_bci >= 0");

What do you think?

Thanks,
Erik

On 2013-10-21, Mikael Gerdin wrote:
> Erik,
> 
> On Monday 21 October 2013 11.26.51 Erik Helin wrote:
> > Hi all,
> > 
> > this is the second version of a patch for JDK-8025834.
> > 
> > Background:
> > 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. The
> > VM_ChangeBreakpoint class has an oops_do method that will visit the oop
> > in the JvmtiBreakpoint that was allocated on the stack.
> > 
> > Since no GC can occur before the JvmtiBreakpoint has been passed to the
> > VM_ChangeBreakpoint (which is then passed to VMThread::execute), there
> > is no need to have a Handle for the _class_loader oop. Once the
> > VM_ChangeBreakpoint is on the VMThread's list for being executed, the
> > oop will be found via VMThread::oops_do.
> > 
> > The oop can't be changed to a Handle because JvmtiBreakpoint:s are also
> > allocated on the heap, and Handles are only used for oops allocated on
> > the stack.
> > 
> > This patch therefore registers the oop as "unhandled" to tell the
> > unhandled oop verifier to (rightfully) ignore it.
> > 
> > Webrev:
> > http://cr.openjdk.java.net/~ehelin/8025834/round-2/webrev.00/
> 
> The change looks good to me. 
> 
> /Mikael
> 
> > 
> > Bug:
> > https://bugs.openjdk.java.net/browse/JDK-8025834
> > 
> > As I explained in a previous email, there are additional issues in the
> > JvmtiEnv::SetBreakpoint and JvmtiEnv::ClearBreapoints methods:
> > - https://bugs.openjdk.java.net/browse/JDK-8026946
> > - https://bugs.openjdk.java.net/browse/JDK-8026948
> > This change does not intend to fix these issues.
> > 
> > Thanks,
> > Erik
>