subject:"RFR\(XS\)\: 8234882\: JVM TI StopThread should only allow ThreadDeath"

Re: RFR(XS): 8234882: JVM TI StopThread should only allow ThreadDeath

2020-06-03 Thread serguei.spit...@oracle.com




On 6/3/20 16:10, David Holmes wrote:

Hi Serguei,

On 4/06/2020 6:41 am, serguei.spit...@oracle.com wrote:

Hi David,

The JetBrains confirmed:
   Ability to select the exception is a valuable feature they provide.
   Throwing only ThreadDeath is almost useless.

So, should I close this and related JDI/JDWP enhancements as WNF?


Yes. Sorry about the wasted work here.


No problem, David.

Thanks!
Serguei



Thanks,
David
-


Thanks,
Serguei


On 6/1/20 08:30, serguei.spit...@oracle.com wrote:

Hi David,

I'll check with JetBrains on this.
Thank you to Dan and you for raising this concern.
The JetBrains use case you posted in the CSR looks like valid and 
useful.


Thanks,
Serguei


On 6/1/20 00:46, David Holmes wrote:

Hi Serguei,

Sorry, I think we have to re-think this change. As Dan flags in the 
CSR request debuggers directly expose this API as part of the 
debugger interface, so any change here will directly impact those 
tools. At a minimum I think we would need to consult with the tool 
developers about the impact of making this change, as well as 
whether it makes any practical difference in the sense that there 
may be other (less convenient but still available) mechanisms to 
achieve the same goal in a debugger or agent.


David

On 31/05/2020 5:50 pm, serguei.spit...@oracle.com wrote:

Hi David,

Also jumping to end.

On 5/30/20 06:50, David Holmes wrote:

Hi Serguei,

Jumping to the end for now ...

On 30/05/2020 5:50 am, serguei.spit...@oracle.com wrote:

Hi David and reviewers,

The updated webrev version is:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.2/src/ 



This update adds testing that StopThread can return 
JVMTI_ERROR_INVALID_OBJECT error code.


The JVM TI StopThread spec is:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.2/docs/specs/jvmti.html#StopThread 




There is a couple of comments below.


On 5/29/20 06:18, David Holmes wrote:

On 29/05/2020 6:24 pm, serguei.spit...@oracle.com wrote:

On 5/29/20 00:56, serguei.spit...@oracle.com wrote:

On 5/29/20 00:42, serguei.spit...@oracle.com wrote:

Hi David,

Thank you for reviewing this!

On 5/28/20 23:57, David Holmes wrote:

Hi Serguei,

On 28/05/2020 3:12 pm, serguei.spit...@oracle.com wrote:

Hi David,

I've updated the CSR and webrev in place.

The changes are:
  - addressed David's suggestion to rephrase StopThread 
description change
  - replaced JVMTI_ERROR_INVALID_OBJECT with 
JVMTI_ERROR_ILLEGAL_ARGUMENT
  - updated the implementation in jvmtiEnv.cpp to return 
JVMTI_ERROR_ILLEGAL_ARGUMENT
  - updated one of the nsk.jvmti StopThread tests to check 
error case with the JVMTI_ERROR_ILLEGAL_ARGUMENT



I'm reposting the links for convenience.

Enhancement:
https://bugs.openjdk.java.net/browse/JDK-8234882

CSR draft:
https://bugs.openjdk.java.net/browse/JDK-8245853


Spec updates are good - thanks.


Thank you for the CSR review.


Webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.1/src/ 



src/hotspot/share/prims/jvmtiEnv.cpp

The ThreadDeath check is fine but I'm a bit confused about 
the additional null check that leads to 
JVMTI_ERROR_INVALID_OBJECT. I can't see how 
resolve_external_guard can return NULL when not passed in 
NULL. Nor why that would result in 
JVMTI_ERROR_INVALID_OBJECT rather than 
JVMTI_ERROR_NULL_POINTER. And I note 
JVMTI_ERROR_NULL_POINTER is not even a listed error for 
StopThread! This part of the change seems unrelated to this 
issue.


I was also surprised with the JVMTI_ERROR_NULL_POINTER and 
JVMTI_ERROR_INVALID_OBJECT error codes.
The JVM TI spec automatic generation adds these two error 
codes for a jobject parameter.


Also, they both are from the Universal Errors section:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.1/docs/specs/jvmti.html#universal-error 



You can find a link to this section at the start of the 
Error section:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.1/docs/specs/jvmti.html#StopThread 



My understanding (not sure, it is right) is that NULL has to 
be reported with JVMTI_ERROR_NULL_POINTER and a bad
jobject (for instance, a WeakReference with a GC-ed target) 
has to be reported with JVMTI_ERROR_INVALID_OBJECT.
At least, I was not able to construct a test case to get 
this error code returned.
So, I'm puzzled with this. I'll try to find some examples 
with JVMTI_ERROR_NULL_POINTER errors.


Found the explanation.
The JDI file:
src/jdk.jdi/share/classes/com/sun/tools/jdi/JDWPException.java

has a fragment that translate the INVALID_OBJECT error to the 
ObjectCollectedException:

    RuntimeException toJDIException() {
    switch (errorCode) {
    case JDWP.Error.INVALID_OBJECT:
    return new ObjectCollectedException();

So, the INVALID_OBJECT is for a jobject handle that is 
referencing a collected object.
It means that previous implementation incorrectly returned 
JVMTI_ERROR_NULL_POINTER error code.


I should create

Re: RFR(XS): 8234882: JVM TI StopThread should only allow ThreadDeath

2020-06-03 Thread David Holmes


Hi Serguei,

On 4/06/2020 6:41 am, serguei.spit...@oracle.com wrote:

Hi David,

The JetBrains confirmed:
   Ability to select the exception is a valuable feature they provide.
   Throwing only ThreadDeath is almost useless.

So, should I close this and related JDI/JDWP enhancements as WNF?


Yes. Sorry about the wasted work here.

Thanks,
David
-


Thanks,
Serguei


On 6/1/20 08:30, serguei.spit...@oracle.com wrote:

Hi David,

I'll check with JetBrains on this.
Thank you to Dan and you for raising this concern.
The JetBrains use case you posted in the CSR looks like valid and useful.

Thanks,
Serguei


On 6/1/20 00:46, David Holmes wrote:

Hi Serguei,

Sorry, I think we have to re-think this change. As Dan flags in the 
CSR request debuggers directly expose this API as part of the 
debugger interface, so any change here will directly impact those 
tools. At a minimum I think we would need to consult with the tool 
developers about the impact of making this change, as well as whether 
it makes any practical difference in the sense that there may be 
other (less convenient but still available) mechanisms to achieve the 
same goal in a debugger or agent.


David

On 31/05/2020 5:50 pm, serguei.spit...@oracle.com wrote:

Hi David,

Also jumping to end.

On 5/30/20 06:50, David Holmes wrote:

Hi Serguei,

Jumping to the end for now ...

On 30/05/2020 5:50 am, serguei.spit...@oracle.com wrote:

Hi David and reviewers,

The updated webrev version is:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.2/src/ 



This update adds testing that StopThread can return 
JVMTI_ERROR_INVALID_OBJECT error code.


The JVM TI StopThread spec is:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.2/docs/specs/jvmti.html#StopThread 




There is a couple of comments below.


On 5/29/20 06:18, David Holmes wrote:

On 29/05/2020 6:24 pm, serguei.spit...@oracle.com wrote:

On 5/29/20 00:56, serguei.spit...@oracle.com wrote:

On 5/29/20 00:42, serguei.spit...@oracle.com wrote:

Hi David,

Thank you for reviewing this!

On 5/28/20 23:57, David Holmes wrote:

Hi Serguei,

On 28/05/2020 3:12 pm, serguei.spit...@oracle.com wrote:

Hi David,

I've updated the CSR and webrev in place.

The changes are:
  - addressed David's suggestion to rephrase StopThread 
description change
  - replaced JVMTI_ERROR_INVALID_OBJECT with 
JVMTI_ERROR_ILLEGAL_ARGUMENT
  - updated the implementation in jvmtiEnv.cpp to return 
JVMTI_ERROR_ILLEGAL_ARGUMENT
  - updated one of the nsk.jvmti StopThread tests to check 
error case with the JVMTI_ERROR_ILLEGAL_ARGUMENT



I'm reposting the links for convenience.

Enhancement:
https://bugs.openjdk.java.net/browse/JDK-8234882

CSR draft:
https://bugs.openjdk.java.net/browse/JDK-8245853


Spec updates are good - thanks.


Thank you for the CSR review.


Webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.1/src/ 



src/hotspot/share/prims/jvmtiEnv.cpp

The ThreadDeath check is fine but I'm a bit confused about 
the additional null check that leads to 
JVMTI_ERROR_INVALID_OBJECT. I can't see how 
resolve_external_guard can return NULL when not passed in 
NULL. Nor why that would result in JVMTI_ERROR_INVALID_OBJECT 
rather than JVMTI_ERROR_NULL_POINTER. And I note 
JVMTI_ERROR_NULL_POINTER is not even a listed error for 
StopThread! This part of the change seems unrelated to this 
issue.


I was also surprised with the JVMTI_ERROR_NULL_POINTER and 
JVMTI_ERROR_INVALID_OBJECT error codes.
The JVM TI spec automatic generation adds these two error 
codes for a jobject parameter.


Also, they both are from the Universal Errors section:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.1/docs/specs/jvmti.html#universal-error 



You can find a link to this section at the start of the Error 
section:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.1/docs/specs/jvmti.html#StopThread 



My understanding (not sure, it is right) is that NULL has to 
be reported with JVMTI_ERROR_NULL_POINTER and a bad
jobject (for instance, a WeakReference with a GC-ed target) 
has to be reported with JVMTI_ERROR_INVALID_OBJECT.
At least, I was not able to construct a test case to get this 
error code returned.
So, I'm puzzled with this. I'll try to find some examples with 
JVMTI_ERROR_NULL_POINTER errors.


Found the explanation.
The JDI file:
src/jdk.jdi/share/classes/com/sun/tools/jdi/JDWPException.java

has a fragment that translate the INVALID_OBJECT error to the 
ObjectCollectedException:

    RuntimeException toJDIException() {
    switch (errorCode) {
    case JDWP.Error.INVALID_OBJECT:
    return new ObjectCollectedException();

So, the INVALID_OBJECT is for a jobject handle that is 
referencing a collected object.
It means that previous implementation incorrectly returned 
JVMTI_ERROR_NULL_POINTER error code.


I should create and delete local or global ref to construct a 
test case for this.


Interesting th

Re: RFR(XS): 8234882: JVM TI StopThread should only allow ThreadDeath

2020-06-03 Thread serguei.spit...@oracle.com


Hi David,

The JetBrains confirmed:
  Ability to select the exception is a valuable feature they provide.
  Throwing only ThreadDeath is almost useless.

So, should I close this and related JDI/JDWP enhancements as WNF?

Thanks,
Serguei


On 6/1/20 08:30, serguei.spit...@oracle.com wrote:

Hi David,

I'll check with JetBrains on this.
Thank you to Dan and you for raising this concern.
The JetBrains use case you posted in the CSR looks like valid and useful.

Thanks,
Serguei


On 6/1/20 00:46, David Holmes wrote:

Hi Serguei,

Sorry, I think we have to re-think this change. As Dan flags in the 
CSR request debuggers directly expose this API as part of the 
debugger interface, so any change here will directly impact those 
tools. At a minimum I think we would need to consult with the tool 
developers about the impact of making this change, as well as whether 
it makes any practical difference in the sense that there may be 
other (less convenient but still available) mechanisms to achieve the 
same goal in a debugger or agent.


David

On 31/05/2020 5:50 pm, serguei.spit...@oracle.com wrote:

Hi David,

Also jumping to end.

On 5/30/20 06:50, David Holmes wrote:

Hi Serguei,

Jumping to the end for now ...

On 30/05/2020 5:50 am, serguei.spit...@oracle.com wrote:

Hi David and reviewers,

The updated webrev version is:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.2/src/ 



This update adds testing that StopThread can return 
JVMTI_ERROR_INVALID_OBJECT error code.


The JVM TI StopThread spec is:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.2/docs/specs/jvmti.html#StopThread 




There is a couple of comments below.


On 5/29/20 06:18, David Holmes wrote:

On 29/05/2020 6:24 pm, serguei.spit...@oracle.com wrote:

On 5/29/20 00:56, serguei.spit...@oracle.com wrote:

On 5/29/20 00:42, serguei.spit...@oracle.com wrote:

Hi David,

Thank you for reviewing this!

On 5/28/20 23:57, David Holmes wrote:

Hi Serguei,

On 28/05/2020 3:12 pm, serguei.spit...@oracle.com wrote:

Hi David,

I've updated the CSR and webrev in place.

The changes are:
  - addressed David's suggestion to rephrase StopThread 
description change
  - replaced JVMTI_ERROR_INVALID_OBJECT with 
JVMTI_ERROR_ILLEGAL_ARGUMENT
  - updated the implementation in jvmtiEnv.cpp to return 
JVMTI_ERROR_ILLEGAL_ARGUMENT
  - updated one of the nsk.jvmti StopThread tests to check 
error case with the JVMTI_ERROR_ILLEGAL_ARGUMENT



I'm reposting the links for convenience.

Enhancement:
https://bugs.openjdk.java.net/browse/JDK-8234882

CSR draft:
https://bugs.openjdk.java.net/browse/JDK-8245853


Spec updates are good - thanks.


Thank you for the CSR review.


Webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.1/src/ 



src/hotspot/share/prims/jvmtiEnv.cpp

The ThreadDeath check is fine but I'm a bit confused about 
the additional null check that leads to 
JVMTI_ERROR_INVALID_OBJECT. I can't see how 
resolve_external_guard can return NULL when not passed in 
NULL. Nor why that would result in JVMTI_ERROR_INVALID_OBJECT 
rather than JVMTI_ERROR_NULL_POINTER. And I note 
JVMTI_ERROR_NULL_POINTER is not even a listed error for 
StopThread! This part of the change seems unrelated to this 
issue.


I was also surprised with the JVMTI_ERROR_NULL_POINTER and 
JVMTI_ERROR_INVALID_OBJECT error codes.
The JVM TI spec automatic generation adds these two error 
codes for a jobject parameter.


Also, they both are from the Universal Errors section:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.1/docs/specs/jvmti.html#universal-error 



You can find a link to this section at the start of the Error 
section:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.1/docs/specs/jvmti.html#StopThread 



My understanding (not sure, it is right) is that NULL has to 
be reported with JVMTI_ERROR_NULL_POINTER and a bad
jobject (for instance, a WeakReference with a GC-ed target) 
has to be reported with JVMTI_ERROR_INVALID_OBJECT.
At least, I was not able to construct a test case to get this 
error code returned.
So, I'm puzzled with this. I'll try to find some examples with 
JVMTI_ERROR_NULL_POINTER errors.


Found the explanation.
The JDI file:
src/jdk.jdi/share/classes/com/sun/tools/jdi/JDWPException.java

has a fragment that translate the INVALID_OBJECT error to the 
ObjectCollectedException:

    RuntimeException toJDIException() {
    switch (errorCode) {
    case JDWP.Error.INVALID_OBJECT:
    return new ObjectCollectedException();

So, the INVALID_OBJECT is for a jobject handle that is 
referencing a collected object.
It means that previous implementation incorrectly returned 
JVMTI_ERROR_NULL_POINTER error code.


I should create and delete local or global ref to construct a 
test case for this.


Interesting that the JDWPException::toJDIException() does not 
convert the ILLEGAL_ARGUMENT error code to an 
IllegalArgumentException.

I've just ad

Re: RFR(XS): 8234882: JVM TI StopThread should only allow ThreadDeath

2020-06-01 Thread serguei.spit...@oracle.com


Hi David,

I'll check with JetBrains on this.
Thank you to Dan and you for raising this concern.
The JetBrains use case you posted in the CSR looks like valid and useful.

Thanks,
Serguei


On 6/1/20 00:46, David Holmes wrote:

Hi Serguei,

Sorry, I think we have to re-think this change. As Dan flags in the 
CSR request debuggers directly expose this API as part of the debugger 
interface, so any change here will directly impact those tools. At a 
minimum I think we would need to consult with the tool developers 
about the impact of making this change, as well as whether it makes 
any practical difference in the sense that there may be other (less 
convenient but still available) mechanisms to achieve the same goal in 
a debugger or agent.


David

On 31/05/2020 5:50 pm, serguei.spit...@oracle.com wrote:

Hi David,

Also jumping to end.

On 5/30/20 06:50, David Holmes wrote:

Hi Serguei,

Jumping to the end for now ...

On 30/05/2020 5:50 am, serguei.spit...@oracle.com wrote:

Hi David and reviewers,

The updated webrev version is:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.2/src/ 



This update adds testing that StopThread can return 
JVMTI_ERROR_INVALID_OBJECT error code.


The JVM TI StopThread spec is:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.2/docs/specs/jvmti.html#StopThread 




There is a couple of comments below.


On 5/29/20 06:18, David Holmes wrote:

On 29/05/2020 6:24 pm, serguei.spit...@oracle.com wrote:

On 5/29/20 00:56, serguei.spit...@oracle.com wrote:

On 5/29/20 00:42, serguei.spit...@oracle.com wrote:

Hi David,

Thank you for reviewing this!

On 5/28/20 23:57, David Holmes wrote:

Hi Serguei,

On 28/05/2020 3:12 pm, serguei.spit...@oracle.com wrote:

Hi David,

I've updated the CSR and webrev in place.

The changes are:
  - addressed David's suggestion to rephrase StopThread 
description change
  - replaced JVMTI_ERROR_INVALID_OBJECT with 
JVMTI_ERROR_ILLEGAL_ARGUMENT
  - updated the implementation in jvmtiEnv.cpp to return 
JVMTI_ERROR_ILLEGAL_ARGUMENT
  - updated one of the nsk.jvmti StopThread tests to check 
error case with the JVMTI_ERROR_ILLEGAL_ARGUMENT



I'm reposting the links for convenience.

Enhancement:
https://bugs.openjdk.java.net/browse/JDK-8234882

CSR draft:
https://bugs.openjdk.java.net/browse/JDK-8245853


Spec updates are good - thanks.


Thank you for the CSR review.


Webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.1/src/ 



src/hotspot/share/prims/jvmtiEnv.cpp

The ThreadDeath check is fine but I'm a bit confused about the 
additional null check that leads to 
JVMTI_ERROR_INVALID_OBJECT. I can't see how 
resolve_external_guard can return NULL when not passed in 
NULL. Nor why that would result in JVMTI_ERROR_INVALID_OBJECT 
rather than JVMTI_ERROR_NULL_POINTER. And I note 
JVMTI_ERROR_NULL_POINTER is not even a listed error for 
StopThread! This part of the change seems unrelated to this 
issue.


I was also surprised with the JVMTI_ERROR_NULL_POINTER and 
JVMTI_ERROR_INVALID_OBJECT error codes.
The JVM TI spec automatic generation adds these two error codes 
for a jobject parameter.


Also, they both are from the Universal Errors section:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.1/docs/specs/jvmti.html#universal-error 



You can find a link to this section at the start of the Error 
section:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.1/docs/specs/jvmti.html#StopThread 



My understanding (not sure, it is right) is that NULL has to be 
reported with JVMTI_ERROR_NULL_POINTER and a bad
jobject (for instance, a WeakReference with a GC-ed target) has 
to be reported with JVMTI_ERROR_INVALID_OBJECT.
At least, I was not able to construct a test case to get this 
error code returned.
So, I'm puzzled with this. I'll try to find some examples with 
JVMTI_ERROR_NULL_POINTER errors.


Found the explanation.
The JDI file:
src/jdk.jdi/share/classes/com/sun/tools/jdi/JDWPException.java

has a fragment that translate the INVALID_OBJECT error to the 
ObjectCollectedException:

    RuntimeException toJDIException() {
    switch (errorCode) {
    case JDWP.Error.INVALID_OBJECT:
    return new ObjectCollectedException();

So, the INVALID_OBJECT is for a jobject handle that is 
referencing a collected object.
It means that previous implementation incorrectly returned 
JVMTI_ERROR_NULL_POINTER error code.


I should create and delete local or global ref to construct a 
test case for this.


Interesting that the JDWPException::toJDIException() does not 
convert the ILLEGAL_ARGUMENT error code to an 
IllegalArgumentException.

I've just added this conversion.


Given the definition of JDWP INVALID_OBJECT then obviously JDI 
converts it to ObjectCollectedException.


So reading further in JNI spec:

"Weak global references are a special kind of global reference. 
Unlike normal global references, a weak global reference allows

Re: RFR(XS): 8234882: JVM TI StopThread should only allow ThreadDeath

2020-06-01 Thread David Holmes


Hi Serguei,

Sorry, I think we have to re-think this change. As Dan flags in the CSR 
request debuggers directly expose this API as part of the debugger 
interface, so any change here will directly impact those tools. At a 
minimum I think we would need to consult with the tool developers about 
the impact of making this change, as well as whether it makes any 
practical difference in the sense that there may be other (less 
convenient but still available) mechanisms to achieve the same goal in a 
debugger or agent.


David

On 31/05/2020 5:50 pm, serguei.spit...@oracle.com wrote:

Hi David,

Also jumping to end.

On 5/30/20 06:50, David Holmes wrote:

Hi Serguei,

Jumping to the end for now ...

On 30/05/2020 5:50 am, serguei.spit...@oracle.com wrote:

Hi David and reviewers,

The updated webrev version is:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.2/src/ 



This update adds testing that StopThread can return 
JVMTI_ERROR_INVALID_OBJECT error code.


The JVM TI StopThread spec is:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.2/docs/specs/jvmti.html#StopThread 




There is a couple of comments below.


On 5/29/20 06:18, David Holmes wrote:

On 29/05/2020 6:24 pm, serguei.spit...@oracle.com wrote:

On 5/29/20 00:56, serguei.spit...@oracle.com wrote:

On 5/29/20 00:42, serguei.spit...@oracle.com wrote:

Hi David,

Thank you for reviewing this!

On 5/28/20 23:57, David Holmes wrote:

Hi Serguei,

On 28/05/2020 3:12 pm, serguei.spit...@oracle.com wrote:

Hi David,

I've updated the CSR and webrev in place.

The changes are:
  - addressed David's suggestion to rephrase StopThread 
description change
  - replaced JVMTI_ERROR_INVALID_OBJECT with 
JVMTI_ERROR_ILLEGAL_ARGUMENT
  - updated the implementation in jvmtiEnv.cpp to return 
JVMTI_ERROR_ILLEGAL_ARGUMENT
  - updated one of the nsk.jvmti StopThread tests to check 
error case with the JVMTI_ERROR_ILLEGAL_ARGUMENT



I'm reposting the links for convenience.

Enhancement:
https://bugs.openjdk.java.net/browse/JDK-8234882

CSR draft:
https://bugs.openjdk.java.net/browse/JDK-8245853


Spec updates are good - thanks.


Thank you for the CSR review.


Webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.1/src/ 



src/hotspot/share/prims/jvmtiEnv.cpp

The ThreadDeath check is fine but I'm a bit confused about the 
additional null check that leads to JVMTI_ERROR_INVALID_OBJECT. 
I can't see how resolve_external_guard can return NULL when not 
passed in NULL. Nor why that would result in 
JVMTI_ERROR_INVALID_OBJECT rather than JVMTI_ERROR_NULL_POINTER. 
And I note JVMTI_ERROR_NULL_POINTER is not even a listed error 
for StopThread! This part of the change seems unrelated to this 
issue.


I was also surprised with the JVMTI_ERROR_NULL_POINTER and 
JVMTI_ERROR_INVALID_OBJECT error codes.
The JVM TI spec automatic generation adds these two error codes 
for a jobject parameter.


Also, they both are from the Universal Errors section:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.1/docs/specs/jvmti.html#universal-error 



You can find a link to this section at the start of the Error 
section:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.1/docs/specs/jvmti.html#StopThread 



My understanding (not sure, it is right) is that NULL has to be 
reported with JVMTI_ERROR_NULL_POINTER and a bad
jobject (for instance, a WeakReference with a GC-ed target) has 
to be reported with JVMTI_ERROR_INVALID_OBJECT.
At least, I was not able to construct a test case to get this 
error code returned.
So, I'm puzzled with this. I'll try to find some examples with 
JVMTI_ERROR_NULL_POINTER errors.


Found the explanation.
The JDI file:
src/jdk.jdi/share/classes/com/sun/tools/jdi/JDWPException.java

has a fragment that translate the INVALID_OBJECT error to the 
ObjectCollectedException:

    RuntimeException toJDIException() {
    switch (errorCode) {
    case JDWP.Error.INVALID_OBJECT:
    return new ObjectCollectedException();

So, the INVALID_OBJECT is for a jobject handle that is referencing 
a collected object.
It means that previous implementation incorrectly returned 
JVMTI_ERROR_NULL_POINTER error code.


I should create and delete local or global ref to construct a test 
case for this.


Interesting that the JDWPException::toJDIException() does not 
convert the ILLEGAL_ARGUMENT error code to an 
IllegalArgumentException.

I've just added this conversion.


Given the definition of JDWP INVALID_OBJECT then obviously JDI 
converts it to ObjectCollectedException.


So reading further in JNI spec:

"Weak global references are a special kind of global reference. 
Unlike normal global references, a weak global reference allows the 
underlying Java object to be garbage collected. Weak global 
references may be used in any situation where global or local 
references are used."


So it seems that any function that takes a jobject cxould in fact 
accept a

Re: RFR(XS): 8234882: JVM TI StopThread should only allow ThreadDeath

2020-05-31 Thread serguei.spit...@oracle.com


Hi David,

Also jumping to end.

On 5/30/20 06:50, David Holmes wrote:

Hi Serguei,

Jumping to the end for now ...

On 30/05/2020 5:50 am, serguei.spit...@oracle.com wrote:

Hi David and reviewers,

The updated webrev version is:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.2/src/ 



This update adds testing that StopThread can return 
JVMTI_ERROR_INVALID_OBJECT error code.


The JVM TI StopThread spec is:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.2/docs/specs/jvmti.html#StopThread 




There is a couple of comments below.


On 5/29/20 06:18, David Holmes wrote:

On 29/05/2020 6:24 pm, serguei.spit...@oracle.com wrote:

On 5/29/20 00:56, serguei.spit...@oracle.com wrote:

On 5/29/20 00:42, serguei.spit...@oracle.com wrote:

Hi David,

Thank you for reviewing this!

On 5/28/20 23:57, David Holmes wrote:

Hi Serguei,

On 28/05/2020 3:12 pm, serguei.spit...@oracle.com wrote:

Hi David,

I've updated the CSR and webrev in place.

The changes are:
  - addressed David's suggestion to rephrase StopThread 
description change
  - replaced JVMTI_ERROR_INVALID_OBJECT with 
JVMTI_ERROR_ILLEGAL_ARGUMENT
  - updated the implementation in jvmtiEnv.cpp to return 
JVMTI_ERROR_ILLEGAL_ARGUMENT
  - updated one of the nsk.jvmti StopThread tests to check 
error case with the JVMTI_ERROR_ILLEGAL_ARGUMENT



I'm reposting the links for convenience.

Enhancement:
https://bugs.openjdk.java.net/browse/JDK-8234882

CSR draft:
https://bugs.openjdk.java.net/browse/JDK-8245853


Spec updates are good - thanks.


Thank you for the CSR review.


Webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.1/src/ 



src/hotspot/share/prims/jvmtiEnv.cpp

The ThreadDeath check is fine but I'm a bit confused about the 
additional null check that leads to JVMTI_ERROR_INVALID_OBJECT. 
I can't see how resolve_external_guard can return NULL when not 
passed in NULL. Nor why that would result in 
JVMTI_ERROR_INVALID_OBJECT rather than JVMTI_ERROR_NULL_POINTER. 
And I note JVMTI_ERROR_NULL_POINTER is not even a listed error 
for StopThread! This part of the change seems unrelated to this 
issue.


I was also surprised with the JVMTI_ERROR_NULL_POINTER and 
JVMTI_ERROR_INVALID_OBJECT error codes.
The JVM TI spec automatic generation adds these two error codes 
for a jobject parameter.


Also, they both are from the Universal Errors section:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.1/docs/specs/jvmti.html#universal-error 



You can find a link to this section at the start of the Error 
section:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.1/docs/specs/jvmti.html#StopThread 



My understanding (not sure, it is right) is that NULL has to be 
reported with JVMTI_ERROR_NULL_POINTER and a bad
jobject (for instance, a WeakReference with a GC-ed target) has 
to be reported with JVMTI_ERROR_INVALID_OBJECT.
At least, I was not able to construct a test case to get this 
error code returned.
So, I'm puzzled with this. I'll try to find some examples with 
JVMTI_ERROR_NULL_POINTER errors.


Found the explanation.
The JDI file:
src/jdk.jdi/share/classes/com/sun/tools/jdi/JDWPException.java

has a fragment that translate the INVALID_OBJECT error to the 
ObjectCollectedException:

    RuntimeException toJDIException() {
    switch (errorCode) {
    case JDWP.Error.INVALID_OBJECT:
    return new ObjectCollectedException();

So, the INVALID_OBJECT is for a jobject handle that is referencing 
a collected object.
It means that previous implementation incorrectly returned 
JVMTI_ERROR_NULL_POINTER error code.


I should create and delete local or global ref to construct a test 
case for this.


Interesting that the JDWPException::toJDIException() does not 
convert the ILLEGAL_ARGUMENT error code to an 
IllegalArgumentException.

I've just added this conversion.


Given the definition of JDWP INVALID_OBJECT then obviously JDI 
converts it to ObjectCollectedException.


So reading further in JNI spec:

"Weak global references are a special kind of global reference. 
Unlike normal global references, a weak global reference allows the 
underlying Java object to be garbage collected. Weak global 
references may be used in any situation where global or local 
references are used."


So it seems that any function that takes a jobject cxould in fact 
accept a jweak, in which case JVMTI_ERROR_INVALID_OBJECT is a 
possibility in all cases. So IIUC JNIHandles::resolve_external_guard 
can return NULL if a weak reference has been collected. So the new 
code you propose seems correct.


You are right about weak global references.
I was able to construct a test case for JVMTI_ERROR_INVALID_OBJECT.
The JNI NewGlobalRef and DeleteGlobalRef are used for it.
You can find it in the updated webrev version.

However, this still is unrelated to the current issue and I do not 
see other JVM TI doing checks for this case. So this seems to be a 
much bro

Re: RFR(XS): 8234882: JVM TI StopThread should only allow ThreadDeath

2020-05-30 Thread David Holmes


Hi Serguei,

Jumping to the end for now ...

On 30/05/2020 5:50 am, serguei.spit...@oracle.com wrote:

Hi David and reviewers,

The updated webrev version is:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.2/src/

This update adds testing that StopThread can return 
JVMTI_ERROR_INVALID_OBJECT error code.


The JVM TI StopThread spec is:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.2/docs/specs/jvmti.html#StopThread


There is a couple of comments below.


On 5/29/20 06:18, David Holmes wrote:

On 29/05/2020 6:24 pm, serguei.spit...@oracle.com wrote:

On 5/29/20 00:56, serguei.spit...@oracle.com wrote:

On 5/29/20 00:42, serguei.spit...@oracle.com wrote:

Hi David,

Thank you for reviewing this!

On 5/28/20 23:57, David Holmes wrote:

Hi Serguei,

On 28/05/2020 3:12 pm, serguei.spit...@oracle.com wrote:

Hi David,

I've updated the CSR and webrev in place.

The changes are:
  - addressed David's suggestion to rephrase StopThread 
description change
  - replaced JVMTI_ERROR_INVALID_OBJECT with 
JVMTI_ERROR_ILLEGAL_ARGUMENT
  - updated the implementation in jvmtiEnv.cpp to return 
JVMTI_ERROR_ILLEGAL_ARGUMENT
  - updated one of the nsk.jvmti StopThread tests to check error 
case with the JVMTI_ERROR_ILLEGAL_ARGUMENT



I'm reposting the links for convenience.

Enhancement:
https://bugs.openjdk.java.net/browse/JDK-8234882

CSR draft:
https://bugs.openjdk.java.net/browse/JDK-8245853


Spec updates are good - thanks.


Thank you for the CSR review.


Webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.1/src/ 



src/hotspot/share/prims/jvmtiEnv.cpp

The ThreadDeath check is fine but I'm a bit confused about the 
additional null check that leads to JVMTI_ERROR_INVALID_OBJECT. I 
can't see how resolve_external_guard can return NULL when not 
passed in NULL. Nor why that would result in 
JVMTI_ERROR_INVALID_OBJECT rather than JVMTI_ERROR_NULL_POINTER. 
And I note JVMTI_ERROR_NULL_POINTER is not even a listed error for 
StopThread! This part of the change seems unrelated to this issue.


I was also surprised with the JVMTI_ERROR_NULL_POINTER and 
JVMTI_ERROR_INVALID_OBJECT error codes.
The JVM TI spec automatic generation adds these two error codes for 
a jobject parameter.


Also, they both are from the Universal Errors section:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.1/docs/specs/jvmti.html#universal-error 



You can find a link to this section at the start of the Error section:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.1/docs/specs/jvmti.html#StopThread 



My understanding (not sure, it is right) is that NULL has to be 
reported with JVMTI_ERROR_NULL_POINTER and a bad
jobject (for instance, a WeakReference with a GC-ed target) has to 
be reported with JVMTI_ERROR_INVALID_OBJECT.
At least, I was not able to construct a test case to get this error 
code returned.
So, I'm puzzled with this. I'll try to find some examples with 
JVMTI_ERROR_NULL_POINTER errors.


Found the explanation.
The JDI file:
src/jdk.jdi/share/classes/com/sun/tools/jdi/JDWPException.java

has a fragment that translate the INVALID_OBJECT error to the 
ObjectCollectedException:

    RuntimeException toJDIException() {
    switch (errorCode) {
    case JDWP.Error.INVALID_OBJECT:
    return new ObjectCollectedException();

So, the INVALID_OBJECT is for a jobject handle that is referencing a 
collected object.
It means that previous implementation incorrectly returned 
JVMTI_ERROR_NULL_POINTER error code.


I should create and delete local or global ref to construct a test 
case for this.


Interesting that the JDWPException::toJDIException() does not convert 
the ILLEGAL_ARGUMENT error code to an IllegalArgumentException.

I've just added this conversion.


Given the definition of JDWP INVALID_OBJECT then obviously JDI 
converts it to ObjectCollectedException.


So reading further in JNI spec:

"Weak global references are a special kind of global reference. Unlike 
normal global references, a weak global reference allows the 
underlying Java object to be garbage collected. Weak global references 
may be used in any situation where global or local references are used."


So it seems that any function that takes a jobject cxould in fact 
accept a jweak, in which case JVMTI_ERROR_INVALID_OBJECT is a 
possibility in all cases. So IIUC JNIHandles::resolve_external_guard 
can return NULL if a weak reference has been collected. So the new 
code you propose seems correct.


You are right about weak global references.
I was able to construct a test case for JVMTI_ERROR_INVALID_OBJECT.
The JNI NewGlobalRef and DeleteGlobalRef are used for it.
You can find it in the updated webrev version.

However, this still is unrelated to the current issue and I do not see 
other JVM TI doing checks for this case. So this seems to be a much 
broader issue.

There are many such checks in JVM TI.
For instance, there are checks

Re: RFR(XS): 8234882: JVM TI StopThread should only allow ThreadDeath

2020-05-29 Thread serguei.spit...@oracle.com

Hi David and reviewers,

The updated webrev version is:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.2/src/

This update adds testing that StopThread can return
JVMTI_ERROR_INVALID_OBJECT error code.

The JVM TI StopThread spec is:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.2/docs/specs/jvmti.html#StopThread

There is a couple of comments below.

On 5/29/20 06:18, David Holmes wrote:

On
29/05/2020 6:24 pm, serguei.spit...@oracle.com wrote:

On 5/29/20 00:56,
serguei.spit...@oracle.com wrote:

On 5/29/20 00:42,
serguei.spit...@oracle.com wrote:

Hi David,

Thank you for reviewing this!

On 5/28/20 23:57, David Holmes wrote:

Hi Serguei,

On 28/05/2020 3:12 pm, serguei.spit...@oracle.com wrote:

Hi David,

I've updated the CSR and webrev in place.

The changes are:

- addressed David's suggestion to rephrase StopThread
description change

- replaced JVMTI_ERROR_INVALID_OBJECT with
JVMTI_ERROR_ILLEGAL_ARGUMENT

- updated the implementation in jvmtiEnv.cpp to return
JVMTI_ERROR_ILLEGAL_ARGUMENT

- updated one of the nsk.jvmti StopThread tests to
check error case with the JVMTI_ERROR_ILLEGAL_ARGUMENT

I'm reposting the links for convenience.

Enhancement:

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

CSR draft:

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

Spec updates are good - thanks.

Thank you for the CSR review.

Webrev:

http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.1/src/

src/hotspot/share/prims/jvmtiEnv.cpp

The ThreadDeath check is fine but I'm a bit confused about
the additional null check that leads to
JVMTI_ERROR_INVALID_OBJECT. I can't see how
resolve_external_guard can return NULL when not passed in
NULL. Nor why that would result in
JVMTI_ERROR_INVALID_OBJECT rather than
JVMTI_ERROR_NULL_POINTER. And I note
JVMTI_ERROR_NULL_POINTER is not even a listed error for
StopThread! This part of the change seems unrelated to
this issue.

I was also surprised with the JVMTI_ERROR_NULL_POINTER and
JVMTI_ERROR_INVALID_OBJECT error codes.

The JVM TI spec automatic generation adds these two error
codes for a jobject parameter.

Also, they both are from the Universal Errors section:

http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.1/docs/specs/jvmti.html#universal-error

You can find a link to this section at the start of the
Error section:

http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.1/docs/specs/jvmti.html#StopThread

My understanding (not sure, it is right) is that NULL has to
be reported with JVMTI_ERROR_NULL_POINTER and a bad

jobject (for instance, a WeakReference with a GC-ed target)
has to be reported with JVMTI_ERROR_INVALID_OBJECT.

At least, I was not able to construct a test case to get
this error code returned.

So, I'm puzzled with this. I'll try to find some examples
with JVMTI_ERROR_NULL_POINTER errors.

Found the explanation.

The JDI file:

src/jdk.jdi/share/classes/com/sun/tools/jdi/JDWPException.java

has a fragment that translate the INVALID_OBJECT error to the
ObjectCollectedException:

RuntimeException toJDIException() {

switch (errorCode) {

Re: RFR(XS): 8234882: JVM TI StopThread should only allow ThreadDeath

2020-05-29 Thread David Holmes


On 29/05/2020 6:24 pm, serguei.spit...@oracle.com wrote:

On 5/29/20 00:56, serguei.spit...@oracle.com wrote:

On 5/29/20 00:42, serguei.spit...@oracle.com wrote:

Hi David,

Thank you for reviewing this!

On 5/28/20 23:57, David Holmes wrote:

Hi Serguei,

On 28/05/2020 3:12 pm, serguei.spit...@oracle.com wrote:

Hi David,

I've updated the CSR and webrev in place.

The changes are:
  - addressed David's suggestion to rephrase StopThread description 
change
  - replaced JVMTI_ERROR_INVALID_OBJECT with 
JVMTI_ERROR_ILLEGAL_ARGUMENT
  - updated the implementation in jvmtiEnv.cpp to return 
JVMTI_ERROR_ILLEGAL_ARGUMENT
  - updated one of the nsk.jvmti StopThread tests to check error 
case with the JVMTI_ERROR_ILLEGAL_ARGUMENT



I'm reposting the links for convenience.

Enhancement:
   https://bugs.openjdk.java.net/browse/JDK-8234882

CSR draft:
   https://bugs.openjdk.java.net/browse/JDK-8245853


Spec updates are good - thanks.


Thank you for the CSR review.


Webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.1/src/ 



src/hotspot/share/prims/jvmtiEnv.cpp

The ThreadDeath check is fine but I'm a bit confused about the 
additional null check that leads to JVMTI_ERROR_INVALID_OBJECT. I 
can't see how resolve_external_guard can return NULL when not passed 
in NULL. Nor why that would result in JVMTI_ERROR_INVALID_OBJECT 
rather than JVMTI_ERROR_NULL_POINTER. And I note 
JVMTI_ERROR_NULL_POINTER is not even a listed error for StopThread! 
This part of the change seems unrelated to this issue.


I was also surprised with the JVMTI_ERROR_NULL_POINTER and 
JVMTI_ERROR_INVALID_OBJECT error codes.
The JVM TI spec automatic generation adds these two error codes for a 
jobject parameter.


Also, they both are from the Universal Errors section:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.1/docs/specs/jvmti.html#universal-error 



You can find a link to this section at the start of the Error section:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.1/docs/specs/jvmti.html#StopThread 



My understanding (not sure, it is right) is that NULL has to be 
reported with JVMTI_ERROR_NULL_POINTER and a bad
jobject (for instance, a WeakReference with a GC-ed target) has to be 
reported with JVMTI_ERROR_INVALID_OBJECT.
At least, I was not able to construct a test case to get this error 
code returned.
So, I'm puzzled with this. I'll try to find some examples with 
JVMTI_ERROR_NULL_POINTER errors.


Found the explanation.
The JDI file:
  src/jdk.jdi/share/classes/com/sun/tools/jdi/JDWPException.java

has a fragment that translate the INVALID_OBJECT error to the 
ObjectCollectedException:

    RuntimeException toJDIException() {
    switch (errorCode) {
    case JDWP.Error.INVALID_OBJECT:
    return new ObjectCollectedException();

So, the INVALID_OBJECT is for a jobject handle that is referencing a 
collected object.
It means that previous implementation incorrectly returned 
JVMTI_ERROR_NULL_POINTER error code.


I should create and delete local or global ref to construct a test case 
for this.


Interesting that the JDWPException::toJDIException() does not convert 
the ILLEGAL_ARGUMENT error code to an IllegalArgumentException.

I've just added this conversion.


Given the definition of JDWP INVALID_OBJECT then obviously JDI converts 
it to ObjectCollectedException.


So reading further in JNI spec:

"Weak global references are a special kind of global reference. Unlike 
normal global references, a weak global reference allows the underlying 
Java object to be garbage collected. Weak global references may be used 
in any situation where global or local references are used."


So it seems that any function that takes a jobject cxould in fact accept 
a jweak, in which case JVMTI_ERROR_INVALID_OBJECT is a possibility in 
all cases. So IIUC JNIHandles::resolve_external_guard can return NULL if 
a weak reference has been collected. So the new code you propose seems 
correct.


However, this still is unrelated to the current issue and I do not see 
other JVM TI doing checks for this case. So this seems to be a much 
broader issue.


David
-


Thanks,
Serguei




Thanks,
Serguei

test/hotspot/jtreg/vmTestbase/nsk/jvmti/StopThread/stopthrd006/TestDescription.java 



The copyright year should be change to "2018, 2020,".

Thank you for the catch.
I planned to update the copyright comments.

I'm a little surprised the test doesn't actually check that a valid 
call doesn't produce an error. But that's an existing quirk of the 
test and not something you need to address here (if indeed it needs 
addressing - perhaps there is another test for that).


There are plenty of other nsk.jvmti tests which check valid calls.

Thanks,
Serguei


Thanks,
David
-



Updated JVM TI StopThread spec:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.1/docs/specs/jvmti.html#StopThread 




The old webrev and spec are here:
http://cr.op

Re: RFR(XS): 8234882: JVM TI StopThread should only allow ThreadDeath

2020-05-29 Thread serguei.spit...@oracle.com

On 5/29/20 00:56, serguei.spit...@oracle.com wrote:

On 5/29/20 00:42, serguei.spit...@oracle.com wrote:

Hi David,

Thank you for reviewing this!

On 5/28/20 23:57, David Holmes wrote:

Hi Serguei,

On 28/05/2020 3:12 pm, serguei.spit...@oracle.com wrote:

Hi David,

I've updated the CSR and webrev in place.

The changes are:
- addressed David's suggestion to rephrase StopThread description
change
- replaced JVMTI_ERROR_INVALID_OBJECT with
JVMTI_ERROR_ILLEGAL_ARGUMENT
- updated the implementation in jvmtiEnv.cpp to return
JVMTI_ERROR_ILLEGAL_ARGUMENT
- updated one of the nsk.jvmti StopThread tests to check error
case with the JVMTI_ERROR_ILLEGAL_ARGUMENT

I'm reposting the links for convenience.

Enhancement:
https://bugs.openjdk.java.net/browse/JDK-8234882

CSR draft:
https://bugs.openjdk.java.net/browse/JDK-8245853

Spec updates are good - thanks.

Thank you for the CSR review.

Webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.1/src/

src/hotspot/share/prims/jvmtiEnv.cpp

The ThreadDeath check is fine but I'm a bit confused about the
additional null check that leads to JVMTI_ERROR_INVALID_OBJECT. I
can't see how resolve_external_guard can return NULL when not passed
in NULL. Nor why that would result in JVMTI_ERROR_INVALID_OBJECT
rather than JVMTI_ERROR_NULL_POINTER. And I note
JVMTI_ERROR_NULL_POINTER is not even a listed error for StopThread!
This part of the change seems unrelated to this issue.

I was also surprised with the JVMTI_ERROR_NULL_POINTER and
JVMTI_ERROR_INVALID_OBJECT error codes.
The JVM TI spec automatic generation adds these two error codes for a
jobject parameter.

Also, they both are from the Universal Errors section:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.1/docs/specs/jvmti.html#universal-error

You can find a link to this section at the start of the Error section:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.1/docs/specs/jvmti.html#StopThread

My understanding (not sure, it is right) is that NULL has to be
reported with JVMTI_ERROR_NULL_POINTER and a bad
jobject (for instance, a WeakReference with a GC-ed target) has to be
reported with JVMTI_ERROR_INVALID_OBJECT.
At least, I was not able to construct a test case to get this error
code returned.
So, I'm puzzled with this. I'll try to find some examples with
JVMTI_ERROR_NULL_POINTER errors.

Found the explanation.
The JDI file:
src/jdk.jdi/share/classes/com/sun/tools/jdi/JDWPException.java

has a fragment that translate the INVALID_OBJECT error to the
ObjectCollectedException:

RuntimeException toJDIException() {
switch (errorCode) {
case JDWP.Error.INVALID_OBJECT:
return new ObjectCollectedException();

So, the INVALID_OBJECT is for a jobject handle that is referencing a
collected object.
It means that previous implementation incorrectly returned
JVMTI_ERROR_NULL_POINTER error code.

I should create and delete local or global ref to construct a test case
for this.

Interesting that the JDWPException::toJDIException() does not convert
the ILLEGAL_ARGUMENT error code to an IllegalArgumentException.

I've just added this conversion.

Thanks,
Serguei

test/hotspot/jtreg/vmTestbase/nsk/jvmti/StopThread/stopthrd006/TestDescription.java

The copyright year should be change to "2018, 2020,".

Thank you for the catch.
I planned to update the copyright comments.

I'm a little surprised the test doesn't actually check that a valid
call doesn't produce an error. But that's an existing quirk of the
test and not something you need to address here (if indeed it needs
addressing - perhaps there is another test for that).

There are plenty of other nsk.jvmti tests which check valid calls.

Thanks,
Serguei

Thanks,
David
-

Updated JVM TI StopThread spec:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.1/docs/specs/jvmti.html#StopThread

The old webrev and spec are here:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.0/

Thanks,
Serguei

On 5/27/20 18:03, serguei.spit...@oracle.com wrote:

Hi David,

On 5/27/20 02:00, David Holmes wrote:

On 27/05/2020 6:36 pm, serguei.spit...@oracle.com wrote:

Hi David,

On 5/27/20 00:47, David Holmes wrote:

Hi Serguei,

On 27/05/2020 1:01 pm, serguei.spit...@oracle.com wrote:

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

CSR draft (one CSR reviewer is needed before finalizing it):
https://bugs.openjdk.java.net/browse/JDK-8245853

I have some thoughts on the wording which I will add to the CSR.

Thank you a lot for looking at this!

Also on reflection I think JVMTI_ERROR_ILLEGAL_ARGUMENT would
the best error to use, and it has an equivalent in JDWP and at
the Java level for JDI.

This is an interesting variant, thanks!
We need to balance on several criteria:
1) Compatibility: keep returning error as close as possible to

Re: RFR(XS): 8234882: JVM TI StopThread should only allow ThreadDeath

2020-05-29 Thread serguei.spit...@oracle.com

On 5/29/20 00:59, David Holmes wrote:

On 29/05/2020 5:42 pm, serguei.spit...@oracle.com wrote:

Hi David,

Thank you for reviewing this!

On 5/28/20 23:57, David Holmes wrote:

Hi Serguei,

On 28/05/2020 3:12 pm, serguei.spit...@oracle.com wrote:

Hi David,

I've updated the CSR and webrev in place.

I'm reposting the links for convenience.

Enhancement:
https://bugs.openjdk.java.net/browse/JDK-8234882

CSR draft:
https://bugs.openjdk.java.net/browse/JDK-8245853

Spec updates are good - thanks.

Thank you for the CSR review.

Webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.1/src/

src/hotspot/share/prims/jvmtiEnv.cpp

The ThreadDeath check is fine but I'm a bit confused about the
additional null check that leads to JVMTI_ERROR_INVALID_OBJECT. I
can't see how resolve_external_guard can return NULL when not passed
in NULL. Nor why that would result in JVMTI_ERROR_INVALID_OBJECT
rather than JVMTI_ERROR_NULL_POINTER. And I note
JVMTI_ERROR_NULL_POINTER is not even a listed error for StopThread!
This part of the change seems unrelated to this issue.

I was also surprised with the JVMTI_ERROR_NULL_POINTER and
JVMTI_ERROR_INVALID_OBJECT error codes.
The JVM TI spec automatic generation adds these two error codes for a
jobject parameter.

Also, they both are from the Universal Errors section:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.1/docs/specs/jvmti.html#universal-error

Ah I see. Some functions specify JVMTI_ERROR_NULL_POINTER explicitly.

You can find a link to this section at the start of the Error section:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.1/docs/specs/jvmti.html#StopThread

Got it - thanks.

I'm a bit unclear here. If you pass a jweak then resolving it may
yield a NULL if the referent has been GC'd. But IIUC a plain jobject
is just a JNI global reference to an Object - if the jobject is not
NULL then resolving it to an oop can also not yield a NULL.

You are right, thanks.
I tried but did not succeed in getting this error code with a
WeakReference pointing to NULL object.

You are explained why.
I've already sent you one more message with the explanation of this
error code.

Thanks,
Serguei

But regardless this seems unrelated to current issue and just
complicates matters.

Cheers,
David

At least, I was not able to construct a test case to get this error
code returned.
So, I'm puzzled with this. I'll try to find some examples with
JVMTI_ERROR_NULL_POINTER errors.

test/hotspot/jtreg/vmTestbase/nsk/jvmti/StopThread/stopthrd006/TestDescription.java

The copyright year should be change to "2018, 2020,".

Thank you for the catch.
I planned to update the copyright comments.

There are plenty of other nsk.jvmti tests which check valid calls.

Thanks,
Serguei

Thanks,
David
-

Updated JVM TI StopThread spec:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.1/docs/specs/jvmti.html#StopThread

The old webrev and spec are here:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.0/

Thanks,
Serguei

On 5/27/20 18:03, serguei.spit...@oracle.com wrote:

Hi David,

On 5/27/20 02:00, David Holmes wrote:

On 27/05/2020 6:36 pm, serguei.spit...@oracle.com wrote:

Hi David,

On 5/27/20 00:47, David Holmes wrote:

Hi Serguei,

On 27/05/2020 1:01 pm, serguei.spit...@oracle.com wrote:

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

CSR draft (one CSR reviewer is needed before finalizing it):
https://bugs.openjdk.java.net/browse/JDK-8245853

I have some thoughts on the wording which I will add to the CSR.

Thank you a lot for looking at this!

Also on reflection I think JVMTI_ERROR_ILLEGAL_ARGUMENT would
the best error to use, and it has an equivalent in JDWP and at
the Java level for JDI.

This is an interesting variant, thanks!
We need to balance on several criteria:
1) Compatibility: keep returning error as close as possible to
the current spec

If you are adding a new error condition I don't understand what
you mean by "close to the current spec" ??

If the

Re: RFR(XS): 8234882: JVM TI StopThread should only allow ThreadDeath

2020-05-29 Thread David Holmes

On 29/05/2020 5:42 pm, serguei.spit...@oracle.com wrote:

Hi David,

Thank you for reviewing this!

On 5/28/20 23:57, David Holmes wrote:

Hi Serguei,

On 28/05/2020 3:12 pm, serguei.spit...@oracle.com wrote:

Hi David,

I've updated the CSR and webrev in place.

I'm reposting the links for convenience.

Enhancement:
https://bugs.openjdk.java.net/browse/JDK-8234882

CSR draft:
https://bugs.openjdk.java.net/browse/JDK-8245853

Spec updates are good - thanks.

Thank you for the CSR review.

Webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.1/src/

src/hotspot/share/prims/jvmtiEnv.cpp

The ThreadDeath check is fine but I'm a bit confused about the
additional null check that leads to JVMTI_ERROR_INVALID_OBJECT. I
can't see how resolve_external_guard can return NULL when not passed
in NULL. Nor why that would result in JVMTI_ERROR_INVALID_OBJECT
rather than JVMTI_ERROR_NULL_POINTER. And I note
JVMTI_ERROR_NULL_POINTER is not even a listed error for StopThread!
This part of the change seems unrelated to this issue.

I was also surprised with the JVMTI_ERROR_NULL_POINTER and
JVMTI_ERROR_INVALID_OBJECT error codes.
The JVM TI spec automatic generation adds these two error codes for a
jobject parameter.

Also, they both are from the Universal Errors section:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.1/docs/specs/jvmti.html#universal-error

Ah I see. Some functions specify JVMTI_ERROR_NULL_POINTER explicitly.

You can find a link to this section at the start of the Error section:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.1/docs/specs/jvmti.html#StopThread

Got it - thanks.

I'm a bit unclear here. If you pass a jweak then resolving it may yield
a NULL if the referent has been GC'd. But IIUC a plain jobject is just a
JNI global reference to an Object - if the jobject is not NULL then
resolving it to an oop can also not yield a NULL.

But regardless this seems unrelated to current issue and just
complicates matters.

Cheers,
David

At least, I was not able to construct a test case to get this error code
returned.
So, I'm puzzled with this. I'll try to find some examples with
JVMTI_ERROR_NULL_POINTER errors.

test/hotspot/jtreg/vmTestbase/nsk/jvmti/StopThread/stopthrd006/TestDescription.java

The copyright year should be change to "2018, 2020,".

Thank you for the catch.
I planned to update the copyright comments.

There are plenty of other nsk.jvmti tests which check valid calls.

Thanks,
Serguei

Thanks,
David
-

Updated JVM TI StopThread spec:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.1/docs/specs/jvmti.html#StopThread

The old webrev and spec are here:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.0/

Thanks,
Serguei

On 5/27/20 18:03, serguei.spit...@oracle.com wrote:

Hi David,

On 5/27/20 02:00, David Holmes wrote:

On 27/05/2020 6:36 pm, serguei.spit...@oracle.com wrote:

Hi David,

On 5/27/20 00:47, David Holmes wrote:

Hi Serguei,

On 27/05/2020 1:01 pm, serguei.spit...@oracle.com wrote:

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

CSR draft (one CSR reviewer is needed before finalizing it):
https://bugs.openjdk.java.net/browse/JDK-8245853

I have some thoughts on the wording which I will add to the CSR.

Thank you a lot for looking at this!

Also on reflection I think JVMTI_ERROR_ILLEGAL_ARGUMENT would the
best error to use, and it has an equivalent in JDWP and at the
Java level for JDI.

This is an interesting variant, thanks!
We need to balance on several criteria:
1) Compatibility: keep returning error as close as possible to
the current spec

If you are adding a new error condition I don't understand what you
mean by "close to the current spec" ??

If the JVMTI_ERROR_INVALID_OBJECT is returned than the JDWP agent
does not need any new error handling.
The same can be true in the JDI if the JDWP returns the same error
as it returned before.
In this case we do not add new error code but extend the existing to
cover new error condition.