From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 2451 invoked by alias); 15 Feb 2007 18:35:36 -0000 Received: (qmail 2441 invoked by uid 22791); 15 Feb 2007 18:35:34 -0000 X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (66.187.233.31) by sourceware.org (qpsmtpd/0.31) with ESMTP; Thu, 15 Feb 2007 18:35:27 +0000 Received: from int-mx1.corp.redhat.com (int-mx1.corp.redhat.com [172.16.52.254]) by mx1.redhat.com (8.13.1/8.13.1) with ESMTP id l1FIZO25025118 for ; Thu, 15 Feb 2007 13:35:24 -0500 Received: from pobox.toronto.redhat.com (pobox.toronto.redhat.com [172.16.14.4]) by int-mx1.corp.redhat.com (8.13.1/8.13.1) with ESMTP id l1FIZNe3019013; Thu, 15 Feb 2007 13:35:23 -0500 Received: from [172.16.14.104] (to-dhcp4.toronto.redhat.com [172.16.14.104]) by pobox.toronto.redhat.com (8.12.11.20060308/8.12.11) with ESMTP id l1FIZNXm002106; Thu, 15 Feb 2007 13:35:23 -0500 Message-ID: <45D4A7EB.4050202@redhat.com> Date: Thu, 15 Feb 2007 18:35:00 -0000 From: Kyle Galloway User-Agent: Thunderbird 1.5.0.9 (X11/20070103) MIME-Version: 1.0 To: tromey@redhat.com CC: Java Patch List Subject: Re: [RFA] JMTI Exception Events References: <45D383B0.8010807@redhat.com> In-Reply-To: Content-Type: multipart/mixed; boundary="------------080506090408000702050009" X-IsSubscribed: yes Mailing-List: contact java-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: java-patches-owner@gcc.gnu.org X-SW-Source: 2007-q1/txt/msg00499.txt.bz2 This is a multi-part message in MIME format. --------------080506090408000702050009 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Transfer-Encoding: 7bit Content-length: 3229 Tom Tromey wrote: >>>>>> "Kyle" == Kyle Galloway 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 _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 (); > 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 --------------080506090408000702050009 Content-Type: text/x-patch; name="exceptions-jvmti.patch" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="exceptions-jvmti.patch" Content-length: 13760 Index: libjava/interpret.cc =================================================================== --- libjava/interpret.cc (revision 121856) +++ libjava/interpret.cc (working copy) @@ -43,6 +43,7 @@ #include #include #include +#include #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 _ex_map = new WeakHashMap (); + + /** + * 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 +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 +#include +#include +#include +#include +#include +#include +#include + +#include + +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 (_throw_meth), + static_cast (_throw_loc), _ex, + reinterpret_cast (_catch_meth), + static_cast (_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 (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 (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 (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 \ --------------080506090408000702050009--