RFR(XS): 8234882: JVM TI StopThread should only allow ThreadDeath
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Thu Jun 4 00:09:36 UTC 2020
On 6/3/20 16:10, David Holmes wrote:
> 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.
No problem, David.
Thanks!
Serguei
>
> 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