This is the mail archive of the
java-patches@gcc.gnu.org
mailing list for the Java project.
Re: [RFA] JDWP LocalVariables
- From: Tom Tromey <tromey at redhat dot com>
- To: Kyle Galloway <kgallowa at redhat dot com>
- Cc: GCJ-patches <java-patches at gcc dot gnu dot org>
- Date: 30 Mar 2007 14:04:27 -0600
- Subject: Re: [RFA] JDWP LocalVariables
- References: <460AC6A9.1050509@redhat.com>
- Reply-to: tromey at redhat dot com
>>>>> "Kyle" == Kyle Galloway <kgallowa@redhat.com> writes:
Kyle> This patch implements the functions to allow JDWP to get and set
Kyle> local variables through JVMTI.
A few buglets in here but overall it is looking good.
Kyle> +void
Kyle> +checkJVMTIError (jvmtiEnv *env, jthread thread, jvmtiError jerr, jint slot,
Kyle> + jbyte sig)
A helper method like this, which isn't a class member, should be
'static'. There are several instances of this.
Kyle> +void
Kyle> +setLongJVMTI (jvmtiEnv *env, jthread thread, jint slot, jint depth, jbyte sig,
Kyle> + jlong value)
Kyle> +{
Kyle> + jvmtiError jerr = env->SetLocalInt (thread, depth, slot, value);
Cut-and-paste bug? Should be SetLocalLong.
This also affects the Float and Double cases.
Kyle> + // If the method is non-static, we need to set the type for the "this" pointer.
Kyle> + if ((method->accflags & java::lang::reflect::Modifier::STATIC) == 0)
Kyle> + {
Kyle> + frame_desc.locals_type[0] = 'o';
Kyle> + type_ctr++;
Kyle> + }
You might as well hoist this code, from later on, into this 'if':
Kyle> frame_desc.obj_ptr = locals[0].o;
Kyle> + for (int i = 0; signature[i] != ')' && i <= sig_len; i++)
Start with 'i = 1' to skip the '('.
Kyle> + else if (signature[i] == '[')
Kyle> [...]
Kyle> +
Kyle> + // Check for an object array
Kyle> + if (signature[i] == 'L')
Kyle> + {
Kyle> + while (signature[i] != ';')
Kyle> + i++;
Kyle> + }
Kyle> + continue;
I think this won't do the right thing for a primitive array, like '[[I'.
In this case we need to skip the 'I' as well, so I think we need 'else ++i'
in there. What do you think?
Tom