[RFA] Step & Breakpoint co-located events

[Keith Seitz <keiths at redhat dot com>]

Hi,

The attached patch "fixes" the multiple co-located events problems at a 
location. [Review: we currently require two steps while stepping through 
a line with a breakpoint: one for the step and one for the breakpoint.]

According to the JDWP specification, we can expect to receive 
MethodEntry (or MethodExit), Step, and Breakpoint events together in a 
single packet. While I have not implemented MethodEntry/Exit (since: 1) 
we don't have them and 2) they don't seem all that useful for JDWP), I 
have left them out of the equation. However, the same framework could be 
used to deal with MethodEntry/Exit if they were added.

QCC?

Keith

ChangeLog
2007-05-07  Keith Seitz  <keiths@redhat.com>

        * include/java-interp.h (breakpoint_at): Declare.
        * interpret.cc (breakpoint_at): New function.
        * gnu/classpath/jdwp/VMVirtualMachine.java (_event_list):
        New member.
        * gnu/classpath/jdwp/natVMVirtualMachine.cc (initialize):
        Initialize _event_list.
        (handle_single_step): If there is a breakpoint at the
        location at which we are stopping, do not send the notification.
        Instead add the event to a list of events that occur at this
        location.
        (jdwpBreakpointCB): If the event list is not empty, send
        whatever events are in it and the breakpoint event in a single
        notification.
        Mark parameter jni_env as MAYBE_UNUSED.
        * classpath/lib/gnu/classpath/jdwp/VMVirtualMachine.class:
        Regenerated.
        * gnu/classpath/jdwp/VMVirtualMachine.h: Regenerated.

Index: include/java-interp.h
===================================================================
--- include/java-interp.h	(revision 124515)
+++ include/java-interp.h	(working copy)
@@ -274,6 +274,9 @@
      the insn or NULL if index is invalid. */
   pc_t set_insn (jlong index, pc_t insn);
 
+  // Is the given location in this method a breakpoint?
+  bool breakpoint_at (jlong index);
+
 #ifdef DIRECT_THREADED
   friend void _Jv_CompileMethod (_Jv_InterpMethod*);
 #endif
Index: interpret.cc
===================================================================
--- interpret.cc	(revision 124515)
+++ interpret.cc	(working copy)
@@ -1559,6 +1559,23 @@
   return &code[index];
 }
 
+bool
+_Jv_InterpMethod::breakpoint_at (jlong index)
+{
+  pc_t insn = get_insn (index);
+  if (insn != NULL)
+    {
+#ifdef DIRECT_THREADED
+      return (insn->insn == breakpoint_insn->insn);
+#else
+      pc_t code = reinterpret_cast<pc_t> (bytecode ());
+      return (code[index] == breakpoint_insn);
+#endif
+    }
+
+  return false;
+}
+
 void *
 _Jv_JNIMethod::ncode (jclass klass)
 {
Index: gnu/classpath/jdwp/VMVirtualMachine.java
===================================================================
--- gnu/classpath/jdwp/VMVirtualMachine.java	(revision 124515)
+++ gnu/classpath/jdwp/VMVirtualMachine.java	(working copy)
@@ -83,6 +83,9 @@
   // List of stepping threads: maps Thread -> stepping info
   static Hashtable _stepping_threads;
   
+  // List of co-located JVMTI events
+  static ArrayList _event_list;
+
   public static native void initialize ();
 
   /**
Index: gnu/classpath/jdwp/natVMVirtualMachine.cc
===================================================================
--- gnu/classpath/jdwp/natVMVirtualMachine.cc	(revision 124515)
+++ gnu/classpath/jdwp/natVMVirtualMachine.cc	(working copy)
@@ -116,6 +116,7 @@
 {
   _jdwp_suspend_counts = new ::java::util::Hashtable ();
   _stepping_threads = new ::java::util::Hashtable ();
+  _event_list = new ::java::util::ArrayList ();
 
   JavaVM *vm = _Jv_GetJavaVM ();
   union
@@ -895,7 +896,23 @@
   jobject instance = iframe->get_this_ptr ();
   event::SingleStepEvent *event
     = new event::SingleStepEvent (thread, loc, instance);
-  Jdwp::notify (event);
+
+  // We only want to send the notification (and consequently
+  // suspend) if we are not about to execute a breakpoint.
+  _Jv_InterpMethod *im = reinterpret_cast<_Jv_InterpMethod *> (iframe->self);
+  if (im->breakpoint_at (location))
+    {
+      // Next insn is a breakpoint -- record event and
+      // wait for the JVMTI breakpoint notification to
+      // enforce a suspension policy.
+      VMVirtualMachine::_event_list->add (event);
+    }
+  else
+    {
+      // Next insn is not a breakpoint, so send notification
+      // and enforce the suspend policy.
+      Jdwp::notify (event);
+    }
 }
 
 static void
@@ -935,7 +952,23 @@
   jobject instance = iframe->get_this_ptr ();
   event::BreakpointEvent *event
     = new event::BreakpointEvent (thread, loc, instance);
-  Jdwp::notify (event);
+
+  if (VMVirtualMachine::_event_list->isEmpty ())
+    {
+      // No co-located events -- send the event
+      Jdwp::notify (event);
+    }
+  else
+    {
+      // We have several events at the same location,
+      // so package them up and then send them.
+      using namespace gnu::classpath::jdwp::event;
+      VMVirtualMachine::_event_list->add (event);
+      JArray<Event *> *events
+	= (JArray<Event *> *) VMVirtualMachine::_event_list->toArray ();
+      VMVirtualMachine::_event_list->clear ();
+      Jdwp::notify (events);
+    }
 }
 
 static void JNICALL
@@ -1001,7 +1034,7 @@
 }
 
 static void JNICALL
-jdwpSingleStepCB (jvmtiEnv *env, JNIEnv *jni_env, jthread thread,
+jdwpSingleStepCB (jvmtiEnv *env, MAYBE_UNUSED JNIEnv *jni_env, jthread thread,
 		  jmethodID method, jlocation location)
 {
   jobject si =