preliminary RFR: 8049365 - Update JDI and JDWP for modules

[serguei.spitsyn at oracle.com]

Alan,

Thanks for the comments and suggestions!


On 12/10/15 04:04, Alan Bateman wrote:
>
> 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.

Fixed in both JDI and JDWP.


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

Fixed.

>
> 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.

The empty string is returned in the implementation.
Would it be Ok to update the jdwp spec with this?


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

Fixed.


>
>
> 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, ...

Sure.


>
>
> 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.

Ok, will try it.


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

Ok.

>
> 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.

Ok.


>
> In ReferenceTypeImpl then I assume isModuleCached is not needed.

Not sure, I understand this. Why?
It seems there is some confusion here.
This flag is similar to the flag isClassLoaderCached.


> Maybe you have this to the JDWP command when connected to a target VM 
> that is JDK 8 or older?

No.
For the older JDK the UnsupportedOperationException is expected to be 
thrown.


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

Fixed all places in the JDI update.


>
> ModuleImpl - I assume ref can be final.

Fixed.


> 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.

I'll ask Dmitry as he covers the SA.
He had some plans on the Jigsaw update.


>
> 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.

Yes.
I'd like to add more sophisticated self-checks to the existing test.
More negative tests ...
Good JDWP and JVMTI tests are needed as well.


Thanks,
Serguei



>
> -Alan