Re: 8181087: Module system implementation refresh (6/2017 update)

[serguei.spit...@oracle.com]

Hi Alan,

I looked at the hotspot and top changes.
They look good.

Thanks,
Serguei


On 6/14/17 09:52, Alan Bateman wrote:
It's time to bring the changes accumulated in the jake forest into 
jdk9/dev. I'm hoping we are near the end of these updates and that we 
can close the jake forest soon.


A summary of the changes that have accumulated for this update are 
listed in this issue:

https://bugs.openjdk.java.net/browse/JDK-8181087

Aside from the important spec/javadoc updates, this update will bring 
the -illegal-access option into JDK 9 to replace the 
--permit-illegal-access option.


The webrevs with the changes are here:
  http://cr.openjdk.java.net/~alanb/8181087/1/

-Alan

Re: 8178380: Module system implementation refresh (5/2017 update)

[serguei.spit...@oracle.com]

Hi Alan,

I reviewed the Hotspot changes and the Jdk changes related to 
java.instrument and jdk.attch

(including a fragment in the LauncherHelper.java).

The fixes look good to me.

Thanks,
Serguei



On 5/1/17 13:28, Alan Bateman wrote:
As I mentioned in another thread, we need to get the changes 
accumulated in jake to jdk9/dev.


JDK-8178380 [1] has the summary of the changes that have accumulated 
since the last refresh.


One note on the hotspot repo is that the changes to dynamically 
augment the platform modules run into JDK-8178604 when testing with an 
exploded build. So there is a partial fix for this in jake. Harold has 
the more complete fix in review on hotspot-runtime-dev for JDK 10.


The webrevs with the changes is here:
http://cr.openjdk.java.net/~alanb/8178380/1/

-Alan

[1] https://bugs.openjdk.java.net/browse/JDK-8178380

Re: 8177530: Module system implementation refresh (4/2017)

[serguei.spit...@oracle.com]

Ok.

Thanks, Alan
Serguei

On 4/4/17 23:52, Alan Bateman wrote:

On 05/04/2017 07:42, serguei.spit...@oracle.com wrote:


The hotspot part looks good to me.

+ It seems, all the modules are modifiable at this point. Is it right?

That's right.

On the copyright dates then Lana will be doing a bulk update soon. I 
expect it will update hundreds of files. I don't mind manually 
updating files that we've missed here of course.


-Alan

Re: 8177530: Module system implementation refresh (4/2017)

[serguei.spit...@oracle.com]

The hotspot part looks good to me.

Some copyright comments need update:
http://cr.openjdk.java.net/~alanb/8177530/1/hotspot/src/jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.test/src/org/graalvm/compiler/test/ExportingClassLoader.java.frames.html
http://cr.openjdk.java.net/~alanb/8177530/1/hotspot/src/share/vm/oops/klass.cpp.frames.html
http://cr.openjdk.java.net/~alanb/8177530/1/hotspot/src/share/vm/prims/jvmtiEnv.cpp.frames.html


One question:
http://cr.openjdk.java.net/~alanb/8177530/1/hotspot/src/share/vm/prims/jvmtiEnv.cpp.udiff.html

+// module - pre-checked for NULL
+// is_modifiable_class_ptr - pre-checked for NULL
+jvmtiError
+JvmtiEnv::IsModifiableModule(jobject module, jboolean* 
is_modifiable_module_ptr) {

+ JavaThread* THREAD = JavaThread::current();
+
+ // check module
+ Handle h_module(THREAD, JNIHandles::resolve(module));
+ if (!java_lang_Module::is_instance(h_module())) {
+ return JVMTI_ERROR_INVALID_MODULE;
+ }
+
+ *is_modifiable_module_ptr = JNI_TRUE;
+ return JVMTI_ERROR_NONE;
+} /* end IsModifiableModule */
+ It seems, all the modules are modifiable at this point. Is it right?



Thanks,
Serguei



On 4/4/17 09:28, Alan Bateman wrote:
As I mentioned on jigsaw-dev yesterday, we have accumulated a number 
of changes in the jake forest and would like to bring the changes into 
jdk9/dev for jdk-9+165.


Most of the changes in this update are the move of Module and friends 
from java.lang.reflect to java.lang. This is mostly a mechanical update.


We also have the update to the derivation of automatic modules to no 
longer ignore trailing digits in modules names, this is to align JDK 9 
with the updated proposal for issue #VersionsInModuleNames [1].


There are a number of smaller changes, summarized in JDK-8177530 [2].

The webrev with the changes is here:
  http://cr.openjdk.java.net/~alanb/8177530/1

The changes are currently based on jdk-9+163 and will be rebased 
before pushing to jdk9/dev.


The corresponding update to jtreg is already in the code-tools/jtreg 
repo and will be tagged (I assume as jtreg4.2-b07) before this 
integration. Once it is tagged then we'll rev'ing the requiredVersion 
in each TEST.ROOT.


-Alan

[1] 
http://openjdk.java.net/projects/jigsaw/spec/issues/#VersionsInModuleNames

[2] https://bugs.openjdk.java.net/browse/JDK-8177530

Re: RFR(S) : 8177374 : fix module dependency declaration in jdk_svc tests

[serguei.spit...@oracle.com]

Hi Igor,

It looks good.

Thanks,
Serguei


On 3/22/17 13:09, Igor Ignatyev wrote:

http://cr.openjdk.java.net/~iignatyev/8177374/webrev.00/index.html

40 lines changed: 26 ins; 13 del; 1 mod;

Hi all,

could you please review this changeset which fixes in a few jdk_svc tests which 
were missed by JDK-8176176[1]?

testing: :jdk_svc tests
webrev: http://cr.openjdk.java.net/~iignatyev/8177374/webrev.00/index.html
JBS: https://bugs.openjdk.java.net/browse/JDK-8177374

[1] https://bugs.openjdk.java.net/browse/JDK-8176176

Thanks,
-- Igor

Re: 8174823: Module system implementation refresh (3/2017)

[serguei.spit...@oracle.com]

Alan,

The hotspot part looks good.

Thanks,
Serguei


On 3/21/17 14:42, Alan Bateman wrote:
We have been accumulating changes in the jake forest for the last few 
weeks to align with the proposals in JSR 376 and also to pick up API 
changes and bug fixes. It's time to bring these changes into jdk9/dev. 
The issue to bring these changes into jdk9/dev in bulk has been 
approved via the FC extension process (still in use during JDK 9 RDP2).


A summary of the main changes is listed in JDK-8174823 [1], and the 
webrevs with the changes to bring to jdk9/dev are here:

   http://cr.openjdk.java.net/~alanb/8174823/1/

The changes are currently based on jdk-9+161 and will be rebased 
before pushing.


-Alan

[1] https://bugs.openjdk.java.net/browse/JDK-8174823

Re: RFR(L) : 8176176 : fix @modules in jdk_svc tests

[serguei.spit...@oracle.com]

Hi Igor,

This looks good to me.
A couple of questions below.


http://cr.openjdk.java.net/~iignatyev//8176176/webrev.01/test/com/sun/jdi/InvokeHangTest.java.udiff.html

- * @modules jdk.jdi
  *  @library /test/lib
+ * @modules java.management
+ * jdk.jdiShould the jdk.jdi be removed from this com/sun/jdi test?


http://cr.openjdk.java.net/~iignatyev//8176176/webrev.01/test/com/sun/jdi/RedefineCrossEvent.java.udiff.html

  *  @modules java.corba
  *   jdk.jdi

  Should the jdk.jdi be removed from this com/sun/jdi test?


Thanks,
Serguei


On 3/15/17 11:16, Igor Ignatyev wrote:

Shura,

Thank you for your review. I have update 
test/java/lang/management/PlatformLoggingMXBean/TEST.properties[1] and checked 
that there are no similar things in other TEST.properties files.

Still looking for a review from a Reviewer.

[1]

--- a/test/java/lang/management/PlatformLoggingMXBean/TEST.properties Mon 
Mar 13 18:54:58 2017 -0700
+++ b/test/java/lang/management/PlatformLoggingMXBean/TEST.properties Wed 
Mar 15 11:09:06 2017 -0700
@@ -1,3 +1,2 @@
-modules = java.management \
-  java.logging
+modules = java.logging


Thanks,
— Igor


On Mar 15, 2017, at 9:56 AM, Alexandre (Shura) Iline 
 wrote:

Igor,

I have looked through a bunch of tests where @modules is changed - that looks 
good.

One minor thing I noticed is that, in 
test/java/lang/management/PlatformLoggingMXBean/TEST.properties you are 
explicitly calling out java.management. You do not have to do that because 
“modules” property is concatenated through the hierarchy.  java.management is 
already listed in test/java/lang/management/TEST.properties.

Shura


On Mar 13, 2017, at 9:03 PM, Igor Ignatyev  wrote:

Shura,

Thank you for review, I agree that having separate bugs is more convenient. [1] 
is new webrev w/ changes only in the files w/ incorrect module dependency 
declarations.

[1] http://cr.openjdk.java.net/~iignatyev//8176176/webrev.01/index.html

Thanks,
— Igor


On Mar 9, 2017, at 5:02 PM, Alexandre (Shura) Iline 
 wrote:

Igor,

I have reviewed some number tests which change the @modules - they are fine.

You, however, fix more things with this than missing module dependency 
declaration. There is a redesign of line-number-sensitive tests [1] and other 
multiple improvements such as in [2]. Would it be more convenient to have that 
as separate bugs?

Shura

[1] 
http://cr.openjdk.java.net/~iignatyev/8176176/webrev.00/test/com/sun/jdi/ArgumentValuesTest.java.sdiff.html
[2] 
http://cr.openjdk.java.net/~iignatyev/8176176/webrev.00/test/com/sun/jdi/ArrayLengthDumpTest.sh.sdiff.html


On Mar 7, 2017, at 1:07 PM, Igor Ignatyev  wrote:

http://cr.openjdk.java.net/~iignatyev/8176176/webrev.00/index.html

2586 lines changed: 669 ins; 484 del; 1433 mod;

Hi all,

Could you please review this changeset which fix @modules dependency 
declaration in jdk_svc tests?
there are a couple issues w/ modules in jdk_svc tests:
- some tests do not specify modules which they depend on
- modules in TEST.properties is not used in cases there all tests (should) have 
the same @modules directive
- @modules directive isn't placed according to current convention (before the 
1st run directive)

Since this fix has already touched lots of tests, I have decided to use this 
opportunity and reordered some of jtreg tags as well, so there won’t be two 
massive updates in the tests.

Some of our tests are line number sensitive, and then I fixed jtreg 
declaration, they started to fail. It was really hard to find our all line 
number sensitive tests, so I have unified the way we declare that as a part of 
this fix. Please let me know if you prefer to have it done separately.

There are two one-liners which, I hope, can simplify review:
[1] shows only the changes which are not in comments. Besides obvious new added 
TEST.properties, there are changes in the following line number sensitive tests 
(which I mentioned before):
test/com/sun/jdi/ArgumentValuesTest.java
test/com/sun/jdi/BreakpointTest.java
test/com/sun/jdi/FetchLocals.java
test/com/sun/jdi/GetLocalVariables.java
test/com/sun/jdi/GetSetLocalTest.java
test/com/sun/jdi/LambdaBreakpointTest.java
test/com/sun/jdi/LineNumberOnBraceTest.java
test/com/sun/jdi/PopAndStepTest.java

[2] shows changes in jtreg tags, it can help to see that almost all changes in 
jtreg tags are either moving of tags which does not affect execution order or 
@modules changes.

webrev: http://cr.openjdk.java.net/~iignatyev/8176176/webrev.00/index.html
bug: https://bugs.openjdk.java.net/browse/JDK-8176176
Testing:
- jdk_svc on linux, windows, mac
- checked that all tests which could be executed with full jdk before still can 
be executed with full jdk

Thanks,
— Igor

[1] $ hg diff -w | grep "^[+-]" | grep -v "^[+-]\s*[*#]"
[2] $ hg diff -w | grep -e "^\(+++ b\)\|\(--- a\)" -e "^[+-]\s*[*#]\s*@"

Re: RFR: 8171855: Move package name transformations during module bootstrap into native code

[serguei.spit...@oracle.com]

On 1/9/17 04:16, Claes Redestad wrote:

Hi Serguei,

On 2017-01-09 09:11, serguei.spit...@oracle.com wrote:

Hi Claes,

It looks pretty good.


thanks,



One question on the following fragment:

http://cr.openjdk.java.net/~redestad/8171855/jdk.04/src/java.base/share/native/libjava/Module.c.udiff.html 




+ int valid = 1;
+ for (idx = 0; idx < num_packages; idx++) {
+ jstring pkg = (*env)->GetObjectArrayElement(env, packages, idx);
+ pkgs[idx] = GetInternalPackageName(env, pkg, NULL, 0);
+ if (pkgs[idx] == NULL) {
+ valid = 0;
+ break;
+ }
+ }
+
+ if (valid != 0) {
+ JVM_DefineModule(env, module, is_open, version, location,
+ (const char* const*)pkgs, num_packages);
+ }
+ }


  It looks like the module won't be defined if there was an OOM in
processing one of the package names.
  The malloc is used only if the supplied buffer size is not enough.
  Would it make sense to increase the buffer size from 128 to 256 (or
even 512) at the lines:

119 char buf[128];  140 char buf[128];
161 char buf[128];  181 char buf[128];


  Nit: it is also better to use a named constant instead.


The 128 value was taken from similar code in Class.c, and since package
names are always shorter I assumed there was no real need to evaluate
other heuristics.

How about re-visiting this for an enhancement in 10 and evaluate if
a different value and a freshly introduced constant makes sense?


Sure.

Thanks,
Serguei



Thanks!

/Claes



Thanks, Serguei On 1/6/17 06:23, Claes Redestad wrote:

On 2017-01-06 15:07, Alan Bateman wrote:

On 06/01/2017 13:18, Claes Redestad wrote:

: Updated jdk webrev in-place:
http://cr.openjdk.java.net/~redestad/8171855/jdk.04/

I looked through the latest webrev and it looks quite good. Part of
me regrets introducing JNI code of course but this is startup
motivated. In Module.c then I assume GetInternalPackageName should be
static getInternalPackageName as the scope is just this file (no need
for JNIEXPORT).

Done, will update in place.

Do we have tests that use package names > 128 to test cases where the
buffer on the stack not large enough?

I'm not sure; the malloc'ing path is exercised by DefineModule, but it
would make sense to add a sanity test to each method I guess. /Claes

Re: RFR: 8171855: Move package name transformations during module bootstrap into native code

[serguei.spit...@oracle.com]

Hi Claes,

It looks pretty good.

One question on the following fragment:

http://cr.openjdk.java.net/~redestad/8171855/jdk.04/src/java.base/share/native/libjava/Module.c.udiff.html

+ int valid = 1;
+ for (idx = 0; idx < num_packages; idx++) {
+ jstring pkg = (*env)->GetObjectArrayElement(env, packages, idx);
+ pkgs[idx] = GetInternalPackageName(env, pkg, NULL, 0);
+ if (pkgs[idx] == NULL) {
+ valid = 0;
+ break;
+ }
+ }
+
+ if (valid != 0) {
+ JVM_DefineModule(env, module, is_open, version, location,
+ (const char* const*)pkgs, num_packages);
+ }
+ }


  It looks like the module won't be defined if there was an OOM in 
processing one of the package names.

  The malloc is used only if the supplied buffer size is not enough.
  Would it make sense to increase the buffer size from 128 to 256 (or 
even 512) at the lines:


119 char buf[128];  140 char buf[128];
161 char buf[128];  181 char buf[128];


  Nit: it is also better to use a named constant instead.

Thanks, Serguei On 1/6/17 06:23, Claes Redestad wrote:

On 2017-01-06 15:07, Alan Bateman wrote:

On 06/01/2017 13:18, Claes Redestad wrote:
: Updated jdk webrev in-place: 
http://cr.openjdk.java.net/~redestad/8171855/jdk.04/ 
I looked through the latest webrev and it looks quite good. Part of 
me regrets introducing JNI code of course but this is startup 
motivated. In Module.c then I assume GetInternalPackageName should be 
static getInternalPackageName as the scope is just this file (no need 
for JNIEXPORT). 

Done, will update in place.
Do we have tests that use package names > 128 to test cases where the 
buffer on the stack not large enough? 
I'm not sure; the malloc'ing path is exercised by DefineModule, but it 
would make sense to add a sanity test to each method I guess. /Claes 

Re: Request Review: JDK-6479237 (cl) Add support for classloader names

[serguei.spit...@oracle.com]

Thank you for clarifications, Mandy!
Serguei


On 10/28/16 14:36, Mandy Chung wrote:

On Oct 28, 2016, at 12:06 AM, serguei.spit...@oracle.com wrote:

   Can we give the bootstrap classloader the name "boot" or "bootstrap”?

BootClassLoader is not the boostrap class loader but instead it's 
implementation details.  The bootstrap ClassLoader instance is null and so you 
can’t invoke ClassLoader::getName.

   
Also, the lines 415 and 422 can be simplified: 415 s += m.getName(); 422 s += version;

OK.  At one point, that was how it was coded.


Also, if the loader has a name but (m == null || !m.isNamed())  then it looks like the sign 
"/" will be added twice (see L410 and L428). It can be fixed and simplified with: Add 
line before 425: s += "/"; 428 return s + cls.getName();

“//” is correct.


  Also, it is not clear why the loader name is not included for an instance of 
theBuiltinClassLoader?

Make the output compact when it can, for example, the class loader name “app” 
and “platform” from classes from the JDK can be implied.


  Would it make sense to add a comment explaining it?

Maybe not much to add that.

Mandy

Re: Request Review: JDK-6479237 (cl) Add support for classloader names

[serguei.spit...@oracle.com]

Hi Mandy,


I have a few comments.

http://cr.openjdk.java.net/~mchung/jdk9/webrevs/6479237/webrev.00/jdk/src/java.base/share/classes/jdk/internal/loader/ClassLoaders.java.udiff.html

 private static class BootClassLoader extends BuiltinClassLoader {
 BootClassLoader(URLClassPath bcp) {
- super(null, bcp);
+ super(null, null, bcp);
 }
. . .

 PlatformClassLoader(BootClassLoader parent) {
- super(parent, null);
+ super("platform", parent, null);
 }

. . .

 AppClassLoader(PlatformClassLoader parent, URLClassPath ucp) {
- super(parent, ucp);
+ super("app", parent, ucp);
 this.ucp = ucp;
 }


   Can we give the bootstrap classloader the name "boot" or "bootstrap"?
   Or this will impact too many places, and so, very risky to do?



http://cr.openjdk.java.net/~mchung/jdk9/webrevs/6479237/webrev.00/jdk/src/java.base/share/classes/java/lang/StackTraceElement.java.frames.html

379 ClassLoader loader = cls.getClassLoader0(); The loader is unused.
402 private static String toLoaderModuleClassName(Class cls) {
403 ClassLoader loader = cls.getClassLoader0();
404 Module m = cls.getModule();
405
406 // First element - class loader name
407 String s = "";
408 if (loader != null && !(loader instanceof BuiltinClassLoader) &&
409 loader.getName() != null) {
410 s = loader.getName() + "/";
411 }
412
413 // Second element - module name and version
414 if (m != null && m.isNamed()) {
415 s = s.isEmpty() ? m.getName() : s + m.getName();
416 // drop version if it's JDK module tied with java.base,
417 // i.e. non-upgradeable
418 if (!HashedModules.contains(m)) {
419 Optional ov = m.getDescriptor().version();
420 if (ov.isPresent()) {
421 String version = "@" + ov.get().toString();
422 s = s.isEmpty() ? version : s + version;
423 }
424 }
425 }
426
427 // fully-qualified class name
428 return s.isEmpty() ? cls.getName() : s + "/" + cls.getName();
429 }
Also, the lines 415 and 422 can be simplified: 415 s += m.getName(); 422 
s += version; Also, if the loader has a name but (m == null || 
!m.isNamed())  then it looks like the sign "/" will be added twice (see 
L410 and L428). It can be fixed and simplified with: Add line before 
425: s += "/"; 428 return s + cls.getName();


  Also, it is not clear why the loader name is not included for an instance of 
theBuiltinClassLoader?
  Would it make sense to add a comment explaining it?

Thanks, Serguei

On 10/25/16 16:10, Mandy Chung wrote:

Webrev at:
http://cr.openjdk.java.net/~mchung/jdk9/webrevs/6479237/webrev.00/

Specdiff:

http://cr.openjdk.java.net/~mchung/jdk9/webrevs/6479237/specdiff/overview-summary.html

This is a long-standing RFE for adding support for class
loader names.  It's #ClassLoaderNames on JSR 376 issue
list where the proposal [1] has been implemented in jake
for some time.  This patch brings this change to jdk9.

A short summary:
- New constructors are added in ClassLoader, SecureClassLoader
   and URLClassLoader to specify the class loader name.

- New ClassLoader::getName and StackTraceElement::getClassLoaderName
   method

- StackTraceElement::toString is updated to include the name
   of the class loader and module of that frame in this format:
  //(:)

The detail is in StackTraceElement::buildLoaderModuleClassName
that compress the output string for cases when the loader
has no name or the module is unnamed module.  Another thing
to mention is that VM sets the Class object when filling in
a stack trace of a Throwable object.  Then the library will
build a String from the Class object for serialization purpose.

Mandy
[1] 
http://mail.openjdk.java.net/pipermail/jpms-spec-observers/2016-September/000550.html

Re: Round #3: RFR: 8147467 - Add ClassFileTransformer transform method that provide the Module to the agent

[serguei.spit...@oracle.com]

On 2/29/16 04:29, Alan Bateman wrote:

On 29/02/2016 08:32, serguei.spit...@oracle.com wrote:

:


Please, review the fix for:
https://bugs.openjdk.java.net/browse/JDK-8147467


Jdk webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2016/jdk/8147467-Jigsaw-agents.jdk3/


This looks good and I can get this into the jake/jdk repo for you.


Thanks, Alan!



I think the javadoc will need a few additional edits but I can look 
after these. In particular, the links to the transform method need to 
be updated as it's the new transform method that is invoked. In 
addition I think we can make the class description of 
ClassFileTransformer a bit clearer.


Sure, some correction of the javadoc is needed to make it clearer.


Thanks,
Serguei




-Alan.

Re: Jake build failed

[serguei.spit...@oracle.com]

On 2/24/16 07:43, mark.reinh...@oracle.com wrote:

2016/2/24 7:33 -0800, serguei.spit...@oracle.com:

I have the same issue.
The network is very unstable:

...

It's not the network.  There was an intermittent problem
with hg.openjdk.java.net, which has now been fixed.


It works now.

Thanks!
Serguei



- Mark

Re: Jake build failed

[serguei.spit...@oracle.com]

Hi Deven,

I have the same issue.
The network is very unstable:

  hotspot:   searching for changes
corba:   pulling from 
http://hg.openjdk.java.net/jigsaw/jake/corba

corba:   abort: HTTP Error 500: Internal Server Error
 jaxp:   pulling from 
http://hg.openjdk.java.net/jigsaw/jake/jaxp

 jaxp:   abort: HTTP Error 500: Internal Server Error
  . . .
  nashorn:   pulling from 
http://hg.openjdk.java.net/jigsaw/jake/nashorn
jaxws:   pulling from 
http://hg.openjdk.java.net/jigsaw/jake/jaxws

  nashorn:   abort: HTTP Error 500: Internal Server Error
jaxws:   abort: HTTP Error 500: Internal Server Error
  . . .
WARNING: corba exited abnormally (255)
WARNING: jaxp exited abnormally (255)
WARNING: jaxws exited abnormally (255)
WARNING: jdk exited abnormally (255)
WARNING: nashorn exited abnormally (255)

Thanks,
Serguei


On 2/24/16 07:20, deven you wrote:

Hi Alan,

I now can build jdk9/jdk9 and jigsaw/jake. I find the root cause from the
logs on our jenkins machine. Somehow, the network connecting to
hg.openjdk.java.net is not stable:

abort: HTTP Error 500: Internal Server Error

This error is very frequent so that our forest can not pull a consistent
forest.

I just wonder if any third party meets the same problem or it is just the
problem within my own network.

Thanks a lot!

2016-02-24 21:45 GMT+08:00 Alan Bateman :


On 24/02/2016 13:27, deven you wrote:


Yes, it's a fresh clone on Linux amd64.

  Alan, what information you need? I can collect all information necessary
for this issue.

Are you able to build jdk9/jdk9 or jdk9/dev?

-Alan

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

[serguei.spit...@oracle.com]

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:


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.

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

[serguei.spit...@oracle.com]

Thanks, Alan!

I'm Ok with the tweaks you suggest before the push.

Thanks,
Serguei


On 1/26/16 04:45, Alan Bateman wrote:


On 26/01/2016 11:48, serguei.spit...@oracle.com wrote:

Please, review this initial fix for the M4 Jigsaw task:
https://bugs.openjdk.java.net/browse/JDK-8049365


Jdk webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2016/jdk/8049365-Jigsaw-jdk.4/

Hotspot webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2016/hotspot/8049365-Jigsaw-hs.4/

Thanks Serguei, I think this is looking good and I think you've caught 
all the places that were sensitive to the major/minor version.


I will get this push to jigsaw/jake today. If you don't mind, I will 
revert the header in the checked-in jvmti.h as otherwise the run-time 
images will have a jvmti.h with the GPL header. The issue that the 
checked-in version was slightly out of sync with the generated is of 
course technical debt so hopefully we will get to those issues in JDK 9.


One minor point is that the the canGetModuleInfo has a link to 
ReferenceType#Module() then it should be ReferenceType#module(). I can 
fix this before push your patches. At some point I think we should add 
@implSpec to the new methods so that the default implementation is 
specified.


-Alan

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

[serguei.spit...@oracle.com]

Hi Lois,

Some comments below.


On 1/25/16 11:42, Lois Foltan wrote:


On 1/22/2016 5:16 PM, serguei.spit...@oracle.com wrote:

Please, review this initial fix for the M4 Jigsaw task:
https://bugs.openjdk.java.net/browse/JDK-8049365


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/ 



Hi Serguei,

I have reviewed the hotspot changes and it looks good.


Thank you for the review!



Some minor comments:

src/share/vm/prims/jvmtiEnvBase.hpp
- line #698: I think the do_module() method should have a 
"assert_locked_or_safepoint(Module_lock);" statement.  The method 
shouldn't be invoked unless under the Module_lock.
- line #705: Is there a need for JvmtiModuleClosure to have an empty 
constructor definition?  Could it be removed?


src/share/vm/prims/jvmtiEnvBase.cpp
- line #1486: With a basic "java -version" command there are 77 
modules defined to the VM, so I think the GrowableArray should be 
initially allocated to at least 77 elements.


Good suggestions, all fixed.




In addition, JvmtiModuleClosure::get_all_modules() is using the 
ClassLoaderDataGraph's modules_do() method that Markus Gronlund had 
added for JFR support.  Good to see you removed the ModulesTable data 
structure that you had added to modules.[c/h]pp in your preliminary 
implementation.  So at this point, I actually don't think you need 
anything further from me, JvmtiModuleClosure implementation looks good 
and my initial concern was around the ModulesTable data structure 
which is now gone.


Great.
I'm resolving the review comments from Alan now and hope to send new 
webrevs today after the testing.


Thanks!
Serguei




Thanks,
Lois




Summary:
  This round resolves most of the Alan's comments from previous 
review rounds.

  Two follow-up tasks were filed:
https://bugs.openjdk.java.net/browse/JDK-8148106
https://bugs.openjdk.java.net/browse/JDK-8148103


Also, please, refer to a related M4 Jigsaw task:
https://bugs.openjdk.java.net/browse/JDK-8049364
Update JVM TI for modules



Thanks,
Serguei

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

[serguei.spit...@oracle.com]

On 1/22/16 00:35, Alan Bateman wrote:

On 22/01/2016 03:34, serguei.spit...@oracle.com wrote:


I saw the discussion but have no strong opinion yet.
Changing to JVMTI_VERSION_9 will make it more consistent though.
Thanks. We can always revert it if it turns out that there is a good 
reason not to move it to 9 and keep it in sync with the Java SE 
version. Capturing this in JEP 223 would be good but may be a bit 
beyond the original scope.


Made this change. It is trivial.






I could easily fix it now but it is Ok to file a separate bug.
I created JDK-8147943 yesterday to track it and also linked it to 
JDK-8063154, the issue to remove jvmti.h from the jdk repo and use the 
generated header.


Sorting out these issues it not core to what we are doing here so I 
suggest just bringing over the changes from the generated jvmti.h to 
the checked in jvmti.h. That will ensure that the jvmti.h in the JDK 
image has the right copyright header.


Ok, thanks!
I do not touch this part now.






Will provide an update in the next review round.
Thanks and assuming we are down to minor issues then I think we should 
get the patches into jake and iterate on them as needed. Also I think 
of this patch as just a first step, there will be more once the IDEs 
start to add explore debugging code in modules.


Agreed.
I discovered that some tweaks in the SA-JDI are necessary to make it 
compiled.

The most of your minor comments are also resolved in new version.
I'll send new webrevs shortly.


Thanks,
Serguei



-Alan.

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

[serguei.spit...@oracle.com]

Please, review this initial fix for the M4 Jigsaw task:
https://bugs.openjdk.java.net/browse/JDK-8049365


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/


Summary:
  This round resolves most of the Alan's comments from previous review 
rounds.

  Two follow-up tasks were filed:
https://bugs.openjdk.java.net/browse/JDK-8148106
https://bugs.openjdk.java.net/browse/JDK-8148103


Also, please, refer to a related M4 Jigsaw task:
https://bugs.openjdk.java.net/browse/JDK-8049364
Update JVM TI for modules



Thanks,
Serguei

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

[serguei.spit...@oracle.com]

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.


Ok
I saw the discussion but have no strong opinion yet.
Changing to JVMTI_VERSION_9 will make it more consistent though.




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.


I hope so.




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.


I could easily fix it now but it is Ok to file a separate bug.




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  after the 
@throws.


Ok



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.


Ok



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.


Will check.



@jdk.Exported has been removed in JDK 9 and that removal has got to 
jake. I assume your forest is a bit behind.


Nice catch.
Will pull an update.




jdi.ModuleReference
- as this code is new then it would be good to use {@code  ..} instead 
of .
- 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".


Ok




tools.jdi.ModuleReferenceImpl
- if change the fields to volatile then we can avoid these methods 
needing to be synchronized.
- 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.


Ok



tools.jdi.VirtualMachineImpl
- can modulesByID be Map<Long, ModuleReference>, I can't quite tell 
why the value needs to be the impl type.
- 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.


Will check.



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.


Agreed.
Will provide an update in the next review round.


Thanks,
Serguei


-Alan

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

[serguei.spit...@oracle.com]

Please, review this initial fix for the Jigsaw Bill milestone task:
https://bugs.openjdk.java.net/browse/JDK-8049365


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/


Summary:
  This round includes the changes suggested by Alan in the round #0:
- The version 1.9 is replaced version
- The Module interface is replaced with the ModuleReference 
interface that extends the ObjectReference.
- The argument name "target" in the CanRead is replaced with 
"source" to make it consistent with

   the class java.lang.reflect.Module.
- The public field TRACE_MODULES has been removed.
- Some other small changes.

   This round also addresses the suggestions from Alan in the review 
round #1:
- new capability "can_get_modules_info" is defined on the JDI level 
only;
  changed the capability name: "canGetModulesInfo" => 
"canGetModuleInfo"
- JVMTI doc cleanup: minor corrections, removed couple of 
references to the JDK 1.1, spurious 's

- copyright fixes
- test improvements

This fix includes JVMTI support for the JDI allModules() as a JVMTI 
function GetAllModules().
  There is an agreement with Lois this will most likely need some 
future adjustment.



  Also, please, refer to a related Jigsaw Bill milestone task:
https://bugs.openjdk.java.net/browse/JDK-8049364
Update JVM TI for modules


Thanks,
Serguei

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

[serguei.spit...@oracle.com]

On 12/29/15 23:52, Alan Bateman wrote:


On 28/12/2015 20:13, serguei.spit...@oracle.com wrote:


It would be nice to get rid of this JVM TI capability.
But my question is if new JVMTI functionality is mandatory for all 
VM's out there.
I think we need to look at aligning the next version of JVM TI with 
Java SE 9 and if this makes sense then it would mean the capability 
isn't needed.


Ok, got it.
Thanks.

It also means that residual references to JDK 1.1 in the spec can be 
dropped, I think these date back to JVMDI or JVMPI.


Do you mean to make a clean up and get rid of all occurrences of 
since="1.1"?


cat -n src/share/vm/prims/jvmti.xml | g since | g '1\.1'
  1468  since="1.1">
  1928  since="1.1">

  2971  
  3021  
  3069  
  3112  
  3155  
  3196  
  3306 since="1.1">
  3330 since="1.1">
  3398 since="1.1">
  3428 since="1.1">
  3624 since="1.1">
  3639 since="1.1">
  3655 since="1.1">
  3700 since="1.1">
  3733 since="1.1">
  3789 since="1.1">
  3840 since="1.1">
  4001  
  4056  
  4160  
  4234  
  4300  
  4363  
  4373  
  4608  
  6888  num="145" since="1.1">
  6932  since="1.1">
  7071  phase="start" num="45" since="1.1">
  7196  since="1.1">
  8434  phase="any" num="73" since="1.1">
  8557  phase="any" num="74" since="1.1">

  9917
  9923id="can_get_owned_monitor_stack_depth_info" since="1.1">

  9929
  9935since="1.1">

  9942
  9959
  9966id="can_generate_resource_exhaustion_heap_events" since="1.1">
  9973id="can_generate_resource_exhaustion_threads_events" since="1.1">
 10589  phase="onload" num="151" since="1.1">

 12951   since="1.1">
 12961   since="1.1">


Just want to make sure I understand this correctly...






The JDWP agent uses JNI to upcall to Module::getClassLoader and 
Module::canRead, we might consider adding JNI or JVM TI functions in 
the future and avoid this.


I was thinking about the same.
My preference would be to add new JVM TI functions.
We can add these later, it's not critical to get these JDWP commands 
working (as you've demonstrated).


Ok.
I put it in my todo list.


Thanks,
Serguei




-Alan.

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

[serguei.spit...@oracle.com]

On 12/30/15 06:57, Alan Bateman wrote:



On 30/12/2015 11:37, serguei.spit...@oracle.com wrote:


Do you mean to make a clean up and get rid of all occurrences of 
since="1.1"?


I meant JDK 1.1 rather than JVM TI 1.1. There are a few functions like 
GetThreadInfo where it has spec like "For JDK 1.1 implementations 
...". This is nothing to do with modules of course, just something 
that could be cleaned up if we can get JVM TI aligned.


Ok, got it.
I see just two places.
Cleaned them up.

Thanks,
Serguei




-Alan

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

[serguei.spit...@oracle.com]

Alan,

Thank you for the review!

Just wanted to notify you that I'm officially on vacation this week as 
all US employees.

But I'm at home so that I can participate in the email discussions.
The next week I'm on vacation as well and will be out of town with 
limited access to the internet.


Please, see my comments below.


On 12/28/15 08:22, Alan Bateman wrote:

On 23/12/2015 06:24, serguei.spit...@oracle.com wrote:

Please, review this initial fix for the Jigsaw Bill milestone task:
https://bugs.openjdk.java.net/browse/JDK-8049365


Jdk webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2015/jdk/8049365-Jigsaw-jdk.1/


Hotspot webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2015/hotspot/8049365-Jigsaw-hs.1/


Summary:
  This round includes most of the changes suggested or discussed by 
Alan in the round #0:

- The version 1.9 is replaced version
- The Module interface is replaced with the ModuleReference 
interface that extends the ObjectReference.
- The argument name "target" in the CanRead is replaced with 
"source" to make it consistent with

   the class java.lang.reflect.Module.
- The public field TRACE_MODULES has been removed.
- Some other small changes.
   I hope, all comments are addressed. But, please, let me know 
if anything is missed.



  This round does not include the corresponding update of the SA-JDI.
  I'm suggesting to separate and postpone this part for now to speed 
up the main part.
  Another reason for it is that the SA-JDI update depends on possible 
VM changes.


  This round adds new capability "can_get_modules_info" that is 
appeared on all levels:

 JVMTI, JDWP and JDI.
No problem updating SA-JDI at a later time, we just need to make sure 
it builds without issues.


Good.
I'll open a task for that.



I looked through the updated webrevs and it looks quite good. A few 
comments:


It's not clear to me that a new JVM TI capability is required. I agree 
that JDWP and JDI need a canXXX command/method but if we rev JVM TI to 
version 9 then the VM supports modules and so a capability shouldn't 
be needed.


It would be nice to get rid of this JVM TI capability.
But my question is if new JVMTI functionality is mandatory for all VM's 
out there.






The JDWP command is currently "canGetModulesInfo", it might be better 
as "canGetModuleInfo". Ditto for the method on VirtualMachine.


Taken.
I was also thinking if this can be better..




The JDWP agent uses JNI to upcall to Module::getClassLoader and 
Module::canRead, we might consider adding JNI or JVM TI functions in 
the future and avoid this.


I was thinking about the same.
My preference would be to add new JVM TI functions.




VirtualMachine::allModules - there's a spurious  in the javadoc.


Will check and clean it up.




ModuleReference javadoc currently contains ".. has a read edge with 
the source ModuleReference". We'll need to do a bit of wordsmithing on 
the javadoc but for this one then starting out with " .. if the module 
reads the source module" would be clearer.


Taken.
Thank you for the correction.




InvalidModuleException - the copyright date says 1998 :-)


Strange.
I've already fixed that. :)



I see you have a basic test. The checkClassLoader method needs 
comments as it's not clear what it is testing. We'll need to add 
further tests to cover all the scenarios, the unnamed module case for 
example.


Yes, of course.
I'll try to enhance the test case.


Thanks!
Serguei




-Alan

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

[serguei.spit...@oracle.com]

On 12/22/15 22:24, serguei.spit...@oracle.com wrote:

Please, review this initial fix for the Jigsaw Bill milestone task:
https://bugs.openjdk.java.net/browse/JDK-8049365


Jdk webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2015/jdk/8049365-Jigsaw-jdk.1/


Hotspot webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2015/hotspot/8049365-Jigsaw-hs.1/


Summary:
  This round includes most of the changes suggested or discussed by 
Alan in the round #0:

- The version 1.9 is replaced version

Somehow a part of the sentence is lost: "is replaced with the version 9".

- The Module interface is replaced with the ModuleReference 
interface that extends the ObjectReference.
- The argument name "target" in the CanRead is replaced with 
"source" to make it consistent with

   the class java.lang.reflect.Module.
- The public field TRACE_MODULES has been removed.
- Some other small changes.
   I hope, all comments are addressed. But, please, let me know if 
anything is missed.



  This round does not include the corresponding update of the SA-JDI.
  I'm suggesting to separate and postpone this part for now to speed 
up the main part.
  Another reason for it is that the SA-JDI update depends on possible 
VM changes.


  This round adds new capability "can_get_modules_info" that is 
appeared on all levels:

 JVMTI, JDWP and JDI.

  It is expected that the JDI and JDWP update for modules will have 
more iterations.

  This is the initial one, and it introduces minimal functionality.

  It does not include yet the API's for introspection of the 
ModuleDescriptor, Configuration and Layer.

  There are no convincing use cases for it yet.
  It is still TBD to contact and get more feedback from the NetBeans 
and Eclipse debuggers teams.

  We also could give them our custom build to try.

This fix includes JVMTI support for the JDI allModules() as a JVMTI 
function GetAllModules().
  Any feedback on the Hotspot webrev as to how to implement it better 
is welcome.
  We agreed with Lois that some adjustments or tweaks from her will be 
necessary.



  Also, please, refer to a related Jigsaw Bill milestone task:
https://bugs.openjdk.java.net/browse/JDK-8049364
Update JVM TI for modules


Thanks,
Serguei

Re: preliminary RFR: 8049365 - Update JDI and JDWP for modules

[serguei.spit...@oracle.com]

Thanks, Mandy!

Yes, the SA JDI connectors need an update for modules too.

Thanks,
Serguei


On 12/11/15 18:07, Mandy Chung wrote:

On Dec 11, 2015, at 12:40 AM, Alan Bateman  wrote:

We just need to make sure that they build and I can't recall if they are 
compiled against the JDI in the boot JDK or the jdk.jdi module. If the former 
then we would only see issues with boot cycle builds.


jdk.hotspot.agent is built in the same way as other JDK modules.  Thanks to 
Erik’s recent change converting converting the SA agent build to modular build.

Mandy
[1] https://bugs.openjdk.java.net/browse/JDK-8142336

Re: preliminary RFR: 8049365 - Update JDI and JDWP for modules

[serguei.spit...@oracle.com]

On 12/11/15 00:40, Alan Bateman wrote:



On 10/12/2015 20:44, serguei.spit...@oracle.com wrote:

:



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?
We haven't decided what Module::getName should return for unnamed 
modules yet, it currently returns null. So we'll have to come back to 
it. For now then maybe JDWP can just return the stringID as 0 and 
we'll come back to it.


In the jdwp.spec the 'stringObject' type is used instead of the 'stringID'.
For the name command I used the 'string' type for consistency as the the 
only
command where the 'stringObject' is returned is the 
VirtualMachine.CreateString.
I'm Ok to convert to using the 'stringObject' for Module.Name if you 
still prefer it.








:



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.


I should have been clearer, I was just making the point that the flag 
isn't really needed as module == null will tell you that it hasn't 
been retreived from the target VM.


I see now, thanks!
The isClassLoaderCached was needed because a classLoader can be null.
Fixed.






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.

Okay, although I'm just asking about the SA JDI connectors here. We 
just need to make sure that they build and I can't recall if they are 
compiled against the JDI in the boot JDK or the jdk.jdi module. If the 
former then we would only see issues with boot cycle builds.


I see now, thanks!
Will check what is needed for the SA JDI connectors and let you know.


Thanks,
Serguei



-Alan.

Re: preliminary RFR: 8049365 - Update JDI and JDWP for modules

[serguei.spit...@oracle.com]

Alan,

Thanks for the comments and suggestions!


On 12/10/15 04:04, Alan Bateman wrote:


On 10/12/2015 08:03, serguei.spit...@oracle.com wrote:


Jdk webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2015/jdk/8049365-Jigsaw-jdk.0/

Hotspot webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2015/hotspot/8049365-Jigsaw-hs.0/


Summary:

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


Fixed.



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.


Fixed.





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


Sure.





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


Ok.



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.


Ok.




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?


No.
For the older JDK the UnsupportedOperationException is expected to be 
thrown.





VirtualMachineImpl - this might be the time to change to 9 rather than 
1.9.


Fixed all places in the JDI update.




ModuleImpl - I assume ref can be final.


Fixed.


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.


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


Thanks,
Serguei





-Alan