Hi Alan,
On 1/23/16 02:35, Alan Bateman wrote:
On 22/01/2016 22:16, serguei.spit...@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.
