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

[serguei.spitsyn at oracle.com]

Hi Alan,

On 1/23/16 02:35, Alan Bateman wrote:
> 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.

Added JVMTI_VERSION_9.
I thought that the JVMTI_VERSION_9 will be need when we release the JDK 10.
I've not found where it must be used so that it does not help much.


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


I've not found anything wrong yet.
But thank you for pointing it out.


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


Done


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


My plan was to run all the relevant tests before the push.
Found and fixed a couple of spots in the VirtualMachineImple.java.
I had to fix a couple of jdk_jdi tests as well.
Now the tests (including nsk.jdi and jdk_jdi) are all passed as in the 
version without my fix.



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


Agreed.
Thank you for filing new bug.



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


Good suggestion, thanks!
Done.



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

Nice catch - fixed.

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


Fixed.
Lois pointed out to a similar place in the JVMTI update.


>
> That's all I have for now.

Thanks a lot!
Will send new webrevs shortly.

Thanks,
Serguei


>
> -Alan.
>
>
>
>