Hi Alan,
Just wanted to provide some detailed update on your comments below.
On 1/21/16 04:51, Alan Bateman wrote:
On 20/01/2016 22:35, serguei.spit...@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.
Fixed
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.
Left it alone for now.
Thank you for filing new bug on this.
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.
Fixed
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.
Fixed
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.
Nice catch - fixed.
@jdk.Exported has been removed in JDK 9 and that removal has got to
jake. I assume your forest is a bit behind.
Pulled an update and removed the @jdk.Exported.
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".
Nice catch about the UOE.
Resolved all 3 comments.
tools.jdi.ModuleReferenceImpl
- if change the fields to volatile then we can avoid these methods
needing to be synchronized.
Currently left it alone.
I feel that making the methods synchronized is better for clarity/safety.
It prevents unnecessary concurrent JDWP commands execution.
I do not think, it is very important however and ready to change to
volatile if you want.
- 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.
Fixed
tools.jdi.VirtualMachineImpl
- can modulesByID be Map<Long, ModuleReference>, I can't quite tell
why the value needs to be the impl type.
Nice suggestion - done.
I had some compile-time error originally but found how to resolve it now.
- 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.
Yes, exactly.
My guess was also that it is Ok.
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.
I've filed two new tasks:
add more tests: https://bugs.openjdk.java.net/browse/JDK-8148103
update SA-JDI: https://bugs.openjdk.java.net/browse/JDK-8148106
Thanks,
Serguei
-Alan
