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 broader issue.
There are many such checks in JVM TI.
For instance, there are checks like the following in jvmtiEnv.cpp:
NULL_CHECK(o, JVMTI_ERROR_INVALID_OBJECT)
Yes but they are incorrect IMO e.g.
JvmtiEnv::GetObjectSize(jobject object, jlong* size_ptr) {
oop mirror = JNIHandles::resolve_external_guard(object);
NULL_CHECK(mirror, JVMTI_ERROR_INVALID_OBJECT);
The NULL_CHECK will fail if either object is NULL or object is a jweak
that has been cleared. In the first case it should report
JVMTI_ERROR_NULL_POINTER.
The correct pattern is what you proposed with this fix:
+ NULL_CHECK(exception, JVMTI_ERROR_NULL_POINTER);
oop e = JNIHandles::resolve_external_guard(exception);
+ // the exception must be a valid jobject
+ if (e == NULL) {
+ return JVMTI_ERROR_INVALID_OBJECT;
+ }
I see your point, thanks!
I'll check these cases and file a bug if necessary.
Though not sure why you didn't use a second NULL_CHECK
I've already replaced it with:
NULL_CHECK(e, JVMTI_ERROR_INVALID_OBJECT);
You, probably, need to refresh the webrev page.
Thanks,
Serguei
David
-----
Thanks,
Serguei
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.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