preliminary RFR: 8049365 - Update JDI and JDWP for modules
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Thu Dec 10 20:44:25 UTC 2015
Thanks for the comments and suggestions!
On 12/10/15 04:04, Alan Bateman wrote:
> On 10/12/2015 08:03, serguei.spitsyn at oracle.com wrote:
>> Jdk webrev:
>> Hotspot webrev:
>> It is expected that the JDI and JDWP update for modules will have
>> several iterations.
>> This is the initial one, and it introduces a very minimal
>> The main purpose of this preliminary review is to make sure the JDI
>> and JDWP update
>> for modules goes in a right direction and has nothing obviously wrong.
> I went through the changes to the JDWP spec + additions to jdwpgen in
> this iteration and it looks quite good. A few comments:
> 1. I don't know if JEP 223 considered the JDWP version but I will
> assume this will move to 9 instead of 1.9.
Fixed in both JDI and JDWP.
> 2. In the Module command then the description would be clearer as "...
> that this reference type ...".
> 3. In the Module command set then we'll need to decide the reply to
> the Name command for the case that the module is an unnamed module.
> There is also an open issue for the runtime API too.
The empty string is returned in the implementation.
Would it be Ok to update the jdwp spec with this?
> 4. Also in the Module command set then the CanRead targetModule should
> be "source" (not target) to get consistency with the runtime API.
> As you mentioned in your mail, I think the GetAllModules will need to
> move to JVM TI so that it's consistent with GetAllClasses,
> GetAllThreads, ...
> I also went through the JDI changes. The type hierarchy in JDI is
> quite deep and is has a convention for naming that we need to follow
> too. Looking at it now then it looks like it should be ModuleReference
> extends ObjectReference. That would give us consistency with other
> mirrors that are object references (ThreadReference and
> ClassLoaderReference for example). We don't rev JDI often enough to
> keep the hierarchy and naming convention in our heads so good to have
> more eyes on this.
Ok, will try it.
> ReferenceType::module looks right, we'll just need to iterate on the
> VirtualMachine::allModules looks right. Note that the TRACE_* fields
> are public and part of the JDI API so we'll need to decide whether
> TRACE_MODULES is important or not.
> In ReferenceTypeImpl then I assume isModuleCached is not needed.
Not sure, I understand this. Why?
It seems there is some confusion here.
This flag is similar to the flag isClassLoaderCached.
> Maybe you have this to the JDWP command when connected to a target VM
> that is JDK 8 or older?
For the older JDK the UnsupportedOperationException is expected to be
> VirtualMachineImpl - this might be the time to change to 9 rather than
Fixed all places in the JDI update.
> ModuleImpl - I assume ref can be final.
> Have you thought about SA yet? I can't recall if it is compiled with
> the boot JDK or will be compiled against the newly built jdk.jdi
> module. If the later then I assume that SA will need updates. If the
> former then I assume we will have issues with boot cycle builds.
I'll ask Dmitry as he covers the SA.
He had some plans on the Jigsaw update.
> Tests. I see you have a basic test but I assume we will need to find
> time to develop a lot more tests for this update.
I'd like to add more sophisticated self-checks to the existing test.
More negative tests ...
Good JDWP and JVMTI tests are needed as well.
More information about the jigsaw-dev