RFR(XS): 8234882: JVM TI StopThread should only allow ThreadDeath
David Holmes
david.holmes at oracle.com
Wed Jun 3 23:10:52 UTC 2020
Hi Serguei,
On 4/06/2020 6:41 am, serguei.spitsyn at 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.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