Re: [RFA] JMTI Exception Events

[Kyle Galloway <kgallowa@redhat.com>]

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

Tom Tromey wrote:
>>>>>> "Kyle" == Kyle Galloway <kgallowa@redhat.com> writes:
>>>>>>             
>
> Kyle> Before I commit this, I wanted to clear up which files I should
> Kyle> commit. Do I need to include more than sources.am and Makefile.am?
> Kyle> For example, should the changed Makefile.ins go in as well?
>
> Yes, commit everything that is changed.
>
> Kyle> Questions/comments/concerns?
>
> A few minor things, but one or two substantive ones as well.  Read on..
>
> Kyle> +// Method to check if an exception is hadled at some location (pc) in a method
>
> Typo: "handled".
>   
Fixed.
> Kyle> +// (meth).  It then sets the pc to the start of the handler
> Kyle> +int
> Kyle> +_Jv_InterpMethod::check_handler (pc_t *pc, _Jv_InterpMethod *meth,
> Kyle> +                                java::lang::Throwable *ex)
>
> This should be declared as returning bool or jboolean, not int.
> The meaning of the return value should be documented in the comment.
>
> Kyle> +return false;
>
> Indentation is incorrect.
>   
Fixed.
> Kyle> +  private static WeakHashMap<Thread, Throwable> _ex_map;
>
> I think you probably want to initialize this here, rather than in the
> ExceptionEvent constructor.  Doing it in the constructor means that
> there is a race.
>
> Static fields are initialized when the class is initialized -- so it
> is already done lazily.
>
> Kyle> +      _ex_map = new WeakHashMap ();
>
> ... when you do this I think it is preferable to write:
>
>     .. = new WeakHashMap<Thread, Throwable> ();
>   
I've changed that.
> Kyle> +        if (!(_ex_map.get (thr).equals (ex)))
>
> Without an equals() method in this class, I doubt this does what you
> intend.  As it is this equals() will always return false, because the
> default equals() tests object identity, and a new object is never
> identical to any existing one.
>   
In this case, however, I believe I want to only equal the exact same 
exception. If an exception is not caught in a method, that method is 
popped off the stack and the exception is pushed. Then the next method 
will hit the catch block of the interpreter with the same Exception 
object. What I'm trying to check is if it is the same exception that 
triggered a previous event. When this check fails, it means that the 
exception was caught and another exception was thrown in this thread, 
which should trigger a new event. Now, am I wrong in thinking that an 
exception thrown to the caller of a method is the same exception, does a 
new Exception object get created?
> Kyle> +            send_event ();
>
> It is a bit odd to have a side-effect-ing constructor.  I'd say it is
> a bit more idiomatic to have a static method to dispatch the event,
> which creates the event object as needed.  Seeing a raw 'new ...' as a
> statement is kind of strange.
>   
What I have done now is add a method called post_exception_event, which 
checks to see if this is a repeated throw, and if not constucts an 
ExceptionEvent and alls send event to send the JVMTI event notification.
> Kyle> +// This method looks up the interpreted call stack to see if the excetion will
>
> Typo: "exception".
>   
Fixed.

I think the attached patch addresses all these issues.

Further comments?

- Kyle


[-- Attachment #2: exceptions-jvmti.patch --]
[-- Type: text/x-patch, Size: 13760 bytes --]

Index: libjava/interpret.cc
===================================================================
--- libjava/interpret.cc	(revision 121856)
+++ libjava/interpret.cc	(working copy)
@@ -43,6 +43,7 @@
 #include <gnu/classpath/jdwp/Jdwp.h>
 #include <gnu/gcj/jvmti/Breakpoint.h>
 #include <gnu/gcj/jvmti/BreakpointManager.h>
+#include <gnu/gcj/jvmti/ExceptionEvent.h>
 
 #ifdef INTERPRETER
 
@@ -1366,6 +1367,51 @@
   return -1;
 }
 
+// Method to check if an exception is caught at some location in a method
+// (meth).  Returns true if this method (meth) contains a catch block for the
+// exception (ex). False otherwise.  If there is a catch block, it sets the pc
+// to the location of the beginning of the catch block.
+jboolean
+_Jv_InterpMethod::check_handler (pc_t *pc, _Jv_InterpMethod *meth,
+                                java::lang::Throwable *ex)
+{
+#ifdef DIRECT_THREADED
+  void *logical_pc = (void *) ((insn_slot *) (*pc) - 1);
+#else
+  int logical_pc = (*pc) - 1 - meth->bytecode ();
+#endif
+  _Jv_InterpException *exc = meth->exceptions ();
+  jclass exc_class = ex->getClass ();
+
+  for (int i = 0; i < meth->exc_count; i++)
+    {
+      if (PCVAL (exc[i].start_pc) <= logical_pc
+          && logical_pc < PCVAL (exc[i].end_pc))
+        {
+#ifdef DIRECT_THREADED
+              jclass handler = (jclass) exc[i].handler_type.p;
+#else
+              jclass handler = NULL;
+              if (exc[i].handler_type.i != 0)
+                    handler
+                      = (_Jv_Linker::resolve_pool_entry (meth->defining_class,
+                                                                             ex$
+#endif /* DIRECT_THREADED */
+              if (handler == NULL || handler->isAssignableFrom (exc_class))
+                {
+#ifdef DIRECT_THREADED
+                  (*pc) = (insn_slot *) exc[i].handler_pc.p;
+#else
+                  (*pc) = meth->bytecode () + exc[i].handler_pc.i;
+#endif /* DIRECT_THREADED */
+                  return true;
+                }
+          }
+      }
+  return false;
+}
+
+
 void
 _Jv_InterpMethod::get_line_table (jlong& start, jlong& end,
 				  jintArray& line_numbers,
Index: libjava/classpath/lib/gnu/gcj/jvmti/ExceptionEvent.class
===================================================================
Cannot display: file marked as a binary type.
svn:mime-type = application/octet-stream

Property changes on: libjava/classpath/lib/gnu/gcj/jvmti/ExceptionEvent.class
___________________________________________________________________
Name: svn:mime-type
   + application/octet-stream

Index: libjava/include/java-interp.h
===================================================================
--- libjava/include/java-interp.h	(revision 121856)
+++ libjava/include/java-interp.h	(working copy)
@@ -210,6 +210,11 @@
   // Convenience function for indexing bytecode PC/insn slots in
   // line tables for JDWP
   jlong insn_index (pc_t pc);
+  
+  // Helper function used to check if there is a handler for an exception
+  // present at this code index
+  jboolean check_handler (pc_t *pc, _Jv_InterpMethod *meth,
+                     java::lang::Throwable *ex);
    
   /* Get the line table for this method.
    * start  is the lowest index in the method
Index: libjava/gnu/gcj/jvmti/ExceptionEvent.java
===================================================================
--- libjava/gnu/gcj/jvmti/ExceptionEvent.java	(revision 0)
+++ libjava/gnu/gcj/jvmti/ExceptionEvent.java	(revision 0)
@@ -0,0 +1,96 @@
+// ExceptionEvent - an exception event for JVMTI
+
+/* Copyright (C) 2006  Free Software Foundation
+
+   This file is part of libgcj.
+
+This software is copyrighted work licensed under the terms of the
+Libgcj License.  Please consult the file "LIBGCJ_LICENSE" for
+details.  */
+
+package gnu.gcj.jvmti;
+
+import java.util.WeakHashMap;
+
+/**
+ * Class to create and send JVMTI Exception events
+ *
+ * @author Kyle Galloway (kgallowa@redhat.com)
+ */
+public class ExceptionEvent
+{
+  // Information about where the exception was thrown
+  private long _throw_meth, _throw_loc;
+  
+  // Information about where the exception was or can be caught
+  private long _catch_meth, _catch_loc;
+  
+  // Thread where the exception occurred
+  private Thread _thread;
+  
+  // The exception
+  private Throwable _ex;
+  
+  // A hash map of the exceptions we've already seen in a thread's call stack
+  private static WeakHashMap<Thread, Throwable> _ex_map = new WeakHashMap<Thread, Throwable> ();
+  
+  /**
+   * Constructs a new ExceptionEvent and sends it.  If it is not caught
+   * within the frame where it was thrown (catch_meth and catch_loc are null),
+   * check_catch will check for a possible catch further up the call stack 
+   * before marking it uncaught.
+   * 
+   * @param thr the thread where the exception occurred
+   * @param throw_meth the method of the throw (a jmethodID)
+   * @param throw_loc the location of the throw (a jlocation)
+   * @param ex the exception
+   * @param catch_meth the method of the catch (a jmethodID), null indicates
+   * that the exception was not caught in the frame where it was thrown
+   * @param catch_loc the location of the catch (a jlocation), null indicates
+   * that the exception was not caught in the frame where it was thrown
+   */
+  private ExceptionEvent (Thread thr, long throw_meth, long throw_loc,
+		                  Throwable ex, long catch_meth, long catch_loc)
+  {
+    _thread = thr;
+    _ex = ex;
+    _throw_meth = throw_meth;
+    _throw_loc = throw_loc;
+    _catch_meth = catch_meth;
+    _catch_loc = catch_loc;
+  }
+  
+  public static void post_exception_event (Thread thr, long throw_meth,
+		                                  long throw_loc, Throwable ex,
+		                                  long catch_meth, long catch_loc)
+  {
+    // Check to see if there is an entry for this Thread thr in the has map.
+	// If not, add the thread to the hash map and send an ExceptionEvent.
+	if (_ex_map.containsKey (thr))
+	  {
+		// Check to see if we are receiving events for the same exception, or a
+		// new one.  If it is not the same exception beign rethrown, send a new
+		// event.
+	    if (!(_ex_map.get (thr).equals (ex)))
+	      {
+            _ex_map.put (thr, ex);
+            ExceptionEvent event = new ExceptionEvent (thr, throw_meth,
+            		                                   throw_loc, ex,
+            		                                   catch_meth, catch_loc);  
+            event.send_event ();
+          }
+	  }
+	else
+	  {
+	    _ex_map.put (thr, ex);
+	    ExceptionEvent event = new ExceptionEvent (thr, throw_meth,
+                                                 throw_loc, ex,
+                                                 catch_meth, catch_loc);
+	    event.send_event ();
+	  }
+  }
+  
+  public native void send_event ();
+  
+  public native void check_catch ();
+}
Index: libjava/gnu/gcj/jvmti/ExceptionEvent.h
===================================================================
--- libjava/gnu/gcj/jvmti/ExceptionEvent.h	(revision 0)
+++ libjava/gnu/gcj/jvmti/ExceptionEvent.h	(revision 0)
@@ -0,0 +1,44 @@
+
+// DO NOT EDIT THIS FILE - it is machine generated -*- c++ -*-
+
+#ifndef __gnu_gcj_jvmti_ExceptionEvent__
+#define __gnu_gcj_jvmti_ExceptionEvent__
+
+#pragma interface
+
+#include <java/lang/Object.h>
+extern "Java"
+{
+  namespace gnu
+  {
+    namespace gcj
+    {
+      namespace jvmti
+      {
+          class ExceptionEvent;
+      }
+    }
+  }
+}
+
+class gnu::gcj::jvmti::ExceptionEvent : public ::java::lang::Object
+{
+
+  ExceptionEvent(::java::lang::Thread *, jlong, jlong, ::java::lang::Throwable *, jlong, jlong);
+public:
+  static void post_exception_event(::java::lang::Thread *, jlong, jlong, ::java::lang::Throwable *, jlong, jlong);
+  virtual void send_event();
+  virtual void check_catch();
+private:
+  jlong __attribute__((aligned(__alignof__( ::java::lang::Object)))) _throw_meth;
+  jlong _throw_loc;
+  jlong _catch_meth;
+  jlong _catch_loc;
+  ::java::lang::Thread * _thread;
+  ::java::lang::Throwable * _ex;
+  static ::java::util::WeakHashMap * _ex_map;
+public:
+  static ::java::lang::Class class$;
+};
+
+#endif // __gnu_gcj_jvmti_ExceptionEvent__
Index: libjava/gnu/gcj/jvmti/natExceptionEvent.cc
===================================================================
--- libjava/gnu/gcj/jvmti/natExceptionEvent.cc	(revision 0)
+++ libjava/gnu/gcj/jvmti/natExceptionEvent.cc	(revision 0)
@@ -0,0 +1,59 @@
+// natExceptionEvent.cc - C++ code for JVMTI Exception events
+
+/* Copyright (C) 2006  Free Software Foundation
+
+   This file is part of libgcj.
+
+This software is copyrighted work licensed under the terms of the
+Libgcj License.  Please consult the file "LIBGCJ_LICENSE" for
+details.  */
+
+#include <config.h>
+#include <gcj/cni.h>
+#include <gcj/method.h>
+#include <java-interp.h>
+#include <java-insns.h>
+#include <java-assert.h>
+#include <jvmti.h>
+#include <jvmti-int.h>
+
+#include <gnu/gcj/jvmti/ExceptionEvent.h>
+
+void
+gnu::gcj::jvmti::ExceptionEvent::send_event ()
+{
+  // Check if the exception is caught somewhere in the interpreted call stack
+  if (_catch_meth == 0 || _catch_loc == 0)
+    check_catch ();
+    
+  JNIEnv *jni = _Jv_GetCurrentJNIEnv ();
+
+  _Jv_JVMTI_PostEvent (JVMTI_EVENT_EXCEPTION, _thread, jni,
+                       reinterpret_cast<jmethodID> (_throw_meth),
+                       static_cast<jlocation> (_throw_loc), _ex,
+                       reinterpret_cast<jmethodID> (_catch_meth),
+                       static_cast<jlocation> (_catch_loc)); 
+}
+
+// This method looks up the interpreted call stack to see if the exception will
+// eventually be caught by some java method.
+void
+gnu::gcj::jvmti::ExceptionEvent::check_catch ()
+{
+  _Jv_InterpFrame *frame 
+    = reinterpret_cast<_Jv_InterpFrame *> (_thread->interp_frame);
+  
+  while ((frame=frame->next_interp))
+    {
+	  _Jv_InterpMethod *meth 
+	    = reinterpret_cast<_Jv_InterpMethod *> (frame->self);
+	  pc_t pc = frame->pc;
+		
+	  if (meth->check_handler (&pc, meth, _ex))
+	    {
+	      _catch_meth = reinterpret_cast<jlong> (meth->get_method ());
+	      _catch_loc = meth->insn_index (pc);
+          break;
+	    }
+    }
+}
Index: libjava/interpret-run.cc
===================================================================
--- libjava/interpret-run.cc	(revision 121856)
+++ libjava/interpret-run.cc	(working copy)
@@ -2540,43 +2540,38 @@
     }
   catch (java::lang::Throwable *ex)
     {
-#ifdef DIRECT_THREADED
-      void *logical_pc = (void *) ((insn_slot *) pc - 1);
-#else
-      int logical_pc = pc - 1 - meth->bytecode ();
+#ifdef DEBUG
+       // This needs to be done before the pc is changed.
+       jlong throw_loc = meth->insn_index (pc);
 #endif
-      _Jv_InterpException *exc = meth->exceptions ();
-      jclass exc_class = ex->getClass ();
-
-      for (int i = 0; i < meth->exc_count; i++)
-	{
-	  if (PCVAL (exc[i].start_pc) <= logical_pc
-	      && logical_pc < PCVAL (exc[i].end_pc))
-	    {
-#ifdef DIRECT_THREADED
-	      jclass handler = (jclass) exc[i].handler_type.p;
-#else
-	      jclass handler = NULL;
-	      if (exc[i].handler_type.i != 0)
-		handler = (_Jv_Linker::resolve_pool_entry (meth->defining_class,
-							   exc[i].handler_type.i)).clazz;
-#endif /* DIRECT_THREADED */
-
-	      if (handler == NULL || handler->isAssignableFrom (exc_class))
-		{
-
-#ifdef DIRECT_THREADED
-		  pc = (insn_slot *) exc[i].handler_pc.p;
-#else
-		  pc = meth->bytecode () + exc[i].handler_pc.i;
-#endif /* DIRECT_THREADED */
-		  sp = stack;
-		  sp++->o = ex; // Push exception.
-		  NEXT_INSN;
-		}
-	    }
-	}
-
+      // Check if the exception is handled and, if so, set the pc to the start
+      // of the appropriate catch block.
+      if (meth->check_handler (&pc, meth, ex))
+        {
+          sp = stack;
+          sp++->o = ex; // Push exception.
+#ifdef DEBUG
+          if (JVMTI_REQUESTED_EVENT (Exception))
+            {
+              using namespace gnu::gcj::jvmti;
+              jlong throw_meth = reinterpret_cast<jlong> (meth->get_method ());
+              jlong catch_loc = meth->insn_index (pc);
+              ExceptionEvent::post_exception_event (thread, throw_meth,
+                                                    throw_loc, ex, throw_meth,
+                                                    catch_loc);
+            }
+#endif
+          NEXT_INSN;
+        }
+#ifdef DEBUG
+      if (JVMTI_REQUESTED_EVENT (Exception))
+        {
+          using namespace gnu::gcj::jvmti;
+          jlong throw_meth = reinterpret_cast<jlong> (meth->get_method ());
+          ExceptionEvent::post_exception_event (thread, throw_meth, throw_loc,
+                                                ex, NULL, NULL);
+        }
+#endif
       // No handler, so re-throw.
       throw ex;
     }
Index: libjava/sources.am
===================================================================
--- libjava/sources.am	(revision 121856)
+++ libjava/sources.am	(working copy)
@@ -509,6 +509,7 @@
 gnu_gcj_jvmti_source_files = \
 gnu/gcj/jvmti/Breakpoint.java \
 gnu/gcj/jvmti/BreakpointManager.java \
+gnu/gcj/jvmti/ExceptionEvent.java \
 gnu/gcj/jvmti/Location.java
 
 gnu_gcj_jvmti_header_files = $(patsubst %.java,%.h,$(gnu_gcj_jvmti_source_files))
Index: libjava/Makefile.am
===================================================================
--- libjava/Makefile.am	(revision 121856)
+++ libjava/Makefile.am	(working copy)
@@ -826,6 +826,7 @@
 gnu/gcj/io/natSimpleSHSStream.cc \
 gnu/gcj/io/shs.cc \
 gnu/gcj/jvmti/natBreakpoint.cc \
+gnu/gcj/jvmti/natExceptionEvent.cc \
 gnu/gcj/runtime/natFinalizerThread.cc \
 gnu/gcj/runtime/natSharedLibLoader.cc \
 gnu/gcj/runtime/natSystemClassLoader.cc \