Re: [RFA] JMTI GetLocal____ methods

[Tom Tromey <tromey at redhat dot com>]

>>>>> "Kyle" == Kyle Galloway <kgallowa@redhat.com> writes:

Kyle> This patch implements the JVMTI side functions to Get and Set local
Kyle> variables.  getLocalFrame is used by all of these methods to do the
Kyle> common error checking and frame retrieval that is needed in every case.
Kyle> If it does not return an error, it returns a newly allocated copy of
Kyle> the frame object containing pointers to the actual local variable and
Kyle> local variable type info.  The Get/SetLocal___ methods then gt/set the
Kyle> variable, free the frame structure and return.

Thanks for the explanation.

Kyle> 	* interpret-run.cc: Add local variable info to frame in the
Kyle> debug interpreter.

Bad wrapping in the ChangeLog entry.  Or maybe your mailer?

Kyle>      DEBUG_LOCALS_INSN (I, 'l');			\
Kyle> +    DEBUG_LOCALS_INSN (I+1, 'l');               \

Good catch.

But, when we assign a long (or double) to slot N, I think it is better
to mark slot N+1 as invalid.  First, this prevents users from
incorrectly treating (combined) slots N+1 and N+2 as a wide type.
Second, if we later assign a narrow type (eg int) to slot N, slot N+1
will already be correctly marked as invalid.

Kyle> +#include  "interpret-run.cc"

Spurious addition of a space.

Kyle> +#define OBJECT_CHECK_VALID(Aobject)
Kyle> +      if (!java::lang::Object::class$.isAssignableFrom (&(Aobject->class$))) \

Two bugs here.

First, Aobject->class$ most likely should be written Aobject->getClass().
Since class$ is a static field, as written this will pick up the
class$ of the declared type of Aobject, which will always be Object.

Second, and more importantly, I don't think this check is useful.  All
object are, by definition, assignable to Object.

Kyle> +  if (slot < 0 || slot >= max_locals)
Kyle> +    return JVMTI_ERROR_INVALID_SLOT;

If the caller is dealing with a wide type, this must check
'slot + 1 >= max_locals'.  I don't see this check being done anywhere.

Kyle> +  *iframe = reinterpret_cast<_Jv_InterpFrame *> 
Kyle> +              (_Jv_MallocUnchecked (sizeof (_Jv_InterpFrame)));
Kyle> +  memcpy (*iframe, tmp_iframe, sizeof (_Jv_InterpFrame));

It is better not to allocate a new frame only to free it in the
caller.  Instead let the caller pass in a pointer to an unfilled frame
or something like that.

[...]
Kyle> +                          jobject value)
[...]
Kyle> +  java::lang::Object *obj = reinterpret_cast<java::lang::Object *> (value);

No need for a cast here.  jobject is just a typedef for
java::lang::Object*.
 
Tom