RFR(XS): 8234882: JVM TI StopThread should only allow ThreadDeath
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Wed Jun 3 20:41:32 UTC 2020
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.spitsyn at 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.spitsyn at 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.spitsyn at 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.spitsyn at oracle.com wrote:
>>>>>>> On 5/29/20 00:56, serguei.spitsyn at oracle.com wrote:
>>>>>>>> On 5/29/20 00:42, serguei.spitsyn at 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.spitsyn at 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.spitsyn at oracle.com wrote:
>>>>>>>>>>>> Hi David,
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> On 5/27/20 02:00, David Holmes wrote:
>>>>>>>>>>>>> On 27/05/2020 6:36 pm, serguei.spitsyn at oracle.com wrote:
>>>>>>>>>>>>>> Hi David,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On 5/27/20 00:47, David Holmes wrote:
>>>>>>>>>>>>>>> Hi Serguei,
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On 27/05/2020 1:01 pm, serguei.spitsyn at 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
>>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>
>>>
>
More information about the serviceability-dev
mailing list