RFR(XS): 8234882: JVM TI StopThread should only allow ThreadDeath

serguei.spitsyn at oracle.com serguei.spitsyn at oracle.com
Fri May 29 08:24:13 UTC 2020 

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.

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