Re: Line number support for interpreter

[Mark Wielaard <mark at klomp dot org>]

Hi,

On Mon, 2004-05-10 at 02:00, Mark Wielaard wrote:
> I haven't tested the patch very much yet. But a simple test class that
> throws some exceptions works and shows the correct line numbers. And I
> am able to startup eclipse (2.1.2) with it under plain or
> direct-threaded interpreter. (This was also the motivation for the patch
> since I use to run eclipse under gij and got annoyed by the fact that
> exceptions didn't have source and line numbers.)

I forgot to mention that I made it against 3.4 sources.
The attached patch is against CVS HEAD.

I tested it some more and it doesn't give regressions (and as mentioned
above eclipse 2.1.2 runs with this). But I haven't checked against Mauve
yet. And I found at least one example which breaks with this code. I
made the attached Invoke.java test (using reflection to call an
interface method on an object that implements that interface).
I cannot figure out what I changed that causes this to fail with my
patch. Anyone a clue?

It might be a good idea to add this small test to testsuite/libjava.lang
since it seems a strange corner case that isn't normally tested/used.

Note that is only a port of the original patch to CVS head, I have not
yet implemented any of the suggestions of Tom and Bryce (who was so nice
to take a look and give some comments on IRC). The patch also includes
the fix that was made for 3.4 to get interpret.cc working again
(http://gcc.gnu.org/PR13468 [3.5 Regression] interpret.cc should
register ffi closure stubs).

Cheers,

Mark

Index: Makefile.am
===================================================================
RCS file: /cvs/gcc/gcc/libjava/Makefile.am,v
retrieving revision 1.377
diff -u -r1.377 Makefile.am
--- Makefile.am	15 May 2004 20:39:08 -0000	1.377
+++ Makefile.am	16 May 2004 17:55:38 -0000
@@ -470,6 +470,13 @@
 $(c_files): %.lo: %.c
 	$(LTCOMPILE) -c -o $@ $<
 
+## A special rule for interpret.lo for the time being.  Our current
+## approach to stack-trace handling is fragile and will not work with
+## unit-at-a-time.  So, for now we disable it.  This will be fixed by
+## a larger patch in the future.
+interpret.lo: interpret.cc
+	$(LTCXXCOMPILE) -fno-unit-at-a-time -c -o $@ $<
+
 $(extra_cc_files): %.lo: %.cc
 	$(LTCXXCOMPILE) -c -o $@ $<
 
Index: Makefile.in
===================================================================
RCS file: /cvs/gcc/gcc/libjava/Makefile.in,v
retrieving revision 1.401
diff -u -r1.401 Makefile.in
--- Makefile.in	15 May 2004 20:39:08 -0000	1.401
+++ Makefile.in	16 May 2004 17:55:42 -0000
@@ -5487,6 +5487,9 @@
 $(c_files): %.lo: %.c
 	$(LTCOMPILE) -c -o $@ $<
 
+interpret.lo: interpret.cc
+	$(LTCXXCOMPILE) -fno-unit-at-a-time -c -o $@ $<
+
 $(extra_cc_files): %.lo: %.cc
 	$(LTCXXCOMPILE) -c -o $@ $<
 
Index: defineclass.cc
===================================================================
RCS file: /cvs/gcc/gcc/libjava/defineclass.cc,v
retrieving revision 1.37
diff -u -r1.37 defineclass.cc
--- defineclass.cc	6 May 2004 14:06:28 -0000	1.37
+++ defineclass.cc	16 May 2004 17:55:47 -0000
@@ -1,6 +1,6 @@
 // defineclass.cc - defining a class from .class format.
 
-/* Copyright (C) 1999, 2000, 2001, 2002, 2003  Free Software Foundation
+/* Copyright (C) 1999, 2000, 2001, 2002, 2003, 2004  Free Software Foundation
 
    This file is part of libgcj.
 
@@ -226,6 +226,10 @@
     pos    = 0;
     def    = klass;
     def_interp = (_Jv_InterpClass *) def->aux_info;
+
+    // Source file name is set when there is a "SourceFile" class attribute.
+    def_interp->source_name = NULL;
+
   }
 
   /** and here goes the parser members defined out-of-line */
@@ -609,26 +613,56 @@
     }
 }
 
-void _Jv_ClassReader::read_one_code_attribute (int /*method*/) 
+void _Jv_ClassReader::read_one_code_attribute (int method_index) 
 {
-  /* ignore for now, ... later we may want to pick up
-     line number information, for debugging purposes;
-     in fact, the whole debugger issue is open!  */
-
-  /* int name = */ read2u ();
+  int name = read2u ();
   int length = read4 ();
-  skip (length);
-
+  if (is_attribute_name (name, "LineNumberTable"))
+    {
+      _Jv_InterpMethod *method = reinterpret_cast<_Jv_InterpMethod *>
+	(def_interp->interpreted_methods[method_index]);
+      if (method->line_table != NULL)
+	throw_class_format_error ("only one LineNumberTable per method");
+
+      int table_len = read2u ();
+      _Jv_LineTableEntry* table
+	= (_Jv_LineTableEntry *) _Jv_Malloc (table_len
+					     * sizeof (_Jv_LineTableEntry));
+      for (int i = 0; i < table_len; i++)
+	{
+	  table[i].pc.i = read2u ();
+	  table[i].line = read2u ();
+	}
+      method->line_table_len = table_len;
+      method->line_table = table;
+    }
+  else
+    {
+      /* ignore unknown code attributes */
+      skip (length);
+    }
 }
 
 void _Jv_ClassReader::read_one_class_attribute () 
 {
-  /* we also ignore the class attributes, ...
+  /* we also ignore most class attributes, ...
      some day we'll add inner-classes support. */
 
-  /* int name = */ read2u ();
+  int name = read2u ();
   int length = read4 ();
-  skip (length);
+  if (is_attribute_name (name, "SourceFile"))
+    {
+      int source_index = read2u ();
+      check_tag (source_index, JV_CONSTANT_Utf8);
+      prepare_pool_entry (source_index, JV_CONSTANT_Utf8);
+      def_interp->source_name = _Jv_NewStringUtf8Const
+	(def->constants.data[source_index].utf8);
+    }
+  else
+    {
+      /* ignore unknown class attributes */
+      skip (length);
+    }
 }
 
 
@@ -1282,6 +1316,10 @@
   method->defining_class = def;
   method->self           = &def->methods[method_index];
   method->prepared       = NULL;
+
+  // This is only set if there is a "LineNumberTable" code attribute.
+  method->line_table_len = 0;
+  method->line_table     = NULL;
 
   // grab the byte code!
   memcpy ((void*) method->bytecode (),
Index: interpret.cc
===================================================================
RCS file: /cvs/gcc/gcc/libjava/interpret.cc,v
retrieving revision 1.41
diff -u -r1.41 interpret.cc
--- interpret.cc	16 Apr 2004 19:38:27 -0000	1.41
+++ interpret.cc	16 May 2004 17:55:48 -0000
@@ -1,6 +1,6 @@
 // interpret.cc - Code for the interpreter
 
-/* Copyright (C) 1999, 2000, 2001, 2002, 2003 Free Software Foundation
+/* Copyright (C) 1999, 2000, 2001, 2002, 2003, 2004 Free Software Foundation
 
    This file is part of libgcj.
 
@@ -12,11 +12,6 @@
 
 #include <config.h>
 
-// Define this to get the direct-threaded interpreter.  If undefined,
-// we revert to a basic bytecode interpreter.  The former is faster
-// but uses more memory.
-#define DIRECT_THREADED
-
 #pragma implementation "java-interp.h"
 
 #include <jvm.h>
@@ -56,26 +51,6 @@
 
 extern "C" double __ieee754_fmod (double,double);
 
-// This represents a single slot in the "compiled" form of the
-// bytecode.
-union insn_slot
-{
-  // Address of code.
-  void *insn;
-  // An integer value used by an instruction.
-  jint int_val;
-  // A pointer value used by an instruction.
-  void *datum;
-};
-
-// The type of the PC depends on whether we're doing direct threading
-// or a more ordinary bytecode interpreter.
-#ifdef DIRECT_THREADED
-typedef insn_slot *pc_t;
-#else
-typedef unsigned char *pc_t;
-#endif
-
 static inline void dupx (_Jv_word *sp, int n, int x)
 {
   // first "slide" n+x elements n to the right
@@ -755,6 +730,10 @@
       exc[i].handler_type.p = handler;
     }
 
+  // Update line number table
+  for (int i = 0; i < line_table_len; i++)
+    line_table[i].pc.p = &insns[pc_mapping[line_table[i].pc.i]];
+
   prepared = insns;
 }
 #endif /* DIRECT_THREADED */
@@ -1012,10 +991,11 @@
     0
   };
 
-  pc_t pc;
+// XXX Ugly quick and dirty HACK so we don't need to rename the variable
+// in the complete source.
+#define pc frame_desc.pc
 
 #ifdef DIRECT_THREADED
-
 #define NEXT_INSN goto *((pc++)->insn)
 #define INTVAL() ((pc++)->int_val)
 #define AVAL() ((pc++)->datum)
@@ -3208,6 +3188,28 @@
 void
 _Jv_EndOfInterpreter (void)
 {
+}
+
+// XXX HACK HACK HACK
+#undef pc
+
+int
+_Jv_InterpMethod::get_source_line(pc_t mpc)
+{
+#ifdef DIRECT_THREADED
+  void *lpc = (void *) ((insn_slot *) mpc - 1);
+#else
+  int lpc = mpc - 1 - bytecode ();
+#endif
+
+  int line = line_table_len > 0 ? line_table[0].line : -1;
+  for (int i = 1; i < line_table_len; i++)
+    if (PCVAL(line_table[i].pc) > lpc)
+      break;
+    else
+      line = line_table[i].line;
+
+  return line;
 }
 
 static void
Index: verify.cc
===================================================================
RCS file: /cvs/gcc/gcc/libjava/verify.cc,v
retrieving revision 1.62
diff -u -r1.62 verify.cc
--- verify.cc	19 Mar 2004 17:38:23 -0000	1.62
+++ verify.cc	16 May 2004 17:55:49 -0000
@@ -3114,8 +3114,10 @@
 void
 _Jv_VerifyMethod (_Jv_InterpMethod *meth)
 {
+#if 0
   _Jv_BytecodeVerifier v (meth);
   v.verify_instructions ();
+#endif
 }
 
 #endif	/* INTERPRETER */
Index: java/lang/Class.h
===================================================================
RCS file: /cvs/gcc/gcc/libjava/java/lang/Class.h,v
retrieving revision 1.66
diff -u -r1.66 Class.h
--- java/lang/Class.h	21 Apr 2004 19:26:22 -0000	1.66
+++ java/lang/Class.h	16 May 2004 17:55:49 -0000
@@ -1,6 +1,7 @@
 // Class.h - Header file for java.lang.Class.  -*- c++ -*-
 
-/* Copyright (C) 1998, 1999, 2000, 2001, 2002, 2003  Free Software Foundation
+/* Copyright (C) 1998, 1999, 2000, 2001, 2002, 2003, 2004
+   Free Software Foundation
 
    This file is part of libgcj.
 
@@ -21,6 +22,7 @@
 #include <java/security/ProtectionDomain.h>
 #include <java/lang/Package.h>
 #include <gnu/gcj/runtime/StackTrace.h>
+#include <gnu/gcj/runtime/NameFinder.h>
 
 // We declare these here to avoid including gcj/cni.h.
 extern "C" void _Jv_InitClass (jclass klass);
@@ -387,6 +389,7 @@
   friend class _Jv_BytecodeVerifier;
   friend class _Jv_StackTrace;
   friend class gnu::gcj::runtime::StackTrace;
+  friend class gnu::gcj::runtime::NameFinder;
   friend class java::io::VMObjectStreamClass;
 
   friend void _Jv_sharedlib_register_hook (jclass klass);
Index: gnu/gcj/runtime/natNameFinder.cc
===================================================================
RCS file: /cvs/gcc/gcc/libjava/gnu/gcj/runtime/natNameFinder.cc,v
retrieving revision 1.6
diff -u -r1.6 natNameFinder.cc
--- gnu/gcj/runtime/natNameFinder.cc	9 Apr 2004 04:39:24 -0000	1.6
+++ gnu/gcj/runtime/natNameFinder.cc	16 May 2004 17:55:50 -0000
@@ -134,22 +134,24 @@
 {
 #ifdef INTERPRETER
   _Jv_frame_info *stack = (_Jv_frame_info *) addrs;
-  if (stack[n].interp == NULL)
+  pc_t pc = (pc_t) stack[n].interp;
+  if (pc == NULL)
     return NULL;
 
   _Jv_InterpMethod *meth
-    = reinterpret_cast<_Jv_InterpMethod *> (stack[n].interp);
+    = reinterpret_cast<_Jv_InterpMethod *> (stack[n].addr);
   java::lang::StringBuffer *sb = new java::lang::StringBuffer();
   sb->append(_Jv_NewStringUtf8Const(meth->self->name));
   sb->append(_Jv_NewStringUtf8Const(meth->self->signature));
-  // FIXME: source file name and line number can be found from
-  // bytecode debug information.  But currently we don't keep that
-  // around.
   // FIXME: is using the defining class correct here?
-  java::lang::String *className = meth->defining_class->getName();
+  jclass clazz = meth->defining_class;
+  _Jv_InterpClass * iclazz = (_Jv_InterpClass *) clazz->aux_info;
+  java::lang::String *source = iclazz->source_name;
+  jint line = meth->get_source_line(pc);
+  java::lang::String *className = clazz->getName();
   java::lang::String *methodName
 	  = demangleInterpreterMethod(sb->toString(), className);
-  return new java::lang::StackTraceElement(NULL, -1,
+  return new java::lang::StackTraceElement(source, line,
 					   className, methodName, false);
 #else // INTERPRETER
   return NULL;
Index: gnu/gcj/runtime/natStackTrace.cc
===================================================================
RCS file: /cvs/gcc/gcc/libjava/gnu/gcj/runtime/natStackTrace.cc,v
retrieving revision 1.6
diff -u -r1.6 natStackTrace.cc
--- gnu/gcj/runtime/natStackTrace.cc	2 Oct 2003 07:10:34 -0000	1.6
+++ gnu/gcj/runtime/natStackTrace.cc	16 May 2004 17:55:51 -0000
@@ -57,6 +57,15 @@
   _Jv_frame_info *frame;
   if (len > 0)
     {
+      // When we don't have an interpreter we simply allocate len
+      // frames which will hold the addresses we just collected.  If
+      // we do have an interpreter the frames will have an extra 'interp'
+      // field that we have to fill in with NULL if the address doesn't
+      // come from the interpreter and otherwise we fill it in with
+      // the 'pc' of the next interpreter frame for this thread and let
+      // the 'addr' field point to an interpreted method struct (which can
+      // be queried later to collect the name of the class and method
+      // that was executing).
 #ifdef INTERPRETER
       extern void _Jv_StartOfInterpreter (void);
       extern void _Jv_EndOfInterpreter (void);
@@ -70,15 +79,21 @@
       frame = (_Jv_frame_info *) _Jv_Malloc (len * sizeof (_Jv_frame_info));
       for (int n = 0; n < len; n++)
 	{
-	  frame[n].addr = p[n];
 #ifdef INTERPRETER
 	  if (p[n] >= &_Jv_StartOfInterpreter && p[n] <= &_Jv_EndOfInterpreter)
 	    {
-	      frame[n].interp = (void *) interp_frame->self;
+	      frame[n].interp = (void *) interp_frame->pc;
+	      frame[n].addr = (void *) interp_frame->self;
 	      interp_frame = interp_frame->next;
 	    }
 	  else
-	    frame[n].interp = 0;
+	    {
+	      frame[n].interp = NULL;
+#endif
+	      // This is the only line in the for loop without INTERPRETER.
+	      frame[n].addr = p[n];
+#ifdef INTERPRETER
+	    }
 #endif // INTERPRETER
 	}
     }
@@ -142,7 +157,7 @@
   if (frame->interp)
     {
       _Jv_InterpMethod *meth
-	= reinterpret_cast<_Jv_InterpMethod *> (frame->interp);
+	= reinterpret_cast<_Jv_InterpMethod *> (frame->addr);
       return meth->defining_class;
     }
 #endif // INTERPRETER
@@ -160,7 +175,7 @@
   if (frame->interp)
     {
       meth
-	= reinterpret_cast<_Jv_InterpMethod *> (frame->interp)
+	= reinterpret_cast<_Jv_InterpMethod *> (frame->addr)
 	->get_method();
     }
 #endif // INTERPRETER
Index: include/java-interp.h
===================================================================
RCS file: /cvs/gcc/gcc/libjava/include/java-interp.h,v
retrieving revision 1.24
diff -u -r1.24 java-interp.h
--- include/java-interp.h	20 Apr 2004 01:38:45 -0000	1.24
+++ include/java-interp.h	16 May 2004 17:55:51 -0000
@@ -27,6 +27,31 @@
 #include <ffi.h>
 }
 
+// Define this to get the direct-threaded interpreter.  If undefined,
+// we revert to a basic bytecode interpreter.  The former is faster
+// but uses more memory.
+#define DIRECT_THREADED 
+
+#ifdef DIRECT_THREADED
+// This represents a single slot in the "compiled" form of the
+// bytecode.
+union insn_slot
+{
+  // Address of code.
+  void *insn;
+  // An integer value used by an instruction.
+  jint int_val;
+  // A pointer value used by an instruction.
+  void *datum;
+};
+
+// The type of the PC depends on whether we're doing direct threading
+// or a more ordinary bytecode interpreter.
+typedef insn_slot *pc_t;
+#else
+typedef unsigned char *pc_t;
+#endif // DIRECT_THREADED
+
 extern inline jboolean
 _Jv_IsInterpretedClass (jclass c)
 {
@@ -100,6 +125,17 @@
   }
 };
 
+// This structure holds the bytecode pc and corresponding source code
+// line number.  An array (plus length field) of this structure is put
+// in each _Jv_InterpMethod and used to resolve the (internal) program
+// counter of the interpreted method to an actual java source file
+// line.
+struct  _Jv_LineTableEntry
+{
+  _Jv_InterpPC pc;
+  int line;
+};
+
 class _Jv_InterpMethod : public _Jv_MethodBase
 {
   _Jv_ushort       max_stack;
@@ -108,6 +144,11 @@
 
   _Jv_ushort       exc_count;
 
+  // Length of the line_table.
+  // When this is zero then line_table is NULL.
+  int line_table_len;
+  _Jv_LineTableEntry *line_table;
+
   void *prepared;
 
   unsigned char* bytecode () 
@@ -142,6 +183,11 @@
 
   void run (void*, ffi_raw *);
 
+  // If there is a line number table for this method then this
+  // function returns the java source file line number given a process
+  // counter (from a stack frame). Otherwise it returns -1.
+  int get_source_line(pc_t mpc);
+
  public:
   static void dump_object(jobject o);
 
@@ -162,6 +208,7 @@
 {
   _Jv_MethodBase **interpreted_methods;
   _Jv_ushort        *field_initializers;
+  jstring	 source_name;
 
   friend class _Jv_ClassReader;
   friend class _Jv_InterpMethod;
@@ -174,6 +221,8 @@
 
   friend _Jv_MethodBase ** _Jv_GetFirstMethod (_Jv_InterpClass *klass);
   friend void _Jv_Defer_Resolution (void *cl, _Jv_Method *meth, void **);
+
+  friend class gnu::gcj::runtime::NameFinder;
 };
 
 // We have an interpreted class CL and we're trying to find the
@@ -254,6 +303,13 @@
 struct _Jv_MethodChain
 {
   const _Jv_InterpMethod *self;
+
+  // The current virtual process counter.
+  // Used by the interpreter run method to keep track of where it is.
+  // Can be turned into an actual source line by feeding it to
+  // self->get_source_line().
+  pc_t pc;
+
   _Jv_MethodChain **ptr;
   _Jv_MethodChain *next;
 
Index: include/jvm.h
===================================================================
RCS file: /cvs/gcc/gcc/libjava/include/jvm.h,v
retrieving revision 1.64
diff -u -r1.64 jvm.h
--- include/jvm.h	14 Apr 2004 17:45:18 -0000	1.64
+++ include/jvm.h	16 May 2004 17:55:51 -0000
@@ -122,8 +122,8 @@
   // PC value.
   void *addr;
 #ifdef INTERPRETER
-  // Actually a _Jv_InterpMethod, but we don't want to include
-  // java-interp.h everywhere.
+  // If not NULL addr is a _Jv_InterpMethod, otherwise addr is a real address.
+  // Actually a pc_t, but we don't want to include java-interp.h everywhere.
   void *interp;
 #endif // INTERPRETER
 };

import java.lang.reflect.Method;

public class Invoke
{
  interface I
  {
    void m();
  }

  static class C implements I
  {
    public void m()
    {
      System.out.println("Hello World");
    }
  }

  public static void main(String[] args) throws Exception
  {
    Method m = I.class.getDeclaredMethod("m", null);
    Object o = new C();
    System.out.println("Invoking...");
    m.invoke(o, null);
    System.out.println("...Invoked");
  }
}

Attachment: signature.asc
Description: This is a digitally signed message part