RFR (M): 8231595 [TEST] develop a test case for SuspendThreadList including current thread

[serguei.spitsyn at oracle.com]

Hi Alex,

On 10/1/19 12:23 PM, Alex Menkov wrote:
> Hi Serguei,
>
> Looks good.
> The only note:
>
> (libSuspendWithCurrentThread.cpp)
>
> 115     check_jvmti_status(jni, err, "resumeTestedThreads: error in 
> ResumeThreadList");
>
> This check in a cycle looks useless.

Nice catch, thanks!
The check_jvmti_status in the loop has to check resutls[i] instead of err.

Thanks,
Serguei

> No need for new webrev.
>
> --alex
>
>
> On 09/30/2019 22:45, serguei.spitsyn at oracle.com wrote:
>> Hi Chris, David and Alex,
>>
>> The updated webrev is:
>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2019/8231595-jvmti-susp-tlist.2/ 
>>
>>
>>
>> It includes the changes:
>>    - added a general comment explaining the test logic suggested by 
>> David
>>    - setAllThreadsReady() is static now, the obsolete comment before 
>> it is deleted
>>    - now the test is run twice: with the suspender thread first and 
>> last in the list
>>    - removed the local variable "success" in the run() method
>>    - several native agent methods return "void" now
>>
>> Thanks,
>> Serguei
>>
>>
>> On 9/30/19 13:49, Chris Plummer wrote:
>>> On 9/30/19 1:30 PM, serguei.spitsyn at oracle.com wrote:
>>>> On 9/30/19 13:25, Chris Plummer wrote:
>>>>> On 9/30/19 1:21 PM, serguei.spitsyn at oracle.com wrote:
>>>>>> Hi Chris,
>>>>>>
>>>>>> Thank you for reviewing this!
>>>>>>
>>>>>>
>>>>>> On 9/28/19 12:33, Chris Plummer wrote:
>>>>>>> Hi Serguei,
>>>>>>>
>>>>>>> Overall looks good. A few questions:
>>>>>>>
>>>>>>> I don't understand the need for all the 'i' and 'n' theatrics in 
>>>>>>> the shouldFinish loop. Can you explain and also add a comment?
>>>>>>
>>>>>> I used this part from one of the old SuspendThreadList nsk tests 
>>>>>> in the vmTestbase.
>>>>>> As I understand it, the point was to make sure the JVMTI 
>>>>>> SuspendThreadList works well
>>>>>> wen the top frames executed on tested threads have been compiled.
>>>>>> These code is needed to make the run() method hot.
>>>>>> I can add a comment if you think it is not clear.
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> Is this comment right?
>>>>>>>
>>>>>>>  193     // set thread is not ready again
>>>>>>>  194     public void setAllThreadsReady() {
>>>>>>>  195         allThreadsReady = true;
>>>>>>>  196     }
>>>>>>
>>>>>> Nice catch.
>>>>>> The comment is not needed anymore.
>>>>>> Is a leftover from previous test version.
>>>>>>
>>>>>>>
>>>>>>> Also, shouldn't "setAllThreadsReady()" be static?
>>>>>>
>>>>>> Right. It has to be static. Will fix it.
>>>>>>
>>>>>>
>>>>>>> Do you think it would be useful to also run the test with the 
>>>>>>> last thread in the list being the suspender thread?
>>>>>>
>>>>>> I'm not sure it is worth to do it.
>>>>>> It'd add more complexity into the test.
>>>>>> We could try to make the suspender thread to be random though.
>>>>> I don't think random is good. Makes it hard to reproduce an issue 
>>>>> if it turns up. I was thinking just rerun the test for each 
>>>>> possible suspender.
>>>>
>>>> Good idea to rerun the test and pass the suspender thread index in 
>>>> the arguments.
>>>> Do you think, two runs would be good enough or we also need an 
>>>> index somewhere in the middle?
>>> Maybe just first and last indices would be good. I'm not sure 
>>> something in the middle helps any.
>>>
>>> Chris
>>>>
>>>> Thanks,
>>>> Serguei
>>>>
>>>>>
>>>>> thanks,
>>>>>
>>>>> Chris
>>>>>>
>>>>>> Thanks,
>>>>>> Serguei
>>>>>>
>>>>>>>
>>>>>>> thanks,
>>>>>>>
>>>>>>> Chris
>>>>>>>
>>>>>>> On 9/27/19 6:25 PM, serguei.spitsyn at oracle.com wrote:
>>>>>>>> Please, review fix for test enhancement:
>>>>>>>>   https://bugs.openjdk.java.net/browse/JDK-8231595
>>>>>>>>
>>>>>>>> Webrev:
>>>>>>>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2019/8231595-jvmti-susp-tlist.1/ 
>>>>>>>>
>>>>>>>>
>>>>>>>> Summary:
>>>>>>>>   New test is a coverage for the JVMTI bug:
>>>>>>>>     https://bugs.openjdk.java.net/browse/JDK-8217762
>>>>>>>>       SuspendThreadList won't work correctly if the current 
>>>>>>>> thread is not last in the list
>>>>>>>>
>>>>>>>>   It provides a prove the bug JDK-8217762 does not exist
>>>>>>>>   as the test is passed with the current implementation.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Serguei
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>>
>>