preliminary RFR: 8049365 - Update JDI and JDWP for modules

[harold seigel]

Hi Serguei,

Looks good.  Just a few comments.

1. Should jni_GetAllModules() contain the DTrace macros that other JNI 
entries have?

2. Can you add a similar GetAllModules() function to jniCheck.cpp ?

Thanks, Harold

On 12/10/2015 7:04 AM, 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.
>
> 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