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

[serguei.spitsyn at oracle.com]

On 1/21/16 04:51, Alan Bateman wrote:
> 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.

Ok
I saw the discussion but have no strong opinion yet.
Changing to JVMTI_VERSION_9 will make it more consistent though.


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

I hope so.


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

I could easily fix it now but it is Ok to file a separate bug.


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

Ok

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

Ok

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

Will check.

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

Nice catch.
Will pull an update.


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

Ok


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

Ok

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

Will check.

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

Agreed.
Will provide an update in the next review round.


Thanks,
Serguei
>
> -Alan