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.
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.
test/hotspot/jtreg/vmTestbase/nsk/jvmti/StopThread/stopthrd006/TestDescription.java
The copyright year should be change to "2018, 2020,".
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).
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.
But, in fact (especially, after rethinking), I do not like the
JVMTI_ERROR_INVALID_OBJECT
error code as it normally means something different.
So, let's avoid using it and skip this criteria.
Then we need new error code to cover new error condition.
2) Best error naming match between JVM TI and JDI/JDWP
3) Best practice in errors naming
If the argument is not a ThreadDeath instance then it is an illegal
argument - perfect fit semantically all the specs involved have an
"illegal argument" error form.
I agree with this.
It is why I like this suggestion. :)
The JDWP equivalent is: ILLEGAL_ARGUMENT.
The JDI equivalent is: IllegalArgumentException
I'll prepare and send the update.
Thanks!
Serguei
Cheers,
David
I think the #1 is most important but will look at it once more.
Thanks,
Serguei
Thanks,
David
Webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.1/src/
Updated JVM TI StopThread spec:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.1/docs/specs/jvmti.html#StopThread
Summary:
The JVM TI StopThread method mirrored the functionality of the
java.lang.Thread::stop(Throwable t) method, in that it allows
any exception
type to be installed as an asynchronous exception in the target
thread.
However, the java.lang.Thread::stop(Throwable t) method was
inherently unsafe
and in Java 8 (under JDK-7059085) it was "retired" so that it
always threw
UnsupportedOperationException.
The updated JVM TI StopThread spec disallows an arbitrary
Throwable from being passed,
and instead restricts the argument to being an instance of
ThreadDeath, thus
mirroring the (deprecated but still functional)
java.lang.Thread::stop() method.
The error JVMTI_ERROR_INVALID_OBJECT is returned if the
exception argument
is not an instance of ThreadDeath.
Also, I will file similar RFE and CSR on the JDI and JDWP spec.
Testing:
Built docs and checked the doc has been generated as expected.
Will run the nsk.jvmti tests locally.
Will submit hs-tiers1-3 to make sure there are no regressions
in the JVM TI and JDI tests.
Thanks,
Serguei
