[RFC/JVMTI] GetMethodDeclaringClass

[RFC/JVMTI] GetMethodDeclaringClass

[Keith Seitz]

Hi,

Okay, well, I hate to be indecisive, but I've spent a lot of time being 
really wishy-washy on this, so I'm leaving it up to the collective 
wisdom of the maintainers and other developers. :-)

JVMTI has a function called GetMethodDeclaringClass which is a real 
pain. Unlike JDWP and JNI, JVMTI does *not* pass class IDs and method 
IDs together. JVMTI simply passes around the jmethodID. As a result, we 
have the evil known as GetMethodDeclaringClass, which is going to be 
required for JDWP (and probably any other real-world JVMTI agent).

So somehow we must maintain a mapping of jmethodIDs to their 
corresponding classes. I've played around with this quite a bit 
recently, and I've come to two solutions for this problem, both with 
pros and cons. [I have hacked both these solutions together; rough 
patches on request.]

It is up to you, gentle reader, to pick one of these solutions or offer 
another up for review. [I should mention a small aside: both JDWP and 
JVMTI also have a function to get all the classes in the VM for which 
this could also be used.]

1) the simple method
ClassLoaders already contain a list of all the classes they've loaded. 
So by adding some code to java.lang.ClassLoader, we can keep track of 
all the ClassLoaders in the system. We can then do several things to 
search through those loaded classes: loop through the classes (dog 
slow), sort the loaded classes and then search (I'm guessing not 
feasible for a large number of classes), etc.

2) maintain yet another list of loaded classes
Yes, I hate to say it, but this is on the table. If it isn't bad enough 
that java.lang.ClassLoader and the stacktracing code maintain lists of 
loaded classes, I'm proposing adding one more, which builds on the 
stacktracing code's _Jv_{Pop,Push}Class. I've implemented this with a 
TreeSet and a custom Comparator. [It maintains a list of classes, not 
jmethodIDs!]

3) ???

Any comments or other ideas?

Keith

Re: [RFC/JVMTI] GetMethodDeclaringClass

[Andrew Haley]

Keith Seitz writes:
 > 
 > Okay, well, I hate to be indecisive, but I've spent a lot of time being 
 > really wishy-washy on this, so I'm leaving it up to the collective 
 > wisdom of the maintainers and other developers. :-)
 > 
 > JVMTI has a function called GetMethodDeclaringClass which is a real 
 > pain. Unlike JDWP and JNI, JVMTI does *not* pass class IDs and method 
 > IDs together. JVMTI simply passes around the jmethodID. As a result, we 
 > have the evil known as GetMethodDeclaringClass, which is going to be 
 > required for JDWP (and probably any other real-world JVMTI agent).
 > 
 > So somehow we must maintain a mapping of jmethodIDs to their 
 > corresponding classes. I've played around with this quite a bit 
 > recently, and I've come to two solutions for this problem, both with 
 > pros and cons. [I have hacked both these solutions together; rough 
 > patches on request.]
 > 
 > It is up to you, gentle reader, to pick one of these solutions or offer 
 > another up for review. [I should mention a small aside: both JDWP and 
 > JVMTI also have a function to get all the classes in the VM for which 
 > this could also be used.]
 > 
 > 1) the simple method
 > ClassLoaders already contain a list of all the classes they've loaded. 
 > So by adding some code to java.lang.ClassLoader, we can keep track of 
 > all the ClassLoaders in the system. We can then do several things to 
 > search through those loaded classes: loop through the classes (dog 
 > slow), sort the loaded classes and then search (I'm guessing not 
 > feasible for a large number of classes), etc.
 > 
 > 2) maintain yet another list of loaded classes
 > Yes, I hate to say it, but this is on the table. If it isn't bad enough 
 > that java.lang.ClassLoader and the stacktracing code maintain lists of 
 > loaded classes, I'm proposing adding one more, which builds on the 
 > stacktracing code's _Jv_{Pop,Push}Class. I've implemented this with a 
 > TreeSet and a custom Comparator. [It maintains a list of classes, not 
 > jmethodIDs!]
 > 
 > 3) ???
 > 
 > Any comments or other ideas?

What is a jmethodID?  How is it created?\

Andrew.

Re: [RFC/JVMTI] GetMethodDeclaringClass

[Andrew Haley]

Andrew Haley writes:
 > Keith Seitz writes:
 >  > 
 >  > Okay, well, I hate to be indecisive, but I've spent a lot of time being 
 >  > really wishy-washy on this, so I'm leaving it up to the collective 
 >  > wisdom of the maintainers and other developers. :-)
 >  > 
 >  > JVMTI has a function called GetMethodDeclaringClass which is a real 
 >  > pain. Unlike JDWP and JNI, JVMTI does *not* pass class IDs and method 
 >  > IDs together. JVMTI simply passes around the jmethodID. As a result, we 
 >  > have the evil known as GetMethodDeclaringClass, which is going to be 
 >  > required for JDWP (and probably any other real-world JVMTI agent).
 >  > 
 >  > So somehow we must maintain a mapping of jmethodIDs to their 
 >  > corresponding classes. I've played around with this quite a bit 
 >  > recently, and I've come to two solutions for this problem, both with 
 >  > pros and cons. [I have hacked both these solutions together; rough 
 >  > patches on request.]
 >  > 
 >  > It is up to you, gentle reader, to pick one of these solutions or offer 
 >  > another up for review. [I should mention a small aside: both JDWP and 
 >  > JVMTI also have a function to get all the classes in the VM for which 
 >  > this could also be used.]
 >  > 
 >  > 1) the simple method
 >  > ClassLoaders already contain a list of all the classes they've loaded. 
 >  > So by adding some code to java.lang.ClassLoader, we can keep track of 
 >  > all the ClassLoaders in the system. We can then do several things to 
 >  > search through those loaded classes: loop through the classes (dog 
 >  > slow), sort the loaded classes and then search (I'm guessing not 
 >  > feasible for a large number of classes), etc.
 >  > 
 >  > 2) maintain yet another list of loaded classes
 >  > Yes, I hate to say it, but this is on the table. If it isn't bad enough 
 >  > that java.lang.ClassLoader and the stacktracing code maintain lists of 
 >  > loaded classes, I'm proposing adding one more, which builds on the 
 >  > stacktracing code's _Jv_{Pop,Push}Class. I've implemented this with a 
 >  > TreeSet and a custom Comparator. [It maintains a list of classes, not 
 >  > jmethodIDs!]
 >  > 
 >  > 3) ???
 >  > 
 >  > Any comments or other ideas?
 > 
 > What is a jmethodID?  How is it created?\

Ah, sorry.  I thought you meant something internal to JVMTI, but I
guess you mean libgcj's jmethodID.

AFAICS we just need to make sure that all methods for interpreted
classes are pushed with _Jv_PushClass.  Then to get your class,

 class = ncodeMap->get (jmethodID->ncode);

Andrew.

Re: [RFC/JVMTI] GetMethodDeclaringClass

[Keith Seitz]

Andrew Haley wrote:

> AFAICS we just need to make sure that all methods for interpreted
> classes are pushed with _Jv_PushClass.  Then to get your class,
> 
>  class = ncodeMap->get (jmethodID->ncode);

I thought about doing that at one time, but I was (too?) worried about 
an invalid jmethodID wreaking havoc. Is there a way to guard against 
something like this or otherwise validate the jmethodID before 
dereferencing it?

Perhaps I'm being too paranoid?

Keith

Re: [RFC/JVMTI] GetMethodDeclaringClass

[Andrew Haley]

Keith Seitz writes:
 > Andrew Haley wrote:
 > 
 > > AFAICS we just need to make sure that all methods for interpreted
 > > classes are pushed with _Jv_PushClass.  Then to get your class,
 > > 
 > >  class = ncodeMap->get (jmethodID->ncode);
 > 
 > I thought about doing that at one time, but I was (too?) worried about 
 > an invalid jmethodID wreaking havoc.

You want to know if a jmethodID really is valid?  That it really is a
pointer to a _Jv_Method?  

 > Is there a way to guard against something like this or otherwise
 > validate the jmethodID before dereferencing it?

The only way I can think of is by keeping a cache of them.

 > Perhaps I'm being too paranoid?

Sure.  I guess you want a debugger to be robust even when the runtime
is FUBAR, but this seems a little extreme.

Andrew.

Re: [RFC/JVMTI] GetMethodDeclaringClass

[Keith Seitz]

Andrew Haley wrote:

>  > Perhaps I'm being too paranoid?
> 
> Sure.  I guess you want a debugger to be robust even when the runtime
> is FUBAR, but this seems a little extreme.

Okay, one last question... I have implemented the changes as you 
suggest, and (of course) they are much, much, much, (much, much, ...) 
simpler.

 > AFAICS we just need to make sure that all methods for interpreted
 > classes are pushed with _Jv_PushClass.  [snip]

In May 2006, a patch was committed to explicitly exclud interpreted 
class methods from showing up in ncodeMap:

2006-05-31  Alan Modra  <amodra@bigpond.net.au>

         * stacktrace.cc (_Jv_StackTrace::UpdateNCodeMap): Don't add
         interpreted classes.

Something about powerpc64-linux stacktrace failures? Original discussion 
here:

http://gcc.gnu.org/ml/java-patches/2006-q2/msg00261.html

Before I submit this, is this going to be a problem? I don't have access 
to a ppc64-linux box (that I know of, at least).

Keith

Re: [RFC/JVMTI] GetMethodDeclaringClass

[Andrew Haley]

Keith Seitz writes:
 > Andrew Haley wrote:
 > 
 > >  > Perhaps I'm being too paranoid?
 > > 
 > > Sure.  I guess you want a debugger to be robust even when the runtime
 > > is FUBAR, but this seems a little extreme.
 > 
 > Okay, one last question... I have implemented the changes as you 
 > suggest, and (of course) they are much, much, much, (much, much, ...) 
 > simpler.
 > 
 >  > AFAICS we just need to make sure that all methods for interpreted
 >  > classes are pushed with _Jv_PushClass.  [snip]
 > 
 > In May 2006, a patch was committed to explicitly exclud interpreted 
 > class methods from showing up in ncodeMap:
 > 
 > 2006-05-31  Alan Modra  <amodra@bigpond.net.au>
 > 
 >          * stacktrace.cc (_Jv_StackTrace::UpdateNCodeMap): Don't add
 >          interpreted classes.

Well spotted.

 > Something about powerpc64-linux stacktrace failures? Original discussion 
 > here:
 > 
 > http://gcc.gnu.org/ml/java-patches/2006-q2/msg00261.html
 > 
 > Before I submit this, is this going to be a problem? I don't have access 
 > to a ppc64-linux box (that I know of, at least).

I understand.  Let's not re-introuce this bug.  We could push these
entries into a separate map or maybe when (looking them up during a
stacktrace) check that the result isn't an interpreted class.  Either
would work, and neither is a huge additional overhead.

Andrew.

Re: [RFC/JVMTI] GetMethodDeclaringClass

[Keith Seitz]

[-- Attachment #1: Type: text/plain, Size: 1368 bytes --]

Andrew Haley wrote:
> I understand.  Let's not re-introuce this bug.  We could push these
> entries into a separate map or maybe when (looking them up during a
> stacktrace) check that the result isn't an interpreted class.  Either
> would work, and neither is a huge additional overhead.

Okay, I've chosen the latter approach (check if the class is interpreted 
when doing backtraces).

How does this look? [It would be really appreciated if someone with 
access to ppc64-linux or somesuch could double-check for a failure in 
the stack tracing tests in lang.exp.]

Keith

ChangeLog
2006-10-23  Keith Seitz  <keiths@redhat.com>

         * include/java-stack.h (ncodeMap): Declare.
         (_Jv_StackTrace): Make _Jv_GetMethodDeclaringClass friend.
         * java/lang/Class.h (_Jv_GetMethodDeclaringClass): Declare.
         * java/lang/natClass.cc (_Jv_GetMethodDeclaringClass): New
         function.
         * stacktrace.cc (ncodeMap): Redefine from file global to global
         for class _Jv_StackTrace.
         (_Jv_StackTrace::UpdateNCodeMap): Add interpreted classes, too,
         so that _Jv_GetMethodDeclaringClass can find them all.
         (_Jv_StackTrace::ClassForFrame): Exclude interpreted classes.
         * jvmti.cc (_Jv_JVMTI_GetMethodDeclaringClass): New function.
         (_Jv_JVMTI_Interface): Define GetMethodDeclaringClass function.

[-- Attachment #2: getmethoddeclaringclass.patch --]
[-- Type: text/x-patch, Size: 5264 bytes --]

Index: include/java-stack.h
===================================================================
--- include/java-stack.h	(revision 117594)
+++ include/java-stack.h	(working copy)
@@ -23,6 +23,7 @@
 #include <java/lang/StackTraceElement.h>
 #include <java/lang/Throwable.h>
 #include <java/lang/Thread.h>
+#include <java/util/IdentityHashMap.h>
 
 #include <gnu/gcj/runtime/NameFinder.h>
 
@@ -102,6 +103,7 @@
   int length;
   _Jv_StackFrame frames[];
 
+  static java::util::IdentityHashMap *ncodeMap;
   static void UpdateNCodeMap ();
   static jclass ClassForFrame (_Jv_StackFrame *frame);
   static void FillInFrameInfo (_Jv_StackFrame *frame);
@@ -126,7 +128,8 @@
   static JArray<jclass> *GetClassContext (jclass checkClass);
   static ClassLoader *GetFirstNonSystemClassLoader (void);
   static jobjectArray GetAccessControlStack ();
-  
+
+  friend jclass _Jv_GetMethodDeclaringClass (jmethodID);
 };
 
 // Information about a given address.
Index: java/lang/Class.h
===================================================================
--- java/lang/Class.h	(revision 117333)
+++ java/lang/Class.h	(working copy)
@@ -289,6 +289,10 @@
 
 void _Jv_sharedlib_register_hook (jclass klass);
 
+/* Find the class that defines the given method. Returns NULL
+   if it cannot be found. Searches both interpreted and native
+   classes. */
+jclass _Jv_GetMethodDeclaringClass (jmethodID method);
 
 class java::lang::Class : public java::lang::Object
 {
Index: java/lang/natClass.cc
===================================================================
--- java/lang/natClass.cc	(revision 117333)
+++ java/lang/natClass.cc	(working copy)
@@ -1259,3 +1259,11 @@
   return NULL;
 }
 #endif
+
+jclass
+_Jv_GetMethodDeclaringClass (jmethodID method)
+{
+  _Jv_StackTrace::UpdateNCodeMap ();
+  jobject obj = reinterpret_cast<jobject> (method->ncode);
+  return reinterpret_cast<jclass> (_Jv_StackTrace::ncodeMap->get (obj));
+}
Index: stacktrace.cc
===================================================================
--- stacktrace.cc	(revision 117333)
+++ stacktrace.cc	(working copy)
@@ -23,7 +23,6 @@
 #include <java/lang/Long.h>
 #include <java/security/AccessController.h>
 #include <java/util/ArrayList.h>
-#include <java/util/IdentityHashMap.h>
 #include <gnu/classpath/jdwp/Jdwp.h>
 #include <gnu/java/lang/MainThread.h>
 #include <gnu/gcj/runtime/NameFinder.h>
@@ -41,7 +40,7 @@
 // NOTE: Currently this Map contradicts class GC for native classes. This map
 // (and the "new class stack") will need to use WeakReferences in order to 
 // enable native class GC.
-static java::util::IdentityHashMap *ncodeMap;
+java::util::IdentityHashMap *_Jv_StackTrace::ncodeMap;
 
 // Check the "class stack" for any classes initialized since we were last 
 // called, and add them to ncodeMap.
@@ -56,21 +55,20 @@
   
   jclass klass;
   while ((klass = _Jv_PopClass ()))
-    if (!_Jv_IsInterpretedClass (klass))
-      {
-	//printf ("got %s\n", klass->name->data);
-	for (int i = 0; i < klass->method_count; i++)
-	  {
-	    _Jv_Method *method = &klass->methods[i];
-	    void *ncode = method->ncode;
-	    // Add non-abstract methods to ncodeMap.
-	    if (ncode)
-	      {
-		ncode = UNWRAP_FUNCTION_DESCRIPTOR (ncode);
-		ncodeMap->put ((java::lang::Object *) ncode, klass);
-	      }
-	  }
-      }
+    {
+      //printf ("got %s\n", klass->name->data);
+      for (int i = 0; i < klass->method_count; i++)
+	{
+	  _Jv_Method *method = &klass->methods[i];
+	  void *ncode = method->ncode;
+	  // Add non-abstract methods to ncodeMap.
+	  if (ncode)
+	    {
+	      ncode = UNWRAP_FUNCTION_DESCRIPTOR (ncode);
+	      ncodeMap->put ((java::lang::Object *) ncode, klass);
+	    }
+	}
+    }
 }
 
 // Given a native frame, return the class which this code belongs 
@@ -85,8 +83,14 @@
 
   // look it up in ncodeMap
   if (frame->start_ip)
-    klass = (jclass) ncodeMap->get ((jobject) frame->start_ip);
+    {
+      klass = (jclass) ncodeMap->get ((jobject) frame->start_ip);
 
+      // Exclude interpreted classes
+      if (klass != NULL && _Jv_IsInterpretedClass (klass))
+	klass = NULL;
+    }
+
   return klass;
 }
 
Index: jvmti.cc
===================================================================
--- jvmti.cc	(revision 117670)
+++ jvmti.cc	(working copy)
@@ -466,6 +466,24 @@
 }
 
 static jvmtiError JNICALL
+_Jv_JVMTI_GetMethodDeclaringClass (MAYBE_UNUSED jvmtiEnv *env,
+				   jmethodID method,
+				   jclass *declaring_class_ptr)
+{
+  REQUIRE_PHASE (env, JVMTI_PHASE_LIVE);
+  NULL_CHECK (declaring_class_ptr);
+
+  jclass klass = _Jv_GetMethodDeclaringClass (method);
+  if (klass != NULL)
+    {
+      *declaring_class_ptr = klass;
+      return JVMTI_ERROR_NONE;
+    }
+
+  return JVMTI_ERROR_INVALID_METHODID;
+}
+
+static jvmtiError JNICALL
 _Jv_JVMTI_GetClassLoaderClasses (MAYBE_UNUSED jvmtiEnv *env,
 				 jobject init_loader,
 				 jint *count_ptr,
@@ -1287,7 +1305,7 @@
   _Jv_JVMTI_GetFieldModifiers,	// GetFieldModifiers
   _Jv_JVMTI_IsFieldSynthetic,	// IsFieldSynthetic
   UNIMPLEMENTED,		// GetMethodName
-  UNIMPLEMENTED,		// GetMethodDeclaringClass
+  _Jv_JVMTI_GetMethodDeclaringClass,  // GetMethodDeclaringClass
   _Jv_JVMTI_GetMethodModifiers,	// GetMethodModifers
   RESERVED,			// reserved67
   UNIMPLEMENTED,		// GetMaxLocals

Re: [RFC/JVMTI] GetMethodDeclaringClass

[Andrew Haley]

Keith Seitz writes:
 > Andrew Haley wrote:
 > > I understand.  Let's not re-introuce this bug.  We could push these
 > > entries into a separate map or maybe when (looking them up during a
 > > stacktrace) check that the result isn't an interpreted class.  Either
 > > would work, and neither is a huge additional overhead.
 > 
 > Okay, I've chosen the latter approach (check if the class is interpreted 
 > when doing backtraces).
 > 
 > How does this look?

I can't fault it.

 > [It would be really appreciated if someone with access to
 > ppc64-linux or somesuch could double-check for a failure in the
 > stack tracing tests in lang.exp.]

I suggest you ask Andrew Overholt for access to his ppc64 box.

Andrew.

Re: [RFC/JVMTI] GetMethodDeclaringClass

[Keith Seitz]

Andrew Haley wrote:
>  > [It would be really appreciated if someone with access to
>  > ppc64-linux or somesuch could double-check for a failure in the
>  > stack tracing tests in lang.exp.]
> 
> I suggest you ask Andrew Overholt for access to his ppc64 box.

Okay, I finally found a machine to test this on. I tried three test runs:

1) Virgin SVN HEAD
2) #1 + my patch
3) #2 - the "filter out interpreted classes" (to make sure that the bug 
was tested)

#1 & #2 were the same. #3 showed backtrace problems. So I guess we can 
conclude that my patch does not cause any regressions.

Keith

Re: [RFC/JVMTI] GetMethodDeclaringClass

[Andrew Haley]

Keith Seitz writes:
 > Andrew Haley wrote:
 > >  > [It would be really appreciated if someone with access to
 > >  > ppc64-linux or somesuch could double-check for a failure in the
 > >  > stack tracing tests in lang.exp.]
 > > 
 > > I suggest you ask Andrew Overholt for access to his ppc64 box.
 > 
 > Okay, I finally found a machine to test this on. I tried three test runs:
 > 
 > 1) Virgin SVN HEAD
 > 2) #1 + my patch
 > 3) #2 - the "filter out interpreted classes" (to make sure that the bug 
 > was tested)
 > 
 > #1 & #2 were the same. #3 showed backtrace problems. So I guess we can 
 > conclude that my patch does not cause any regressions.

OK, excellent.  Please commit.

Andrew.

Re: [RFC/JVMTI] GetMethodDeclaringClass

[Keith Seitz]

Andrew Haley wrote:

> OK, excellent.  Please commit.

Done. Thank you for all your help with this.

Keith

Re: [RFC/JVMTI] GetMethodDeclaringClass

[Per Bothner]

Keith Seitz wrote:
> So somehow we must maintain a mapping of jmethodIDs to their 
> corresponding classes.

The question is: what does a jmethodID map to?  Historically, it maps
to a (_Jv_Method*), and you can't map that to a class.  But perhaps a
jmethodID should be the same as a java::lang::reflect::Method* -
does does have a reference to its declaring class.

Actually, we want to go further: Have Jv_Method be the same
as java::lang::reflect::Method.  That assumes the compiler creates
a compressed data structure, which is lazily unpacked when needed,
as previously discussed.
-- 
	--Per Bothner
per@bothner.com   http://per.bothner.com/