Round #1: RFR: 8049365 - Update JDI and JDWP for modules

[Alan Bateman]

On 23/12/2015 06:24, serguei.spitsyn at oracle.com wrote:
> Please, review this initial fix for the Jigsaw Bill milestone task:
> https://bugs.openjdk.java.net/browse/JDK-8049365
>
>
> Jdk webrev:
> http://cr.openjdk.java.net/~sspitsyn/webrevs/2015/jdk/8049365-Jigsaw-jdk.1/
>
>
> Hotspot webrev:
> http://cr.openjdk.java.net/~sspitsyn/webrevs/2015/hotspot/8049365-Jigsaw-hs.1/
>
>
> Summary:
>   This round includes most of the changes suggested or discussed by 
> Alan in the round #0:
>     - The version 1.9 is replaced version
>     - The Module interface is replaced with the ModuleReference 
> interface that extends the ObjectReference.
>     - The argument name "target" in the CanRead is replaced with 
> "source" to make it consistent with
>        the class java.lang.reflect.Module.
>     - The public field TRACE_MODULES has been removed.
>     - Some other small changes.
>        I hope, all comments are addressed. But, please, let me know if 
> anything is missed.
>
>
>   This round does not include the corresponding update of the SA-JDI.
>   I'm suggesting to separate and postpone this part for now to speed 
> up the main part.
>   Another reason for it is that the SA-JDI update depends on possible 
> VM changes.
>
>   This round adds new capability "can_get_modules_info" that is 
> appeared on all levels:
>      JVMTI, JDWP and JDI.
No problem updating SA-JDI at a later time, we just need to make sure it 
builds without issues.

I looked through the updated webrevs and it looks quite good. A few 
comments:

It's not clear to me that a new JVM TI capability is required. I agree 
that JDWP and JDI need a canXXX command/method but if we rev JVM TI to 
version 9 then the VM supports modules and so a capability shouldn't be 
needed.

The JDWP command is currently "canGetModulesInfo", it might be better as 
"canGetModuleInfo". Ditto for the method on VirtualMachine.

The JDWP agent uses JNI to upcall to Module::getClassLoader and 
Module::canRead, we might consider adding JNI or JVM TI functions in the 
future and avoid this.

VirtualMachine::allModules - there's a spurious <UL> in the javadoc.

ModuleReference javadoc currently contains ".. has a read edge with the 
source ModuleReference". We'll need to do a bit of wordsmithing on the 
javadoc but for this one then starting out with " .. if the module reads 
the source module" would be clearer.

InvalidModuleException - the copyright date says 1998 :-)

I see you have a basic test. The checkClassLoader method needs comments 
as it's not clear what it is testing. We'll need to add further tests to 
cover all the scenarios, the unnamed module case for example.

-Alan