preliminary RFR: 8049365 - Update JDI and JDWP for modules

[Alan Bateman]

On 10/12/2015 08:03, serguei.spitsyn at oracle.com wrote:
>
> Jdk webrev:
> http://cr.openjdk.java.net/~sspitsyn/webrevs/2015/jdk/8049365-Jigsaw-jdk.0/
>
> Hotspot webrev:
> http://cr.openjdk.java.net/~sspitsyn/webrevs/2015/hotspot/8049365-Jigsaw-hs.0/
>
>
> Summary:
>
>   It is expected that the JDI and JDWP update for modules will have 
> several iterations.
>   This is the initial one, and it introduces a very minimal functionality.
>   The main purpose of this preliminary review is to make sure the JDI 
> and JDWP update
>   for modules goes in a right direction and has nothing obviously wrong.

I went through the changes to the JDWP spec + additions to jdwpgen in 
this iteration and it looks quite good. A few comments:

1. I don't know if JEP 223 considered the JDWP version but I will assume 
this will move to 9 instead of 1.9.

2. In the Module command then the description would be clearer as "... 
that this reference type ...".

3. In the Module command set then we'll need to decide the reply to the 
Name command for the case that the module is an unnamed module. There is 
also an open issue for the runtime API too.

4. Also in the Module command set then the CanRead targetModule should 
be "source" (not target) to get consistency with the runtime API.


As you mentioned in your mail, I think the GetAllModules will need to 
move to JVM TI so that it's consistent with GetAllClasses, 
GetAllThreads, ...


I also went through the JDI changes. The type hierarchy in JDI is quite 
deep and is has a convention for naming that we need to follow too. 
Looking at it now then it looks like it should be ModuleReference 
extends ObjectReference. That would give us consistency with other 
mirrors that are object references (ThreadReference and 
ClassLoaderReference for example). We don't rev JDI often enough to keep 
the hierarchy and naming convention in our heads so good to have more 
eyes on this.

ReferenceType::module looks right, we'll just need to iterate on the 
javadoc.

VirtualMachine::allModules looks right. Note that the TRACE_* fields are 
public and part of the JDI API so we'll need to decide whether 
TRACE_MODULES is important or not.

In ReferenceTypeImpl then I assume isModuleCached is not needed. Maybe 
you have this to the JDWP command when connected to a target VM that is 
JDK 8 or older?

VirtualMachineImpl - this might be the time to change to 9 rather than 1.9.

ModuleImpl - I assume ref can be final.


Have you thought about SA yet? I can't recall if it is compiled with the 
boot JDK or will be compiled against the newly built jdk.jdi module. If 
the later then I assume that SA will need updates. If the former then I 
assume we will have issues with boot cycle builds.

Tests. I see you have a basic test but I assume we will need to find 
time to develop a lot more tests for this update.

-Alan