[RFA] REWRITE_INSN

[RFA] REWRITE_INSN

[Keith Seitz]

Hi,

The attached patch adds the macro REWRITE_INSN into the interpreter. 
This macro simply replaces all the code in interpret-run.cc that look 
like "pc[-2].insn = FOO; pc[-1].BAR = BAZ;" with "REWRITE_INSN (FOO, 
BAR, BAZ);"

For the non-debugging case, REWRITE_INSN is simply the previous 
(current) code. For the debugging case, REWRITE_INSN checks to see if 
there is a breakpoint at pc[-2], and, if so, calls 
_Jv_RewriteBreakpointInsn so that the new insn is saved into the 
breakpoint. Without it, we would continue to try to run the "old" insn, 
which really cripples the effectiveness of breakpoints. :-)

This is my last critical patch before I'm ready to say "merge".

Questions/comments/concerns?
Keith

ChangeLog
2007-04-23  Keith Seitz  <keiths@redhat.com>

         * headers.txt (gnu/gcj/jvmti/Breakpoint.h)[DIRECT_THREADED]:
         Add _Jv_RewriteBreakpointInsn friend declaration.
         * gnu/gcj/jvmti/natBreakpoint.cc (_Jv_RewriteBreakpointInsn)
         [DIRECT_THREADED]: New function.
         * gnu/gcj/jvmti/Breakpoint.h: Regenerate.
         * interpret-run.cc: Define new REWRITE_INSN macro.
         Changed all occurrences of insn rewriting to call REWRITE_INSN.

Re: [RFA] REWRITE_INSN

[Tom Tromey]

>>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes:

Keith> The attached patch adds the macro REWRITE_INSN into the
Keith> interpreter. This macro simply replaces all the code in
Keith> interpret-run.cc that look like "pc[-2].insn = FOO; pc[-1].BAR = BAZ;"
Keith> with "REWRITE_INSN (FOO, BAR, BAZ);"

This code is so awful.  I regret writing it actually ... it is racy
for one thing.  Perhaps we should disable it.  Or make it non-racy,
which is possible though a bit tricky.

Anyway ... you didn't attach a patch, but I'm sure it is ok.

Keith> For the non-debugging case, REWRITE_INSN is simply the previous
Keith> (current) code. For the debugging case, REWRITE_INSN checks to see if
Keith> there is a breakpoint at pc[-2], and, if so, calls
Keith> _Jv_RewriteBreakpointInsn so that the new insn is saved into the
Keith> breakpoint. Without it, we would continue to try to run the "old"
Keith> insn, which really cripples the effectiveness of breakpoints. :-)

We could also simply disable rewriting for the debug case.

Tom

Re: [RFA] REWRITE_INSN

[Keith Seitz]

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

Tom Tromey wrote:

> Anyway ... you didn't attach a patch, but I'm sure it is ok.

Whoops. Attached now.

> We could also simply disable rewriting for the debug case.

We could do that, too. I was once again shooting for being least 
intrusive. It's your call, though. Just let me know what you want done.

Keith

[-- Attachment #2: rewrite-insn.patch --]
[-- Type: text/x-patch, Size: 5593 bytes --]

Index: headers.txt
===================================================================
--- headers.txt	(revision 124082)
+++ headers.txt	(working copy)
@@ -63,5 +63,13 @@
 friend class java::io::ObjectInputStream;
 friend java::lang::reflect::Method* ::_Jv_GetReflectedMethod (jclass, _Jv_Utf8Const*, _Jv_Utf8Const*);
 
+class gnu/gcj/jvmti/Breakpoint
+prepend #ifdef DIRECT_THREADED
+prepend void _Jv_RewriteBreakpointInsn (jmethodID, jlocation, pc_t);
+prepend #endif
+add #ifdef DIRECT_THREADED
+add friend void (::_Jv_RewriteBreakpointInsn (jmethodID, jlocation, pc_t));
+add #endif
+
 class gnu/gcj/runtime/ExtensionClassLoader
 friend class ::java::lang::ClassLoader;
Index: gnu/gcj/jvmti/natBreakpoint.cc
===================================================================
--- gnu/gcj/jvmti/natBreakpoint.cc	(revision 124082)
+++ gnu/gcj/jvmti/natBreakpoint.cc	(working copy)
@@ -17,6 +17,7 @@
 #include <jvmti.h>
 
 #include <gnu/gcj/jvmti/Breakpoint.h>
+#include <gnu/gcj/jvmti/BreakpointManager.h>
 
 static _Jv_InterpMethod *
 get_interp_method (jlong method)
@@ -54,3 +55,18 @@
   _Jv_InterpMethod *imeth = get_interp_method (method);
   imeth->set_insn (location, reinterpret_cast<pc_t> (data));
 }
+
+#ifdef DIRECT_THREADED
+void
+_Jv_RewriteBreakpointInsn (jmethodID mid, jlocation loc, pc_t new_insn)
+{
+  using namespace ::gnu::gcj::jvmti;
+  Breakpoint *bp
+    = BreakpointManager::getBreakpoint (reinterpret_cast<jlong> (mid), loc);
+  if (bp != NULL)
+    {
+      pc_t old_insn = (pc_t) bp->data;
+      old_insn->insn = new_insn;
+    }
+}
+#endif // DIRECT_THREADED
Index: interpret-run.cc
===================================================================
--- interpret-run.cc	(revision 124082)
+++ interpret-run.cc	(working copy)
@@ -359,9 +359,32 @@
       goto *((pc++)->insn);						\
     }									\
   while (0)
+
+#undef REWRITE_INSN
+#define REWRITE_INSN(INSN,SLOT,VALUE)					\
+  do {									\
+    if (pc[-2].insn == breakpoint_insn->insn)				\
+      {									\
+	using namespace ::gnu::gcj::jvmti;				\
+	jlocation location = meth->insn_index (pc - 2);			\
+	_Jv_RewriteBreakpointInsn (meth->self, location, (pc_t) INSN);	\
+      }									\
+    else								\
+      pc[-2].insn = INSN;						\
+									\
+    pc[-1].SLOT = VALUE;						\
+  }									\
+  while (0)
+
 #else
 #undef NEXT_INSN
 #define NEXT_INSN goto *((pc++)->insn)
+#define REWRITE_INSN(INSN,SLOT,VALUE)		\
+  do {						\
+    pc[-2].insn = INSN;				\
+    pc[-1].SLOT = VALUE;			\
+  }						\
+  while (0)
 #endif
 
 #define INTVAL() ((pc++)->int_val)
@@ -511,8 +534,7 @@
 #ifdef DIRECT_THREADED
 	// Rewrite instruction so that we use a faster pre-resolved
 	// method.
-	pc[-2].insn = &&invokevirtual_resolved;
-	pc[-1].datum = rmeth;
+	REWRITE_INSN (&&invokevirtual_resolved, datum, rmeth);
 #endif /* DIRECT_THREADED */
       }
       goto perform_invoke;
@@ -1846,8 +1868,7 @@
 	  }
 
 #ifdef DIRECT_THREADED
-	pc[-2].insn = newinsn;
-	pc[-1].datum = field->u.addr;
+	REWRITE_INSN (newinsn, datum, field->u.addr);
 #endif /* DIRECT_THREADED */
       }
       NEXT_INSN;
@@ -1937,8 +1958,7 @@
 	  }
 
 #ifdef DIRECT_THREADED
-	pc[-2].insn = newinsn;
-	pc[-1].int_val = field_offset;
+	REWRITE_INSN (newinsn, int_val, field_offset);
 #endif /* DIRECT_THREADED */
       }
       NEXT_INSN;
@@ -2053,8 +2073,7 @@
 	  }
 
 #ifdef DIRECT_THREADED
-	pc[-2].insn = newinsn;
-	pc[-1].datum = field->u.addr;
+	REWRITE_INSN (newinsn, datum, field->u.addr);
 #endif /* DIRECT_THREADED */
       }
       NEXT_INSN;
@@ -2152,8 +2171,7 @@
 	  }
 
 #ifdef DIRECT_THREADED
-	pc[-2].insn = newinsn;
-	pc[-1].int_val = field_offset;
+	REWRITE_INSN (newinsn, int_val, field_offset);
 #endif /* DIRECT_THREADED */
       }
       NEXT_INSN;
@@ -2228,8 +2246,7 @@
 #ifdef DIRECT_THREADED
 	// Rewrite instruction so that we use a faster pre-resolved
 	// method.
-	pc[-2].insn = &&invokespecial_resolved;
-	pc[-1].datum = rmeth;
+	REWRITE_INSN (&&invokespecial_resolved, datum, rmeth);
 #endif /* DIRECT_THREADED */
       }
       goto perform_invoke;
@@ -2266,8 +2283,7 @@
 #ifdef DIRECT_THREADED
 	// Rewrite instruction so that we use a faster pre-resolved
 	// method.
-	pc[-2].insn = &&invokestatic_resolved;
-	pc[-1].datum = rmeth;
+	REWRITE_INSN (&&invokestatic_resolved, datum, rmeth);
 #endif /* DIRECT_THREADED */
       }
       goto perform_invoke;
@@ -2305,8 +2321,7 @@
 #ifdef DIRECT_THREADED
 	// Rewrite instruction so that we use a faster pre-resolved
 	// method.
-	pc[-2].insn = &&invokeinterface_resolved;
-	pc[-1].datum = rmeth;
+	REWRITE_INSN (&&invokeinterface_resolved, datum, rmeth);
 #else
 	// Skip dummy bytes.
 	pc += 2;
@@ -2344,8 +2359,7 @@
 	PUSHA (res);
 
 #ifdef DIRECT_THREADED
-	pc[-2].insn = &&new_resolved;
-	pc[-1].datum = klass;
+	REWRITE_INSN (&&new_resolved, datum, klass);
 #endif /* DIRECT_THREADED */
       }
       NEXT_INSN;
@@ -2380,8 +2394,7 @@
 	PUSHA (result);
 
 #ifdef DIRECT_THREADED
-	pc[-2].insn = &&anewarray_resolved;
-	pc[-1].datum = klass;
+	REWRITE_INSN (&&anewarray_resolved, datum, klass);
 #endif /* DIRECT_THREADED */
       }
       NEXT_INSN;
@@ -2425,8 +2438,7 @@
 	PUSHA (value);
 
 #ifdef DIRECT_THREADED
-	pc[-2].insn = &&checkcast_resolved;
-	pc[-1].datum = to;
+	REWRITE_INSN (&&checkcast_resolved, datum, to);
 #endif /* DIRECT_THREADED */
       }
       NEXT_INSN;
@@ -2453,8 +2465,7 @@
 	PUSHI (to->isInstance (value));
 
 #ifdef DIRECT_THREADED
-	pc[-2].insn = &&instanceof_resolved;
-	pc[-1].datum = to;
+	REWRITE_INSN (&&instanceof_resolved, datum, to);
 #endif /* DIRECT_THREADED */
       }
       NEXT_INSN;

Re: [RFA] REWRITE_INSN

[Tom Tromey]

>>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes:

>> We could also simply disable rewriting for the debug case.

Keith> We could do that, too. I was once again shooting for being least
Keith> intrusive. It's your call, though. Just let me know what you want done.

Let's go with your patch.
I'll disable the rewriting in a followup patch.

Tom

Re: [RFA] REWRITE_INSN

[Keith Seitz]

Tom Tromey wrote:
> Keith> We could do that, too. I was once again shooting for being least
> Keith> intrusive. It's your call, though. Just let me know what you want done.
> 
> Let's go with your patch.
> I'll disable the rewriting in a followup patch.

Okay, thanks!

Keith