[RFA] JMTI GetLocal____ methods

[Kyle Galloway]

Tom Tromey wrote:
>>>>>> "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?
>   
Yeah my mailer has a way or warping the Changelogs, something to do with 
how long the lines it lets me type are and how long the lines it sends are.
> 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.
>   
I've changed this now so it marks N+1 with 'x' instead of the long type 
in N. I also check when actually getting the variables to see if the 
slot being accessed is the lower half of a long type.
> Kyle> +#include  "interpret-run.cc"
>
> Spurious addition of a space.
>   
Fixed.
> 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.
>   
Your definitely right here, I've totally removed this check. Since you 
pass in a jobject anything not an object will cause a compile-time error 
in the caller.
> 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.
>   
I've fixed this by expanding the check to check slot+1 >= max_locals if 
there is a long type in use.
> 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.
>   
I now allocate a _Jv_InterpFrame then have getLocalFrame copy into it.
> [...]
> 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*.
And lastly, this is gone as well.

New patch is attached.

Thanks,
Kyle

-------------- next part --------------
A non-text attachment was scrubbed...
Name: getlocalvars-jvmti.patch
Type: text/x-patch
Size: 11371 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/java-patches/attachments/20070216/66ffb8c6/attachment.bin>