resurrection: reflection-data stack traces [request 3.1.1 branch approval]

resurrection: reflection-data stack traces [request 3.1.1 branch approval]

[Adam Megacz]

I'd like to resurrect this patch; it was offered in the 11th hour
before 3.1 went out the door, which wasn't such a great time for
people to be considering patches =)

The code path modified by this patch should only be encountered if
CPlusPlusDemangler or printRawStackTrace fails (ie addr2line or
c++filt is not present). This is mainly advantageous on Win32, where
these tools are often not around.

Binary compatability is not affected since no per-instance members or
methods are added or removed.

Ok for branch and trunk?

  - a

Index: Class.h
===================================================================
RCS file: /cvs/gcc/gcc/libjava/java/lang/Class.h,v
retrieving revision 1.43
diff -u -r1.43 Class.h
--- Class.h     21 Dec 2001 19:47:50 -0000      1.43
+++ Class.h     24 Apr 2002 03:06:14 -0000
@@ -308,6 +308,7 @@
   friend void _Jv_LayoutVTableMethods (jclass klass);
   friend void _Jv_SetVTableEntries (jclass, _Jv_VTable *);
   friend void _Jv_MakeVTable (jclass);
+  friend JArray<java::lang::reflect::Method*>* _Jv_GetAllMethods();
 
   // Return array class corresponding to element type KLASS, creating it if
   // necessary.
Index: ClassLoader.java
===================================================================
RCS file: /cvs/gcc/gcc/libjava/java/lang/ClassLoader.java,v
retrieving revision 1.16
diff -u -r1.16 ClassLoader.java
--- ClassLoader.java    7 Dec 2001 23:34:12 -0000       1.16
+++ ClassLoader.java    24 Apr 2002 03:06:14 -0000
@@ -577,4 +577,8 @@
     // Default to returning null.  Derived classes implement this.
     return null;
   }
+
+  static native java.lang.reflect.Method[] getAllMethods();
+  static native long[] getAllMethodAddrs();
+
 }
Index: Throwable.java
===================================================================
RCS file: /cvs/gcc/gcc/libjava/java/lang/Throwable.java,v
retrieving revision 1.10
diff -u -r1.10 Throwable.java
--- Throwable.java      24 Feb 2001 03:52:49 -0000      1.10
+++ Throwable.java      24 Apr 2002 03:06:14 -0000
@@ -123,21 +123,64 @@
     printStackTrace (writer);
   }
 
+  private native static long longFromStackTraceBytes(byte[] stackTrace, int i);
+
   public void printStackTrace (PrintWriter wr)
   {
-    try
-      {
-       CPlusPlusDemangler cPlusPlusFilter = new CPlusPlusDemangler (wr);
-       PrintWriter writer = new PrintWriter (cPlusPlusFilter);
-       printRawStackTrace (writer);    
-       writer.close ();
-       if (cPlusPlusFilter.written == 0) // The demangler has failed...
-         printRawStackTrace (wr);
+      try
+          {
+              CPlusPlusDemangler cPlusPlusFilter = new CPlusPlusDemangler (wr);
+              PrintWriter writer = new PrintWriter (cPlusPlusFilter);
+              printRawStackTrace (writer);     
+              writer.close ();
+              if (cPlusPlusFilter.written > 0) return;
+          }
+      catch (Exception e1)
+          {
+          }
+
+      wr.println(toString());
+      if (stackTrace == null) {
+          wr.flush();
+          return;
       }
-    catch (Exception e1)
-      {
-       printRawStackTrace (wr);
+      
+      long[] allAddrs = ClassLoader.getAllMethodAddrs();
+      java.lang.reflect.Method[] meths = ClassLoader.getAllMethods();
+      
+      // FIXME: assumes little endian
+      for(int i=0; i<stackTrace.length; i++) {
+          long addr = longFromStackTraceBytes(stackTrace, i);
+          if (addr == 0) break;
+          
+          int whichMethod = -1;
+          for(int j=0; j<allAddrs.length; j++) {
+              if (allAddrs[j] <= addr &&
+                  (whichMethod == -1 || allAddrs[whichMethod] < allAddrs[j])) {
+                  whichMethod = j;
+              }
+          }
+          
+          if (whichMethod == -1) {
+              wr.println("[" + Long.toString(addr, 16) + "]  " + "??");
+              continue;
+          }
+          
+          if (meths[whichMethod].getDeclaringClass().getName().equals("gnu.gcj.runtime.FirstThread") &&
+              meths[whichMethod].getName().equals("call_main"))
+              break;
+          
+          wr.println("    [" + Long.toString(addr, 16) + "] " +
+                     meths[whichMethod].getDeclaringClass().getName() + "." +
+                     meths[whichMethod].getName() + "() " +
+                     "+" + (addr - allAddrs[whichMethod])
+                     );
+          
+          if (java.lang.Thread.class.isAssignableFrom(meths[whichMethod].getDeclaringClass()) &&
+              meths[whichMethod].getName().equals("run"))
+              break;
       }
+      wr.flush();
   }
 
   public Throwable ()
Index: natClassLoader.cc
===================================================================
RCS file: /cvs/gcc/gcc/libjava/java/lang/natClassLoader.cc,v
retrieving revision 1.47.8.1
diff -u -r1.47.8.1 natClassLoader.cc
--- natClassLoader.cc   2 Apr 2002 22:19:55 -0000       1.47.8.1
+++ natClassLoader.cc   24 Apr 2002 03:06:14 -0000
@@ -40,6 +40,10 @@
 #include <java/lang/StringBuffer.h>
 #include <java/io/Serializable.h>
 #include <java/lang/Cloneable.h>
+#include <java/lang/reflect/Method.h>
+
+#include<java/lang/reflect/Constructor.h>
+#include<gcj/method.h>
 
 // FIXME: remove these.
 #define CloneableClass java::lang::Cloneable::class$
@@ -347,6 +351,50 @@
 static jclass loaded_classes[HASH_LEN];
 
 // This is the root of a linked list of classes
+
+JArray<java::lang::reflect::Method*>*
+java::lang::ClassLoader::getAllMethods()
+{
+    return _Jv_GetAllMethods();
+}
+
+JArray<jlong>*
+java::lang::ClassLoader::getAllMethodAddrs()
+{
+    JArray<java::lang::reflect::Method*>* arr = _Jv_GetAllMethods();
+    java::lang::reflect::Method** el = (java::lang::reflect::Method**)elements(arr);
+    JArray<jlong>* ret = JvNewLongArray(arr->length);
+    jlong* retel = (jlong*)elements(ret);
+    for(int i=0; i<arr->length; i++)
+        retel[i] = (jlong)((unsigned int)(_Jv_FromReflectedMethod (el[i])->ncode));
+    return ret;
+}
+
+JArray<java::lang::reflect::Method*>*
+_Jv_GetAllMethods()
+{
+  int numMethods = 0;
+
+  for(int i=0; i<HASH_LEN; i++)
+      for(jclass c = loaded_classes[i]; c; c = c->next)
+          numMethods += c->getDeclaredMethods()->length;
+
+  JArray<java::lang::reflect::Method*>* ret =
+    (JArray<java::lang::reflect::Method*>*)
+    JvNewObjectArray(numMethods, &java::lang::reflect::Method::class$, NULL);
+
+  java::lang::reflect::Method** el = (java::lang::reflect::Method**)elements(ret);
+
+  for(int i=0; i<HASH_LEN; i++)
+      for(jclass c = loaded_classes[i]; c; c = c->next) {
+          JArray<java::lang::reflect::Method*>* methods = c->getDeclaredMethods();
+          jint len = methods->length;
+          java::lang::reflect::Method** meths = (java::lang::reflect::Method**)elements(methods);
+          for(int j=0; j<len; j++) el[--numMethods] = meths[j];
+      }
+
+  return ret;
+}
 
 ^L
 
Index: natThrowable.cc
===================================================================
RCS file: /cvs/gcc/gcc/libjava/java/lang/natThrowable.cc,v
retrieving revision 1.11
diff -u -r1.11 natThrowable.cc
--- natThrowable.cc     7 Feb 2002 19:26:06 -0000       1.11
+++ natThrowable.cc     24 Apr 2002 03:06:14 -0000
@@ -32,6 +32,7 @@
 #include <stdio.h>
 
 #include <unistd.h>
+#include <platform.h>
 
 #ifdef HAVE_EXECINFO_H
 #include <execinfo.h>
@@ -102,3 +103,18 @@
 #endif /* HAVE_BACKTRACE */
   wr->flush ();
 }
+
+// Returns the i^th call address in the stackTrace member, or 0 if i
+// is beyond the end of the trace. This has to be done in C++ because
+// the addresses in stackTrace are the same width as the platform's
+// pointers (which is unknown to Java code), and stackTrace is a
+// byte[] using the platform's endianness (which is unknown to Java
+// code).
+jlong
+java::lang::Throwable::longFromStackTraceBytes(jbyteArray stackArr, jint i)
+{
+    if (i * sizeof(void*) > stackArr->length) return 0;
+    unsigned int* stack = (unsigned int*)elements(stackArr);
+    return (jlong)stack[i];
+}
+

ping

[Adam Megacz]

Adam Megacz <gcj@lists.megacz.com> writes:
> I'd like to resurrect this patch; it was offered in the 11th hour
> before 3.1 went out the door, which wasn't such a great time for
> people to be considering patches =)

...

Could somebody please:

1) Approve this
2) Tell me what needs to change to get it approved
3) Tell me why it can never be approved

Any of the above will do. Thank you.

  - a

-- 
Sick of HTML user interfaces?
www.xwt.org

am I not cool enough for you?

[Adam Megacz]

Hrm, all I've gotten is silence. Do I smell bad or something?

  - a

Adam Megacz <gcj@lists.megacz.com> writes:
> Adam Megacz <gcj@lists.megacz.com> writes:
> > I'd like to resurrect this patch; it was offered in the 11th hour
> > before 3.1 went out the door, which wasn't such a great time for
> > people to be considering patches =)
> 
> ...
> 
> Could somebody please:
> 
> 1) Approve this
> 2) Tell me what needs to change to get it approved
> 3) Tell me why it can never be approved
> 
> Any of the above will do. Thank you.
> 
>   - a
> 
> -- 
> Sick of HTML user interfaces?
> www.xwt.org
> 

-- 
Sick of HTML user interfaces?
www.xwt.org

Re: am I not cool enough for you?

[Tom Tromey]

>>>>> "Adam" == Adam Megacz <gcj@lists.megacz.com> writes:

Adam> Hrm, all I've gotten is silence. Do I smell bad or something?

I've been out of town a lot in the last month.
I'm still not caught up.  I do still have the message, though.  I will
look at it, probably in the next couple days.

Tom

am I not cool enough for you?

[Andrew Haley]

Adam Megacz writes:
 > 
 > Hrm, all I've gotten is silence. Do I smell bad or something?

It looks like quite a nice idea, and we do need to get rid of
addr2line sooner or later.

However, "FIXME: little endian" is a bit worrying.  Is it really
endian dependent?

Also, I don't like:

+jlong
+java::lang::Throwable::longFromStackTraceBytes(jbyteArray stackArr, jint i)
+{
+    if (i * sizeof(void*) > stackArr->length) return 0;
+    unsigned int* stack = (unsigned int*)elements(stackArr);
+    return (jlong)stack[i];
+}
+

You'll get alignment faults if you do this -- you can't cast a jbyte*
to a jlong*.  printRawStackTrace copies from stackTraceBytes to a
suitably aligned array.

Andrew.

Re: am I not cool enough for you?

[Adam Megacz]

Andrew Haley <aph@cambridge.redhat.com> writes:
> However, "FIXME: little endian" is a bit worrying.  Is it really
> endian dependent?

No, actually, I fixed that FIXME and forgot to remove it -- that's
what longFromStackTraceBytes() does -- converts octet sequences into
jlongs in an endian-safe manner.


> You'll get alignment faults if you do this -- you can't cast a jbyte*
> to a jlong*.

Ah, crap, I forgot about that.


> printRawStackTrace copies from stackTraceBytes to a suitably aligned
> array.

That sounds like a much better idea. I'll reimplement and post a new
patch.

Any other things I should address?

  - a


-- 
Sick of HTML user interfaces?
www.xwt.org

Re: am I not cool enough for you?

[Jeff Sturm]

On Mon, 10 Jun 2002, Andrew Haley wrote:
> +    unsigned int* stack = (unsigned int*)elements(stackArr);
> 
> You'll get alignment faults if you do this -- you can't cast a jbyte*
> to a jlong*.

True in general, but isn't the 0th array element guaranteed to be
word-aligned in this case?

Jeff

Re: am I not cool enough for you?

[Andrew Haley]

Jeff Sturm writes:
 > On Mon, 10 Jun 2002, Andrew Haley wrote:
 > > +    unsigned int* stack = (unsigned int*)elements(stackArr);
 > > 
 > > You'll get alignment faults if you do this -- you can't cast a jbyte*
 > > to a jlong*.
 > 
 > True in general, but isn't the 0th array element guaranteed to be
 > word-aligned in this case?

No; it may break on Alpha.

Andrew.

Re: am I not cool enough for you?

[Andrew Haley]

Adam Megacz writes:
 > 
 > Andrew Haley <aph@cambridge.redhat.com> writes:
 > 
 > > printRawStackTrace copies from stackTraceBytes to a suitably aligned
 > > array.
 > 
 > That sounds like a much better idea. I'll reimplement and post a new
 > patch.
 > 
 > Any other things I should address?

Quite possibly.  I only scanned the patch for gotchas, rather than
making sure that it does the Right Thing.

Andrew.

Re: Stack traces from reflection metadata

[Bryce McKinlay]

Hi Adam,

Sorry I didn't get to look at this patch earlier.

Adam Megacz wrote:

>>printRawStackTrace copies from stackTraceBytes to a suitably aligned
>>array.
>>    
>>
>
>That sounds like a much better idea. I'll reimplement and post a new
>patch.
>
>Any other things I should address?
>  
>

On mainline you'll notice that Throwable now mostly implements the JDK 
1.4 API. The main bit that isn't updated is printStackTrace(), which 
should call getStackTrace() and then print the stack trace based on the 
contents of the returned StackTraceElement array. So, ideally this would 
be implemented as a "back end" for getStackTrace(). Another back end 
would be the existing addr2line lookup support, and yet another would be 
the hypothetical one that uses dwarf2 debug info.

Another thing is that it getAllMethodAddrs() seems to return methods for 
all loaded classes - it would be more efficient to only search in 
initialized classes. Only methods from initialized classes can appear in 
a stack trace. An optimization would be to cache the array of methods 
returned by getAllMethodAddrs() etc and only return a new one if new 
classes have been initialized in the interim.

I guess there is no easy way to identify native frames that arn't 
methods declared in Java.

Is it safe to compare pointers as Java longs? Since longs are signed it 
could potentially fail with 64-bit pointers?

Thanks for working on this, it will be a very useful addition to libgcj.

regards

Bryce.

Re: Stack traces from reflection metadata

[Andrew Haley]

Bryce McKinlay writes:
 > 
 > On mainline you'll notice that Throwable now mostly implements the JDK 
 > 1.4 API. The main bit that isn't updated is printStackTrace(), which 
 > should call getStackTrace() and then print the stack trace based on the 
 > contents of the returned StackTraceElement array. So, ideally this would 
 > be implemented as a "back end" for getStackTrace(). Another back end 
 > would be the existing addr2line lookup support, and yet another would be 
 > the hypothetical one that uses dwarf2 debug info.

We have to be very careful not to break anything here.

Some desirable features of what we have at the moment:

1.  Creating an instance of Throwable is fast.  It's good that we
    don't do any expensive searches when fillInStackTrace() is called.

2.  We display the whole stack, native methods and all.

Some undesirable features of what we have at the moment:

1.  No information about interpreted classes.  I have a scheme to fix
    this that I'll do as soon as I get a round tuit.

I like the idea of multiple back ends.  This means we could keep the
good features of the existing printStackTrace() where it's
advantageous.

Andrew.

Re: Stack traces from reflection metadata

[Bryce McKinlay]

Andrew Haley wrote:

>We have to be very careful not to break anything here.
>
>Some desirable features of what we have at the moment:
>
>1.  Creating an instance of Throwable is fast.  It's good that we
>    don't do any expensive searches when fillInStackTrace() is called.
>

Right - fillInStackTrace() walks the stack and collects the PC/IP for 
each frame, exactly what it does already. This is called from the 
Throwable constructor. getStackTrace() takes that info and converts it 
to the StackFrameInfo array which contains the method names, line number 
info, etc - as much as is available. Finally printStackTrace() takes the 
StackTraceData array and prints the trace.

regards

Bryce.