[cp-patches] Re: [RFA/JDWP] VirtualMachineCommandSet.java

[Aaron Luchko]

On Tue, 2005-07-12 at 16:03 -0400, Bryce McKinlay wrote:
> Aaron Luchko wrote:
> 
> >Ok this is a touch more complicated but not too bad, there's also a
> >couple possibly minor issues. This implements the VirtualMachine
> >CommandSet
> >http://java.sun.com/j2se/1.5.0/docs/guide/jpda/jdwp/jdwp-protocol.html#JDWP_VirtualMachine
> >
> >The VERSION command is the first one that is a bit of an interpretation
> >http://java.sun.com/j2se/1.5.0/docs/guide/jpda/jdwp/jdwp-protocol.html#JDWP_VirtualMachine_Version
> >All the fields are pretty direct except for the first one.
> >"description: Text information on the VM version"
> >Sun along with IBM and BEA (they all actually return the exact same
> >response for this command) return
> >"Java Debug Wire Protocol (Reference Implementation) version 1.4?JVM
> >Debug Interface version 1.3?JVM version 1.4.2_06 (Java HotSpot(TM)
> >Client VM' mixed mode)"
> >  
> >
> IBM and BEA claim to be HotSpot(TM)? That is odd - sounds like a bug in 
> Sun's JDWP implementation if so.

Yeah, I figured they probably just hard-coded the string in there :)

> 
> >Which seems pretty generic/non-standard so I just did 
> >
> >Properties props = System.getProperties();
> >
> >// The description field is pretty loosely defined
> >String description = "JVM version " + props.getProperty("java.vm.name")
> >                     + " " + props.getProperty("java.vm.version") + " "
> >                     + props.getProperty("java.version");
> >
> >which for my rpm installed gcj gave,
> >"JVM version GNU libgcj 4.0.0 20050622 (Red Hat 4.0.0-13) 1.4.2"
> >  
> >
> 
> Looks good, but should the JDWP version be incorporated into this string 
> as well?

Why not, I'm guessing this is just meant to be displayed to the user by
some debuggers.

Changed description to
"JDWP version 1.4, JVM version GNU libgcj 4.0.0 20050622 (Red Hat
4.0.0-13) 1.4.2"

> 
> >The next issue, the commands 
> >AllThreads and TopLevelThreadGroups
> >http://java.sun.com/j2se/1.5.0/docs/guide/jpda/jdwp/jdwp-protocol.html#JDWP_VirtualMachine_Version
> >
> >Rely on two things:
> >1. There can only be 1 top level thread group.
> >2. The aforesaid top level group can be attained via recursively going
> >group.getParent().
> >
> >If those assumptions are incorrect then things could get more
> >complicated.
> >  
> >
> 
> It is correct that there can only be 1 top level threadgroup for user 
> code (since a ThreadGroup cannot be created without a parent), but 
> perhaps VMs could implement hidden threadgroups for system code? The 
> name of the command, "TopLevelThreadGroups" seems to suggest this.

Not sure. I ran a little debugger I wrote against ibm-jvm with a
HelloWorld app and it only reported a single thread group.

Another concern along this line I have is whether we should return our
jdwp threads to the debugger. I implemented executeAllThreads to filter
them out but the spec is vague.
"The returned list contains threads created through java.lang.Thread,
all native threads attached to the target VM through JNI, and system
threads created by the target VM"
I tried allThreads against ibm and it reported 3 threads for the same
HelloWorld app. What these threads are I'm unsure.


> >There's also the theoretical concern if these are true of a user run
> >program being able to find our Jdwp threads via the same way I do
> >executeAllTheads().
> >  
> >
> 
> Access to the thread lists is a secure operation - untrusted code will 
> not be able to access these threads if the AccessController's security 
> policy doesn't allow it. Likewise, it would be very silly for an 
> application to rely on only certain threads existing, VMs make threads 
> to do things "behind the back" of the application all the time.
 
cool

> >release/holdEvents
> >http://java.sun.com/j2se/1.5.0/docs/guide/jpda/jdwp/jdwp-protocol.html#JDWP_VirtualMachine_HoldEvents
> >should be fine being left on the back burner until later.
> >  
> >
> OK. It looks like to implement this, you'll need to be able to queue 
> outgoing packets after all.
> 
> >2005-07-12  Aaron Luchko  <address@hidden>
> >
> >        * gnu/classpath/jdwp/processor/VirtualMachineCommandSet.java:
> >        New file.
> >  
> >
> 
> You could add a brief description of what the file does to the ChangeLog 
> entry, if you like.
> 
> >+    // The description field is pretty loosely defined
> >+    String description = "JVM version " + props.getProperty("java.vm.name")
> >+                         + " " + props.getProperty("java.vm.version") + " "
> >+                         + props.getProperty("java.version");
> >+    int jdwpMajor = 1; // Get from jvm?
> >+    int jdwpMinor = 5; // Get from jvm?
> >  
> >
> Should this version be 1.4? Also, since this is the implementation, the 
> JDWP code itself probably know better than anywhere else what JDWP 
> version is implemented.

The // Get from jvm? comment is a bit of a red herring from an old
version I should of removed. I'll toss them in JdwpConstants. The reason
I did 1.5 instead of 1.4 is I've been working from the 1.5 spec and
implementing the 1.5 commands where possible though come to think of it
it probably makes more sense to claim 1.4 in case some debuggers mistake
a 1.5 jdwp for a 1.5 java.

> 
> >+    String vmVersion = props.getProperty("java.version"); // Get from jvm?
> >  
> >
> The java.version property comes from the VM, at least in libgcj, so it 
> should be considered the canonical source.

Oops, another obsolete comment, the spec pretty much demands we use that
"Target VM JRE version, as in the java.version property" :)
> 
> >+  private void executeClassesBySignature(ByteBuffer bb, DataOutputStream os)
> >+    throws JdwpException, IOException
> >+  {
> >+    String sig = JdwpString.readString(bb);
> >+
> >+    ArrayList allLoadedClasses, allMatchingClasses;
> >+
> >+    // This will be a vector of all loaded Classes
> >+    allLoadedClasses = vm.getAllLoadedClasses();
> >  
> >
> 
> This is another example of something that could probably be implemented 
> a lot more efficiently in the VM rather than in Java. The problem is 
> that the VM will have to enumerate the list of all loaded classes into 
> an array/ArrayList/etc, which means quite a bit of memory usage and copying.
> 
> If it must be done in Java then it might make sense to have the vm 
> interface return an Iterator rather than a List or ArrayList. This way, 
> a VM which maintains its own internal class lists anyway could implement 
> an iterator around them instead of having to make a copy.

I'd like to keep it in Java just for portability issues and reducing
calls to the VM. As to the Iterator I had to add an extra method,
getAllLoadedClassesCount() to the VM and disable the garbage collection
while making the two calls so we don't end up with fewer classes than we
can write. In the long run it might be better to return an Object which
pairs the count and the Iterator rather than shutting down garbage
collection but this seems better for the moment.

> >+  private void executeDispose(ByteBuffer bb, DataOutputStream os)
> >+    throws JdwpException
> >+  {
> >+    // TODO: resumeAllThreads isn't sufficient as a thread may have been
> >+    // suspended multiple times, we likely need a way to keep track of how 
> >many
> >+    // times a thread has been suspended or else a stronger resume method 
> >for
> >+    // this purpose
> >+    vm.resumeAllThreadsExcept(jdwp.getJdwpThreadGroup());
> >+
> >+    // Simply shutting down the jdwp layer will take care of the rest of the
> >+    // shutdown other than disabling debugging in the VM
> >+    vm.disableDebugging();
> >+  }
> >+
> >+  private void executeIDsizes(ByteBuffer bb, DataOutputStream os)
> >+    throws JdwpException, IOException
> >+  {
> >+    ObjectId oid = new ObjectId();
> >+    os.writeInt(oid.size()); // fieldId
> >+    os.writeInt(oid.size()); // methodId
> >+    os.writeInt(oid.size()); // objectId
> >+    os.writeInt(new ReferenceTypeId((byte) 0x00).size()); // referenceTypeId
> >+    os.writeInt(oid.size()); // frameId
> >+  }
> >+
> >+  private void executeSuspend(ByteBuffer bb, DataOutputStream os)
> >+    throws JdwpException
> >+  {
> >+    vm.suspendAllThreadsExcept(jdwp.getJdwpThreadGroup());
> >+  }
> >  
> >
> 
> Unfortunately, I think its going to be hard to make the JDWP code 
> completely robust in the face of possible deadlocks caused by user code 
> holding locks while suspended. For example, suppose that a user thread 
> is in the middle of initializing some class that is also (directly or 
> indirectly) used by JDWP. That thread will hold a lock on that class, 
> and the JDWP will deadlock. We already try to minimize the number of 
> user-visible locks used by libgcj in order to try and avoid the 
> possibility of this, but when you allow thread suspension it becomes 
> much more difficult. Whether this will be a real problem that is 
> encountered in practice, I'm not sure.

Hrm, I'll keep that in the back of my mind, hopefully we'll think of a
nice solution.

> 
> Again, these are just a few things to think about. Patch is OK to commit 
> with the version ID and ChangeLog changes I mentioned.

Cool though from a discussion with Keith I've got a few more changes as
well. I've changed getRootThreadGroop into a loop (from recursion).
Removed the ThreadGroup hook from Jdwp (getting it from the current
thread instead), and threw a NotImplemented for executeDispose since
we'll need a better idea about where everything ends up wrt the vm
before we can do a clean shutdown. 

thanks,
Aaron

ChangeLog

2005-07-12  Aaron Luchko  <address@hidden>

        * gnu/classpath/jdwp/processor/VirtualMachineCommandSet.java:
        Implemented VirtualMachine Command Set.

jdwpVirtualMachine2.patch
Description: Text Data