Re: RFR(XXS): 8160102: Typo in message for NULL memory size arguments in diagnosticArgument.cpp

[David Holmes]

Looks good.

Thanks,
David

On 23/06/2016 6:57 AM, Dmitry Dmitriev wrote:

Hello,

Please review small patch which fixes error message for NULL memory size
arguments. Currently message for nanotime arguments is printed. I change
it to make it consistent with other error messages for memory size
arguments.

JBS: https://bugs.openjdk.java.net/browse/JDK-8160102
webrev.00: http://cr.openjdk.java.net/~ddmitriev/8160102/webrev.00/

Testing: jprt

Thanks,
Dmitry

Re: Jigsaw Enhancement RFR round #2: 8159145 Add JVMTI function GetModuleByPackageName

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

On 6/22/16 16:40, David Holmes wrote:

On 23/06/2016 9:20 AM, serguei.spit...@oracle.com wrote:

On 6/22/16 15:31, David Holmes wrote:

On 23/06/2016 5:01 AM, Alan Bateman wrote:

On 22/06/2016 19:36, serguei.spit...@oracle.com wrote:



Alan, please, let me know if you support the Stanislav's suggestion.
I can go ahead and implement returning the
JVMTI_ERROR_ILLEGAL_ARGUMENT for invalid package names.
No objection from me. It would be good to first check the existing 
spec

and also the JNI spec to see if there any similar cases (so that the
update and new function is consistent). Off-hand, I can't think of any
functions that require the same check.


Taking GetLocalVariableTable as an example there is no validation that
names are legal, so there should not be a check in this case either.


David,

I agree with it.
Thank you for pointing to this JVMTI example.
I did not find in the JNI where the names are checked to be legal.
We are going to open a can of worms with this kind of check as there can
be many corner cases to cover.


Right - JVM TI and JNI both do minimal argument checking/validation.

You may want to specify an error for the NULL case - or else treat it 
as the empty string (which would need documenting).


It is already there.
It is a part of the universal-error :
http://docs.oracle.com/javase/8/docs/platform/jvmti/jvmti.html#universal-error

Thanks,
Serguei



Cheers,
David


Thanks,
Serguei




Thanks,
David
-


-Alan

Re: Jigsaw Enhancement RFR round #2: 8159145 Add JVMTI function GetModuleByPackageName

[stanislav lukyanov]

Hi Serguei,

What is the expected behavior when passed string is not a valid package 
name (e.g. contains illegal characters)?

I think I'd expect JVMTI_ERROR_ILLEGAL_ARGUMENT to be returned in that case.

Now it looks like the method will return unnamed module not only for an 
empty string

but for any string that is not a known package name.
If it's correct, I think the empty string case shouldn't be described 
explicitly.


Thanks,
Stas

On 22.06.2016 16:05, serguei.spit...@oracle.com wrote:
I've updated the libGetModuleByPkgTest.c in place in the webrev #2 
with minor improvements.

Sorry, if it spoiled your work.

Thanks,
Serguei


On 6/22/16 04:07, serguei.spit...@oracle.com wrote:
Here are new hotspot webrev where I've fixed the comments from Alan 
and Christian.


Hotspot:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2016/hotspot/8159145-jigsaw-jvmti-pkg.2 




The Jdk webrev is the same:
http://javaweb.sfbay.sun.com/java/svc/ss45998/webrevs/2016/hotspot/8159145-jigsaw-jvmti-pkg.jdk1/ 




Thanks,
Serguei


On 6/21/16 02:54, serguei.spit...@oracle.com wrote:


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


The Hotspot webrev:
http://javaweb.sfbay.sun.com/java/svc/ss45998/webrevs/2016/hotspot/8159145-jigsaw-jvmti-pkg.1/ 



The Jdk webrev:
http://javaweb.sfbay.sun.com/java/svc/ss45998/webrevs/2016/hotspot/8159145-jigsaw-jvmti-pkg.jdk1/ 




Summary:

JVM TI agents that instrument code in named modules need the Module 
at class load time.


  One way to do this is by introducing a new ClassFileLoadHook that 
takes an additional parameter but this approach is disruptive.
  The alternative option is a JVM TI function that maps a 
classloader + package name to a module.
  We were initially not confident with this approach so we 
introduced it as JVM function JVM_GetModuleByPackageName.
  Based on experience to date then this approach seems okay and so 
this function needs to be promoted to a JVMTI function.


  This fix is to introduce new JVMTI function GetModuleByPackageName.
  It includes new jtreg test with native JVMTI agent.

  A CCC is fast-tracked and getting an approval is in progress.

Testing:
   Run newly developed jtreg test.

Thanks,
Serguei

Re: Jigsaw Enhancement RFR round #2: 8159145 Add JVMTI function GetModuleByPackageName

[David Holmes]

On 23/06/2016 9:20 AM, serguei.spit...@oracle.com wrote:

On 6/22/16 15:31, David Holmes wrote:

On 23/06/2016 5:01 AM, Alan Bateman wrote:

On 22/06/2016 19:36, serguei.spit...@oracle.com wrote:



Alan, please, let me know if you support the Stanislav's suggestion.
I can go ahead and implement returning the
JVMTI_ERROR_ILLEGAL_ARGUMENT for invalid package names.

No objection from me. It would be good to first check the existing spec
and also the JNI spec to see if there any similar cases (so that the
update and new function is consistent). Off-hand, I can't think of any
functions that require the same check.


Taking GetLocalVariableTable as an example there is no validation that
names are legal, so there should not be a check in this case either.


David,

I agree with it.
Thank you for pointing to this JVMTI example.
I did not find in the JNI where the names are checked to be legal.
We are going to open a can of worms with this kind of check as there can
be many corner cases to cover.


Right - JVM TI and JNI both do minimal argument checking/validation.

You may want to specify an error for the NULL case - or else treat it as 
the empty string (which would need documenting).


Cheers,
David


Thanks,
Serguei




Thanks,
David
-


-Alan

Re: Jigsaw Enhancement RFR round #2: 8159145 Add JVMTI function GetModuleByPackageName

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

On 6/22/16 15:31, David Holmes wrote:

On 23/06/2016 5:01 AM, Alan Bateman wrote:

On 22/06/2016 19:36, serguei.spit...@oracle.com wrote:



Alan, please, let me know if you support the Stanislav's suggestion.
I can go ahead and implement returning the
JVMTI_ERROR_ILLEGAL_ARGUMENT for invalid package names.

No objection from me. It would be good to first check the existing spec
and also the JNI spec to see if there any similar cases (so that the
update and new function is consistent). Off-hand, I can't think of any
functions that require the same check.


Taking GetLocalVariableTable as an example there is no validation that 
names are legal, so there should not be a check in this case either.


David,

I agree with it.
Thank you for pointing to this JVMTI example.
I did not find in the JNI where the names are checked to be legal.
We are going to open a can of worms with this kind of check as there can 
be many corner cases to cover.


Thanks,
Serguei




Thanks,
David
-


-Alan

Re: Jigsaw Enhancement RFR round #2: 8159145 Add JVMTI function GetModuleByPackageName

[David Holmes]

On 23/06/2016 5:01 AM, Alan Bateman wrote:

On 22/06/2016 19:36, serguei.spit...@oracle.com wrote:



Alan, please, let me know if you support the Stanislav's suggestion.
I can go ahead and implement returning the
JVMTI_ERROR_ILLEGAL_ARGUMENT for invalid package names.

No objection from me. It would be good to first check the existing spec
and also the JNI spec to see if there any similar cases (so that the
update and new function is consistent). Off-hand, I can't think of any
functions that require the same check.


Taking GetLocalVariableTable as an example there is no validation that 
names are legal, so there should not be a check in this case either.


Thanks,
David
-


-Alan

Re: Jigsaw Enhancement RFR round #2: 8159145 Add JVMTI function GetModuleByPackageName

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

On 6/22/16 13:54, Jiangli Zhou wrote:

Hi Serguei,

On Jun 22, 2016, at 1:17 PM, serguei.spit...@oracle.com 
 wrote:


Hi Jiangli,

Fixed both places - nice catch!
Would you like I list you as a reviewer?


No worry, if you already committed/pushed your changes.


I've not committed/pushed yet.

Thanks,
Serguei



Thanks,
Jiangli



Thanks,
Serguei


On 6/22/16 10:29, Jiangli Zhou wrote:

Hi Serguei,

The comment in the following assert is outdated. There is no 
get_module_from_pkg. Should it be changed to 
get_module_by_package_name()?


804 assert(ModuleEntryTable::javabase_defined(),
805 "Attempt to call get_module_from_pkg before java.base is defined");

It’s not part of your change, I just happen to notice it. In the 
same get_module_by_package_name(), both the ‘if’ and ‘else’ cases 
have return statement. The ‘return NULL’ at line 825 seems will 
never reached and not needed.


Thanks,
Jiangli


On Jun 22, 2016, at 4:07 AM, serguei.spit...@oracle.com wrote:

Here are new hotspot webrev where I've fixed the comments from Alan 
and Christian.


Hotspot:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2016/hotspot/8159145-jigsaw-jvmti-pkg.2 




The Jdk webrev is the same:
http://javaweb.sfbay.sun.com/java/svc/ss45998/webrevs/2016/hotspot/8159145-jigsaw-jvmti-pkg.jdk1/


Thanks,
Serguei


On 6/21/16 02:54, serguei.spit...@oracle.com wrote:


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


The Hotspot webrev:
http://javaweb.sfbay.sun.com/java/svc/ss45998/webrevs/2016/hotspot/8159145-jigsaw-jvmti-pkg.1/

The Jdk webrev:
http://javaweb.sfbay.sun.com/java/svc/ss45998/webrevs/2016/hotspot/8159145-jigsaw-jvmti-pkg.jdk1/


Summary:

JVM TI agents that instrument code in named modules need the 
Module at class load time.


 One way to do this is by introducing a new ClassFileLoadHook that 
takes an additional parameter but this approach is disruptive.
 The alternative option is a JVM TI function that maps a 
classloader + package name to a module.
 We were initially not confident with this approach so we 
introduced it as JVM function JVM_GetModuleByPackageName.
 Based on experience to date then this approach seems okay and so 
this function needs to be promoted to a JVMTI function.


 This fix is to introduce new JVMTI function GetModuleByPackageName.
 It includes new jtreg test with native JVMTI agent.

 A CCC is fast-tracked and getting an approval is in progress.

Testing:
  Run newly developed jtreg test.

Thanks,
Serguei

RFR(XXS): 8160102: Typo in message for NULL memory size arguments in diagnosticArgument.cpp

[Dmitry Dmitriev]

Hello,

Please review small patch which fixes error message for NULL memory size 
arguments. Currently message for nanotime arguments is printed. I change 
it to make it consistent with other error messages for memory size 
arguments.


JBS: https://bugs.openjdk.java.net/browse/JDK-8160102
webrev.00: http://cr.openjdk.java.net/~ddmitriev/8160102/webrev.00/ 


Testing: jprt

Thanks,
Dmitry

Re: Jigsaw Enhancement RFR round #2: 8159145 Add JVMTI function GetModuleByPackageName

[Jiangli Zhou]

Hi Serguei,

> On Jun 22, 2016, at 1:17 PM, serguei.spit...@oracle.com wrote:
> 
> Hi Jiangli,
> 
> Fixed both places - nice catch!
> Would you like I list you as a reviewer?

No worry, if you already committed/pushed your changes.

Thanks,
Jiangli

> 
> Thanks,
> Serguei
> 
> 
> On 6/22/16 10:29, Jiangli Zhou wrote:
>> Hi Serguei,
>> 
>> The comment in the following assert is outdated. There is no 
>> get_module_from_pkg. Should it be changed to get_module_by_package_name()?
>> 
>>  804   assert(ModuleEntryTable::javabase_defined(),
>>  805  "Attempt to call get_module_from_pkg before java.base is 
>> defined");
>> 
>> It’s not part of your change, I just happen to notice it. In the same 
>> get_module_by_package_name(), both the ‘if’ and ‘else’ cases have return 
>> statement. The ‘return NULL’ at line 825 seems will never reached and not 
>> needed. 
>> 
>> Thanks,
>> Jiangli
>> 
>>> On Jun 22, 2016, at 4:07 AM,  
>>> serguei.spit...@oracle.com 
>>>  wrote:
>>> 
>>> Here are new hotspot webrev where I've fixed the comments from Alan and 
>>> Christian.
>>> 
>>> Hotspot:
>>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2016/hotspot/8159145-jigsaw-jvmti-pkg.2
>>>  
>>> 
>>> 
>>> 
>>> The Jdk webrev is the same:
>>> http://javaweb.sfbay.sun.com/java/svc/ss45998/webrevs/2016/hotspot/8159145-jigsaw-jvmti-pkg.jdk1/
>>>  
>>> 
>>> 
>>> 
>>> Thanks,
>>> Serguei
>>> 
>>> 
>>> On 6/21/16 02:54, serguei.spit...@oracle.com 
>>>  wrote:
 
 Please, review the Jigsaw fix for the enhancement:
 https://bugs.openjdk.java.net/browse/JDK-8159145 
 
 
 
 The Hotspot webrev:
 http://javaweb.sfbay.sun.com/java/svc/ss45998/webrevs/2016/hotspot/8159145-jigsaw-jvmti-pkg.1/
  
 
 
 The Jdk webrev:
 http://javaweb.sfbay.sun.com/java/svc/ss45998/webrevs/2016/hotspot/8159145-jigsaw-jvmti-pkg.jdk1/
  
 
 
 
 Summary:
 
 JVM TI agents that instrument code in named modules need the Module at 
 class load time.
 
  One way to do this is by introducing a new ClassFileLoadHook that takes 
 an additional parameter but this approach is disruptive.
  The alternative option is a JVM TI function that maps a classloader + 
 package name to a module.
  We were initially not confident with this approach so we introduced it as 
 JVM function JVM_GetModuleByPackageName.
  Based on experience to date then this approach seems okay and so this 
 function needs to be promoted to a JVMTI function.
 
  This fix is to introduce new JVMTI function GetModuleByPackageName.
  It includes new jtreg test with native JVMTI agent.
 
  A CCC is fast-tracked and getting an approval is in progress.
 
 Testing:
   Run newly developed jtreg test.
 
 Thanks,
 Serguei
>>> 
>> 
> 

Re: Jigsaw Enhancement RFR round #2: 8159145 Add JVMTI function GetModuleByPackageName

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

Hi Jiangli,

Fixed both places - nice catch!
Would you like I list you as a reviewer?

Thanks,
Serguei


On 6/22/16 10:29, Jiangli Zhou wrote:

Hi Serguei,

The comment in the following assert is outdated. There is no 
get_module_from_pkg. Should it be changed to get_module_by_package_name()?


804 assert(ModuleEntryTable::javabase_defined(),
805 "Attempt to call get_module_from_pkg before java.base is defined");

It’s not part of your change, I just happen to notice it. In the same 
get_module_by_package_name(), both the ‘if’ and ‘else’ cases have 
return statement. The ‘return NULL’ at line 825 seems will never 
reached and not needed.


Thanks,
Jiangli

On Jun 22, 2016, at 4:07 AM, serguei.spit...@oracle.com 
 wrote:


Here are new hotspot webrev where I've fixed the comments from Alan 
and Christian.


Hotspot:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2016/hotspot/8159145-jigsaw-jvmti-pkg.2 




The Jdk webrev is the same:
http://javaweb.sfbay.sun.com/java/svc/ss45998/webrevs/2016/hotspot/8159145-jigsaw-jvmti-pkg.jdk1/


Thanks,
Serguei


On 6/21/16 02:54, serguei.spit...@oracle.com wrote:


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


The Hotspot webrev:
http://javaweb.sfbay.sun.com/java/svc/ss45998/webrevs/2016/hotspot/8159145-jigsaw-jvmti-pkg.1/

The Jdk webrev:
http://javaweb.sfbay.sun.com/java/svc/ss45998/webrevs/2016/hotspot/8159145-jigsaw-jvmti-pkg.jdk1/


Summary:

JVM TI agents that instrument code in named modules need the Module 
at class load time.


 One way to do this is by introducing a new ClassFileLoadHook that 
takes an additional parameter but this approach is disruptive.
 The alternative option is a JVM TI function that maps a classloader 
+ package name to a module.
 We were initially not confident with this approach so we introduced 
it as JVM function JVM_GetModuleByPackageName.
 Based on experience to date then this approach seems okay and so 
this function needs to be promoted to a JVMTI function.


 This fix is to introduce new JVMTI function GetModuleByPackageName.
 It includes new jtreg test with native JVMTI agent.

 A CCC is fast-tracked and getting an approval is in progress.

Testing:
  Run newly developed jtreg test.

Thanks,
Serguei

Re: Jigsaw Enhancement RFR round #2: 8159145 Add JVMTI function GetModuleByPackageName

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

On 6/22/16 12:01, Alan Bateman wrote:

On 22/06/2016 19:36, serguei.spit...@oracle.com wrote:



Alan, please, let me know if you support the Stanislav's suggestion.
I can go ahead and implement returning the 
JVMTI_ERROR_ILLEGAL_ARGUMENT for invalid package names.

No objection from me.


Ok, thanks!


It would be good to first check the existing spec and also the JNI 
spec to see if there any similar cases (so that the update and new 
function is consistent).


Will do.


Off-hand, I can't think of any functions that require the same check.


Agreed.



Thanks,
Serguei



-Alan

Re: Jigsaw Enhancement RFR round #2: 8159145 Add JVMTI function GetModuleByPackageName

[Alan Bateman]

On 22/06/2016 19:36, serguei.spit...@oracle.com wrote:



Alan, please, let me know if you support the Stanislav's suggestion.
I can go ahead and implement returning the 
JVMTI_ERROR_ILLEGAL_ARGUMENT for invalid package names.
No objection from me. It would be good to first check the existing spec 
and also the JNI spec to see if there any similar cases (so that the 
update and new function is consistent). Off-hand, I can't think of any 
functions that require the same check.


-Alan

Re: Jigsaw Enhancement RFR round #2: 8159145 Add JVMTI function GetModuleByPackageName

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

On 6/22/16 11:11, serguei.spit...@oracle.com wrote:

On 6/22/16 10:45, Alan Bateman wrote:



On 22/06/2016 18:09, stanislav lukyanov wrote:

Hi Serguei,

What is the expected behavior when passed string is not a valid 
package name (e.g. contains illegal characters)?
I think I'd expect JVMTI_ERROR_ILLEGAL_ARGUMENT to be returned in 
that case.


Now it looks like the method will return unnamed module not only for 
an empty string

but for any string that is not a known package name.
If it's correct, I think the empty string case shouldn't be 
described explicitly.
The empty string is the unnamed module case and so the function 
should always return the unamed module.


You question on whether this function checks for invalid identifiers 
is a good question as that is currently not specified.


I'd suggest to specify this function does not check for invalid 
characters (or identifiers).

Please, let me know if you have objections.


Alan, please, let me know if you support the Stanislav's suggestion.
I can go ahead and implement returning the JVMTI_ERROR_ILLEGAL_ARGUMENT 
for invalid package names.


Thanks,
Serguei



Thanks,
Serguei




-Alan

Re: Jigsaw Enhancement RFR round #2: 8159145 Add JVMTI function GetModuleByPackageName

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

Hi Stanislav,

Thank you for looking at this update and the comments!

On 6/22/16 10:09, stanislav lukyanov wrote:

Hi Serguei,

What is the expected behavior when passed string is not a valid 
package name (e.g. contains illegal characters)?
I think I'd expect JVMTI_ERROR_ILLEGAL_ARGUMENT to be returned in that 
case.


I initially was thinking the same but currently have a doubt it is 
really needed.

Let's find out if we can reach a consensus here.




Now it looks like the method will return unnamed module not only for 
an empty string

but for any string that is not a known package name.


Yes, this is correct.
We are not able to recognize non-existing package names and always 
return the unnamed package.

I'm still not sure we want to explicitly specify it.


If it's correct, I think the empty string case shouldn't be described 
explicitly.


I agree, it can confuse the users.
I still prefer to explicitly specify it.
The question is if we want to explicitly specify that we always return 
the unnamed package for non-existing packages.


Thanks,
Serguei




Thanks,
Stas

On 22.06.2016 16:05, serguei.spit...@oracle.com wrote:
I've updated the libGetModuleByPkgTest.c in place in the webrev #2 
with minor improvements.

Sorry, if it spoiled your work.

Thanks,
Serguei


On 6/22/16 04:07, serguei.spit...@oracle.com wrote:
Here are new hotspot webrev where I've fixed the comments from Alan 
and Christian.


Hotspot:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2016/hotspot/8159145-jigsaw-jvmti-pkg.2 




The Jdk webrev is the same:
http://javaweb.sfbay.sun.com/java/svc/ss45998/webrevs/2016/hotspot/8159145-jigsaw-jvmti-pkg.jdk1/ 




Thanks,
Serguei


On 6/21/16 02:54, serguei.spit...@oracle.com wrote:


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


The Hotspot webrev:
http://javaweb.sfbay.sun.com/java/svc/ss45998/webrevs/2016/hotspot/8159145-jigsaw-jvmti-pkg.1/ 



The Jdk webrev:
http://javaweb.sfbay.sun.com/java/svc/ss45998/webrevs/2016/hotspot/8159145-jigsaw-jvmti-pkg.jdk1/ 




Summary:

JVM TI agents that instrument code in named modules need the Module 
at class load time.


  One way to do this is by introducing a new ClassFileLoadHook that 
takes an additional parameter but this approach is disruptive.
  The alternative option is a JVM TI function that maps a 
classloader + package name to a module.
  We were initially not confident with this approach so we 
introduced it as JVM function JVM_GetModuleByPackageName.
  Based on experience to date then this approach seems okay and so 
this function needs to be promoted to a JVMTI function.


  This fix is to introduce new JVMTI function GetModuleByPackageName.
  It includes new jtreg test with native JVMTI agent.

  A CCC is fast-tracked and getting an approval is in progress.

Testing:
   Run newly developed jtreg test.

Thanks,
Serguei

Re: Jigsaw Enhancement RFR round #2: 8159145 Add JVMTI function GetModuleByPackageName

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

On 6/22/16 10:45, Alan Bateman wrote:



On 22/06/2016 18:09, stanislav lukyanov wrote:

Hi Serguei,

What is the expected behavior when passed string is not a valid 
package name (e.g. contains illegal characters)?
I think I'd expect JVMTI_ERROR_ILLEGAL_ARGUMENT to be returned in 
that case.


Now it looks like the method will return unnamed module not only for 
an empty string

but for any string that is not a known package name.
If it's correct, I think the empty string case shouldn't be described 
explicitly.
The empty string is the unnamed module case and so the function should 
always return the unamed module.


You question on whether this function checks for invalid identifiers 
is a good question as that is currently not specified.


I'd suggest to specify this function does not check for invalid 
characters (or identifiers).

Please, let me know if you have objections.

Thanks,
Serguei




-Alan

Re: Jigsaw Enhancement RFR round #2: 8159145 Add JVMTI function GetModuleByPackageName

[Alan Bateman]

On 22/06/2016 18:09, stanislav lukyanov wrote:

Hi Serguei,

What is the expected behavior when passed string is not a valid 
package name (e.g. contains illegal characters)?
I think I'd expect JVMTI_ERROR_ILLEGAL_ARGUMENT to be returned in that 
case.


Now it looks like the method will return unnamed module not only for 
an empty string

but for any string that is not a known package name.
If it's correct, I think the empty string case shouldn't be described 
explicitly.
The empty string is the unnamed module case and so the function should 
always return the unamed module.


You question on whether this function checks for invalid identifiers is 
a good question as that is currently not specified.


-Alan

Re: Jigsaw Enhancement RFR round #2: 8159145 Add JVMTI function GetModuleByPackageName

[Jiangli Zhou]

Hi Serguei,

The comment in the following assert is outdated. There is no 
get_module_from_pkg. Should it be changed to get_module_by_package_name()?

 804   assert(ModuleEntryTable::javabase_defined(),
 805  "Attempt to call get_module_from_pkg before java.base is 
defined");

It’s not part of your change, I just happen to notice it. In the same 
get_module_by_package_name(), both the ‘if’ and ‘else’ cases have return 
statement. The ‘return NULL’ at line 825 seems will never reached and not 
needed. 

Thanks,
Jiangli

> On Jun 22, 2016, at 4:07 AM, serguei.spit...@oracle.com wrote:
> 
> Here are new hotspot webrev where I've fixed the comments from Alan and 
> Christian.
> 
> Hotspot:
> http://cr.openjdk.java.net/~sspitsyn/webrevs/2016/hotspot/8159145-jigsaw-jvmti-pkg.2
> 
> 
> The Jdk webrev is the same:
> http://javaweb.sfbay.sun.com/java/svc/ss45998/webrevs/2016/hotspot/8159145-jigsaw-jvmti-pkg.jdk1/
> 
> 
> Thanks,
> Serguei
> 
> 
> On 6/21/16 02:54, serguei.spit...@oracle.com wrote:
>> 
>> Please, review the Jigsaw fix for the enhancement:
>> https://bugs.openjdk.java.net/browse/JDK-8159145
>> 
>> 
>> The Hotspot webrev:
>> http://javaweb.sfbay.sun.com/java/svc/ss45998/webrevs/2016/hotspot/8159145-jigsaw-jvmti-pkg.1/
>> 
>> The Jdk webrev:
>> http://javaweb.sfbay.sun.com/java/svc/ss45998/webrevs/2016/hotspot/8159145-jigsaw-jvmti-pkg.jdk1/
>> 
>> 
>> Summary:
>> 
>> JVM TI agents that instrument code in named modules need the Module at class 
>> load time.
>> 
>>  One way to do this is by introducing a new ClassFileLoadHook that takes an 
>> additional parameter but this approach is disruptive.
>>  The alternative option is a JVM TI function that maps a classloader + 
>> package name to a module.
>>  We were initially not confident with this approach so we introduced it as 
>> JVM function JVM_GetModuleByPackageName.
>>  Based on experience to date then this approach seems okay and so this 
>> function needs to be promoted to a JVMTI function.
>> 
>>  This fix is to introduce new JVMTI function GetModuleByPackageName.
>>  It includes new jtreg test with native JVMTI agent.
>> 
>>  A CCC is fast-tracked and getting an approval is in progress.
>> 
>> Testing:
>>   Run newly developed jtreg test.
>> 
>> Thanks,
>> Serguei
> 

Re: Jigsaw Enhancement RFR round #2: 8159145 Add JVMTI function GetModuleByPackageName

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

I've updated the libGetModuleByPkgTest.c in place in the webrev #2 with 
minor improvements.

Sorry, if it spoiled your work.

Thanks,
Serguei


On 6/22/16 04:07, serguei.spit...@oracle.com wrote:
Here are new hotspot webrev where I've fixed the comments from Alan 
and Christian.


Hotspot:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2016/hotspot/8159145-jigsaw-jvmti-pkg.2


The Jdk webrev is the same:
http://javaweb.sfbay.sun.com/java/svc/ss45998/webrevs/2016/hotspot/8159145-jigsaw-jvmti-pkg.jdk1/


Thanks,
Serguei


On 6/21/16 02:54, serguei.spit...@oracle.com wrote:


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


The Hotspot webrev:
http://javaweb.sfbay.sun.com/java/svc/ss45998/webrevs/2016/hotspot/8159145-jigsaw-jvmti-pkg.1/

The Jdk webrev:
http://javaweb.sfbay.sun.com/java/svc/ss45998/webrevs/2016/hotspot/8159145-jigsaw-jvmti-pkg.jdk1/


Summary:

JVM TI agents that instrument code in named modules need the Module 
at class load time.


  One way to do this is by introducing a new ClassFileLoadHook that 
takes an additional parameter but this approach is disruptive.
  The alternative option is a JVM TI function that maps a classloader 
+ package name to a module.
  We were initially not confident with this approach so we introduced 
it as JVM function JVM_GetModuleByPackageName.
  Based on experience to date then this approach seems okay and so 
this function needs to be promoted to a JVMTI function.


  This fix is to introduce new JVMTI function GetModuleByPackageName.
  It includes new jtreg test with native JVMTI agent.

  A CCC is fast-tracked and getting an approval is in progress.

Testing:
   Run newly developed jtreg test.

Thanks,
Serguei

Jigsaw Enhancement RFR round #2: 8159145 Add JVMTI function GetModuleByPackageName

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

Here are new hotspot webrev where I've fixed the comments from Alan and 
Christian.


Hotspot:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2016/hotspot/8159145-jigsaw-jvmti-pkg.2


The Jdk webrev is the same:
http://javaweb.sfbay.sun.com/java/svc/ss45998/webrevs/2016/hotspot/8159145-jigsaw-jvmti-pkg.jdk1/


Thanks,
Serguei


On 6/21/16 02:54, serguei.spit...@oracle.com wrote:


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


The Hotspot webrev:
http://javaweb.sfbay.sun.com/java/svc/ss45998/webrevs/2016/hotspot/8159145-jigsaw-jvmti-pkg.1/

The Jdk webrev:
http://javaweb.sfbay.sun.com/java/svc/ss45998/webrevs/2016/hotspot/8159145-jigsaw-jvmti-pkg.jdk1/


Summary:

JVM TI agents that instrument code in named modules need the Module at 
class load time.


  One way to do this is by introducing a new ClassFileLoadHook that 
takes an additional parameter but this approach is disruptive.
  The alternative option is a JVM TI function that maps a classloader 
+ package name to a module.
  We were initially not confident with this approach so we introduced 
it as JVM function JVM_GetModuleByPackageName.
  Based on experience to date then this approach seems okay and so 
this function needs to be promoted to a JVMTI function.


  This fix is to introduce new JVMTI function GetModuleByPackageName.
  It includes new jtreg test with native JVMTI agent.

  A CCC is fast-tracked and getting an approval is in progress.

Testing:
   Run newly developed jtreg test.

Thanks,
Serguei

Re: Jigsaw Enhancement RFR: 8159145 Add JVMTI function GetModuleByPackageName

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

On 6/22/16 02:02, Alan Bateman wrote:



On 22/06/2016 10:00, serguei.spit...@oracle.com wrote:

Alan,

Now I prefer to separate both changes.
My plan was to separate dropping the JVM_* function as we discussed 
previously.
The JPLISAgent can be updated as a part of adding back the transform 
method

class loader argument as the change is pretty trivial.
But I can make this change as a part of this fix if you think it is 
more appropriate.


Two steps is okay with me, I just want to make sure that this part is 
covered.


Yes, I keep it in mind.

Thanks!
Serguei




-Alan

Re: Jigsaw Enhancement RFR: 8159145 Add JVMTI function GetModuleByPackageName

[Alan Bateman]

On 22/06/2016 10:00, serguei.spit...@oracle.com wrote:

Alan,

Now I prefer to separate both changes.
My plan was to separate dropping the JVM_* function as we discussed 
previously.
The JPLISAgent can be updated as a part of adding back the transform 
method

class loader argument as the change is pretty trivial.
But I can make this change as a part of this fix if you think it is 
more appropriate.


Two steps is okay with me, I just want to make sure that this part is 
covered.


-Alan

Re: Jigsaw Enhancement RFR: 8159145 Add JVMTI function GetModuleByPackageName

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

Alan,

Now I prefer to separate both changes.
My plan was to separate dropping the JVM_* function as we discussed 
previously.

The JPLISAgent can be updated as a part of adding back the transform method
class loader argument as the change is pretty trivial.
But I can make this change as a part of this fix if you think it is more 
appropriate.



Thanks,
Serguei



On 6/22/16 01:44, Alan Bateman wrote:

Serguei,

One other question - are you going to drop the (temporary) JVM_* 
function and update the JPLISAgent as part of this or are you leaving 
that to a follow-on issue?


-Alan

Re: Jigsaw Enhancement RFR: 8159145 Add JVMTI function GetModuleByPackageName

[Alan Bateman]

Serguei,

One other question - are you going to drop the (temporary) JVM_* 
function and update the JPLISAgent as part of this or are you leaving 
that to a follow-on issue?


-Alan

Re: Jigsaw Enhancement RFR: 8159145 Add JVMTI function GetModuleByPackageName

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

Hi Christian,

I understand that.
Will do my best to achieve this.
There are some corner cases however.

Thanks,
Serguei


On 6/21/16 10:06, Christian Tornqvist wrote:


Hi Serguei,

>Will check if this works well in this case.

>The unique error messages are already printed in the native code.

Yes, they’re printed on stdout/stderr but the test will fail with a 
“Test failed” exception and our failure matching system wouldn’t be 
able to tell why it failed. It’d be a lot nicer if it’d just throw an 
exception when one of the checks in the native code failed.


Thanks,

Christian

*From:*serguei.spit...@oracle.com [mailto:serguei.spit...@oracle.com]
*Sent:* Tuesday, June 21, 2016 10:59 AM
*To:* Christian Tornqvist ; 'Alan 
Bateman' ; 
serviceability-dev@openjdk.java.net; 'Alexander Kulyakhtin' 
; hotspot-...@openjdk.java.net
*Subject:* Re: Jigsaw Enhancement RFR: 8159145 Add JVMTI function 
GetModuleByPackageName


Hi Christian,

Thank you for looking at it!


On 6/21/16 04:56, Christian Tornqvist wrote:

Hi Serguei,

I’ve only looked at the test code so far, here are some comments:

* You’re not allowed to call System.exit in a jtreg test. If
something fails, you need to throw an exception.


Ok, an exception is Ok with me.


* Instead of a run() method you could simply collapse all this
into the main method like this:

assertEQ(check(), 0);


Will check if this works for me.





* Would it make sense to throw exceptions with useful error
messages from the JNI code instead of returning failed status
codes? Our error matching system would work a lot better with
unique error messages.


Will check if this works well in this case.
The unique error messages are already printed in the native code.

Thanks,
Serguei


Thanks,

Christian

*From:*serguei.spit...@oracle.com

[mailto:serguei.spit...@oracle.com]
*Sent:* Tuesday, June 21, 2016 6:49 AM
*To:* Alan Bateman 
;
serviceability-dev@openjdk.java.net
; Christian Törnqvist

; Alexander Kulyakhtin

;
hotspot-...@openjdk.java.net 
Developers 

*Subject:* Re: Jigsaw Enhancement RFR: 8159145 Add JVMTI function
GetModuleByPackageName

On 6/21/16 03:23, Alan Bateman wrote:

On 21/06/2016 10:54, serguei.spit...@oracle.com
 wrote:


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


The Hotspot webrev:

http://javaweb.sfbay.sun.com/java/svc/ss45998/webrevs/2016/hotspot/8159145-jigsaw-jvmti-pkg.1/

The Jdk webrev:

http://javaweb.sfbay.sun.com/java/svc/ss45998/webrevs/2016/hotspot/8159145-jigsaw-jvmti-pkg.jdk1/

Serguei - can you put the patches on cr.openjdk.java.net?


Sorry.
My initial plan was to write an nsk.jvmti test and review it on a
confidential mailing list.
It is why I put the webrevs on the non-public server.
Forgot to switch the server when the test was converted into the
jtreg format.

The public webrevs are:

Hotspot:

http://cr.openjdk.java.net/~sspitsyn/webrevs/2016/hotspot/8159145-jigsaw-jvmti-pkg.1/



Jdk:

http://cr.openjdk.java.net/~sspitsyn/webrevs/2016/hotspot/8159145-jigsaw-jvmti-pkg.jdk1/




Thanks,
Serguei





-Alan