Tom Tromey wrote:
>>>>>> "Kyle" == Kyle Galloway <kgallowa@redhat.com> writes:
>>>>>>             
>
> Kyle> This patch creates a second version of _Jv_InterpMethod::run() using 
> Kyle> preprocessor directives to allow the debugging info to be collected only 
> Kyle> when the interpreter is passed the debug command line argument.  This 
> Kyle> avoids slowdowns in non-debugging cases by not gathering debug 
> Kyle> information or sending events when not debugging.
>
> When posting put a bit more rationale in for a big change like this.
>
> We talked about it offline and the idea is, we need to track the types
> of stack and local variable slots while interpreting if the debugging
> code is enabled.  However, we don't want to do this unconditionally as
> it adds overhead to interpretation.
>
> One idea was to keep the verifier-generated information around, but
> this looked costly due to increased memory use and also because we'd
> still have to do some work in the interpreter (the verifier only keeps
> type states at branch targets).
>
> So, having a second debug-only interpreter seemed more suitable.
>
> This is looking pretty good.  Some problems below.
>
> Kyle> +  //switch debug mode on or off
> Kyle> +  static void setDebug(bool debug)
> Kyle> +  {
> Kyle> +  	run = (debug) ? run_debug : run_no_debug;
> Kyle> +  }
>
> Do we really need this as a method on _Jv_InterpMethod?
> I was assuming that "debug mode" would be a global choice.
> But... you tell me, I don't know how this part is supposed to work.
>   
I've changed this now so that debug mode is set globally, and cannot be 
changed without restarting the interpreter.
> Kyle> Index: /notnfs/kgallowa/vanilla/libjava/interpret-run.cc
> Kyle> ===================================================================
> Kyle> --- /notnfs/kgallowa/vanilla/libjava/interpret-run.cc	(revision 0)
> Kyle> +++ /notnfs/kgallowa/vanilla/libjava/interpret-run.cc	(revision 0)
> Kyle> @@ -0,0 +1,2500 @@
> Kyle> +  using namespace java::lang::reflect;
>
> This file needs a copyright header.
> Also there ought to be a comment after the copyright header explaining
> that this file shouldn't be compiled but instead is only included by
> interpret.cc.
>   
Added.
> Kyle> Index: /notnfs/kgallowa/vanilla/libjava/interpret.cc
> Kyle> +#define STOREA(I) 		\
> Kyle> +DEBUG_LOCALS_INSN(I,o)	\
>
> It is our style to put a space after the ',' here.
>   
I've fixed this everywhere I saw it.
> Kyle> +void (*_Jv_InterpMethod::run) (void *, ffi_raw *, _Jv_InterpMethod *) = NULL;
>
> If debug mode is a global then I don't think we need this.  We can
> simply have multiple run_normal, etc, methods, and choose the ones we
> like when preparing the ffi closures.  That lets us avoid putting an
> extra indirect dispatch into every call to an interpreted method.
>   
This has been chnaged so that either a debug or normal version of the 4 
run_* functions are called when preparing the closure.  The function 
pointer has been removed.
> Kyle> +/* Used to keep track of local variable type
> Kyle> + * 
> Kyle> + * Possible Types:
> Kyle> + * o object
> Kyle> + * i integer
> Kyle> + * f float
> Kyle> + * l long (one slot)
> Kyle> + * m long (two slots)
> Kyle> + * d double (one slot)
> Kyle> + * e double (two slots)
> Kyle> + */
>
> I think we don't really need the 'm' and 'e' types -- on a given
> platform, we will always either do one or the other.  This knowledge
> might as well be available to other parts of libgcj.
>   
I got rid of these.
> Kyle> +#define DEBUG
> Kyle> +#undef DEBUG_LOCALS_INSN
> Kyle> +#define DEBUG_LOCALS_INSN(s,t) { localsType[s] = 't'; }
>
> This won't do what you think it does.  You'll always see a literal 't'
> in the result.
>   
I've switched to passing character constants and changed the instruction 
appropriately.

I also stripped out some changes that aren't really part of the split 
itself (they are more a beginning to debugging, I will submit in another 
patch once this change is accepted) and updated the changelog and 
included the fixed patch:

2006-07-28  Kyle Galloway  <kgallowa@redhat.com>

    * /include/java-interp.h (_Jv_InterpMethod::run_debug): New method.
    * /interpret.cc: Added placeholder for debug variable type info to 
STORE*
    macros.
    (_Jv_InterpMethod::run_debug): New method.
    (_Jv_InterpMethod::run_sync_object_debug): New method.
    (_Jv_InterpMethod::run_sync_class_debug): New method.
    (_Jv_InterpMethod::run_normal_debug): New method.
    (_Jv_InterpMethod::run_class_debug): New method.
    * /interpret-run.cc: New file, holds contents of old 
_Jv_InterpMethod::run
    method.