preliminary RFR: 8049365 - Update JDI and JDWP for modules

serguei.spitsyn at serguei.spitsyn at
Thu Dec 10 20:44:25 UTC 2015


Thanks for the comments and suggestions!

On 12/10/15 04:04, Alan Bateman wrote:
> On 10/12/2015 08:03, serguei.spitsyn at wrote:
>> Jdk webrev:
>> Hotspot webrev:
>> 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 ...".


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


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

Ok, will try it.

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

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?

For the older JDK the UnsupportedOperationException is expected to be 

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


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

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.


> -Alan

More information about the jigsaw-dev mailing list