Re: [RFA] JMTI GetLocal____ methods

[Tom Tromey <tromey at redhat dot com>]

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

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

I should have mentioned earlier -- put spaces around (most) operators.
So, 'I + 1'.

Kyle> +#define DEBUG_LOCALS_INSN(s, t)  \
Kyle> +do  \
Kyle> +  {  \
Kyle> +    frame_desc.locals_type[s] = t; \
Kyle> +  }  \
Kyle> +while (0)
 
There should be some indentation on the do-while here.  I think 2
spaces at the start of each line (other than the #define) will do it.

Kyle> +  // The second slot taken up by a long type is marked as type 'x' meaning it
Kyle> +  // is not valid for access since it holds only the 4 low bytes of the value.
Kyle> +  if (tmp_iframe->locals_type[slot] == 'x')
Kyle> +    return JVMTI_ERROR_INVALID_SLOT;

It occurs to me now that slots are also invalid when created.

Kyle> +  _Jv_InterpFrame *frame = reinterpret_cast<_Jv_InterpFrame *>
Kyle> +                             (_Jv_MallocUnchecked (sizeof (_Jv_InterpFrame)));
Kyle> +  jvmtiError jerr = getLocalFrame (env, thread, depth, slot, 'o', frame);

The idea behind not allocating in getLocalFrame is to avoid dynamic
memory allocation, and instead to stack allocate these temporary frame
objects.

Best would be to pass in a '_Jv_InterpFrame **' and have getLocalFrame
return the frame pointer, not a copy of the frame.

  _Jv_InterpFrame *frame;
  jvmtiError jerr = getLocalFrame (env, thread, depth, slot, 'o', &frame);

.. and replace:

Kyle> +  memcpy (iframe, tmp_iframe, sizeof (_Jv_InterpFrame));

.. with '*iframe = tmp_iframe'

Kyle> +  val.ia[1] = frame->locals[slot+1].ia[0];

Spaces around the '+'.  There's a few of these.

Kyle> +#ifdef DEBUG  
Kyle> +  frame_desc.locals = locals;
Kyle> +  frame_desc.locals_type 
Kyle> +    = reinterpret_cast<char *> (_Jv_AllocBytes (meth->max_locals));
Kyle> +#endif
 
Stack allocate here, and initialize to be invalid.

  char locals_type[meth->max_locals];
  memset (locals_type, 'x', meth->max_locals);
  frame_desc.locals_type = locals_type;

This should always be ok since locals_type should not escape (except
temporarily, while the frame is active).

Tom