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

[Alan Bateman]

On 20/01/2016 22:35, serguei.spitsyn at oracle.com wrote:
> :
>
> Jdk webrev:
> http://cr.openjdk.java.net/~sspitsyn/webrevs/2016/jdk/8049365-Jigsaw-jdk.2/
>
> Hotspot webrev:
> http://cr.openjdk.java.net/~sspitsyn/webrevs/2016/hotspot/8049365-Jigsaw-hs.2/
>
>
jvmti.xml - the module catagory has since="9" but the JVM TI spec 
version rev's to 1.3.2. I started a thread a few weeks ago on whether 
JNI and JVM TI should move to major version 9 and just track the Java SE 
version going forward. I don't think we have closure on that discussion 
yet but for now then I suggest we move to JVMTI_VERSION_9 on the 
assumption that this is the way forward.

The GetAllModules implementation is okay for now and I believe Lois will 
replace JvmtiModuleClosure once your changes are in and she gets time to 
replace it.

Now to the jdk repo ...

We seem to have an issue with the generated jvmti.h generating a GPL 
copyright header. This means that copying jvmti.h into the jdk repo will 
change it from GPL + "Classpath" exception to pure GPL. I'll create a 
separate bug for this, this seems to be technical debt that we have a 
copy of jvmti.h in the jdk repo but fixing that issue means we need a 
jvmti.h with the right copyright because that is what goes into the JDK 
include directory.

jdi.ReferenceType - I didn't generate the javadoc with your patch but it 
looks like the "Not all target ..." note will end up in the @return tag 
and I assume should move up. There is also a stray <UL> after the @throws.

jdi.VirtualMachine - this has the same issue with the "Not all target 
..." note. In the canGetModuleInfo() method then I assume the @see 
should be moved to the end of the class description.

Can you check allModules in VirtualMachineImpl.c as it doesn't look 
quite right when GetAllModules fails, is there a return missing as 
otherwise it will write the module count and attempt to deallocate NULL.

@jdk.Exported has been removed in JDK 9 and that removal has got to 
jake. I assume your forest is a bit behind.

jdi.ModuleReference
- as this code is new then it would be good to use {@code  ..} instead 
of <code></code>.
- I don't think name, classLoader or canRead can throw UOE so this 
exception can be removed from the javadoc.
- the class description reads "A module that currently exists in the 
target VM". Looking at the other JDI classes then it might be more 
consistent to use "A module in the target VM".

tools.jdi.ModuleReferenceImpl
- if change the fields to volatile then we can avoid these methods 
needing to be synchronized.
- a minor nit is that it would be good to avoid overly long lines as 
it's a pain to have v. long lines when doing side-by-side diffs.

tools.jdi.VirtualMachineImpl
- can modulesByID be Map<Long, ModuleReference>, I can't quite tell why 
the value needs to be the impl type.
- It looks like vmMajorVersion will return 0 when the target VM is JDK 8 
or older, is that right? I guess that is okay because we only care about 
 >= 9 but we will need to look at this closely.

As per previous mails then I think we'll need additional tests in this 
area. It would be good to create another issue in JIRA to track that work.

-Alan