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

[Alan Bateman]

On 22/01/2016 22:16, serguei.spitsyn at oracle.com wrote:
> :
>
> Jdk webrev:
> cr.openjdk.java.net/~sspitsyn/webrevs/2016/jdk/8049365-Jigsaw-jdk.3/
>
> Hotspot webrev:
> http://cr.openjdk.java.net/~sspitsyn/webrevs/2016/hotspot/8049365-Jigsaw-hs.3/

Good to see the JVMTI_VERSION rev'ed but I suspect that more is needed.

In jvmtiH.xsl then I assume we need to add JVMTI_VERSION_9.

In JvmtiExport::get_jvmti_interface then I assume it needs to handle a 
major version of 9, otherwise an agent requesting a JVMTI_VERSION_9 env 
will fail.

In the JDWP agent then compatible_versions (debugInit.c) has a matrix of 
compatible versions that should be checked. I don't see any issues with 
it but another set of eyes would help verify that.

One other thing about the version update is that I assume this should be 
dropped from jvmti.xml:
<change date="1 July 2015" version="1.3.1">
The reason being the jake history will be dropped when we bring the 
module system into JDK 9.

Since we are close then I tried the patches locally and ran the jdk_jdi 
test group. Unfortunately there are quite a few test failures and hangs. 
Mostly it seems to be JDI methods throwing UOE because the target VM is 
considered a JVMTI 1.0. I don't know if you've seen the same failures 
but there must be another place where the version isn't handled correctly.

Back to my whining about the copyright header in jvmti.h :-) I think 
we'll need to manually fix the header in the checked-in version for now. 
That should go away once JDK-8147943 and JDK-8147943 are addressed in JDK 9.

A few other comments:

ReferenceType::module is specified to throw UOE when not supported by 
the target VM. The temporary update to the SA class returns null. Not a 
big deal for now but would be better to have change Reference::module to 
be default method that throws UOE. That way you could drop the temporary 
addition to the SA class. Same thing for VirtualMachine::allModules.

ModuleReferenceImpl.c - L56 needs to set name to NULL as otherwise 
jvmtiDeallocate will attempt to deallocate an uninitialized name.

Minor comment on VirtualMachineImpl.java where modulesByID is 
initialized sized as 20. Do you need to specify a capacity here? There 
are 70+ modules in the JDK so this will immediately need to re-size.

That's all I have for now.

-Alan.