RFR JDK-8215425: vmTestbase/nsk/jvmti/PopFrame should provide more detailed output

[serguei.spitsyn at oracle.com]

Alex,

Thank you for the update!

It looks good.
There is another instance with incorrect spacing:

121 totRes=doPopFrame(false, Thread.currentThread());


No need in new webrev if you fix the above.

Thanks,
Serguei


On 12/18/18 6:01 PM, Alex Menkov wrote:
> Hi Serguei,
>
> Thank you for the review.
> Updated webrev:
> http://cr.openjdk.java.net/~amenkov/popframe_cleanup/webrev.02/
>
> On 12/18/2018 16:49, serguei.spitsyn at oracle.com wrote:
>> Hi Alex,
>>
>> A couple of minor comments.
>>
>> http://cr.openjdk.java.net/~amenkov/popframe_cleanup/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/jvmti/PopFrame/popframe002.java.frames.html 
>>
>> http://cr.openjdk.java.net/~amenkov/popframe_cleanup/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/jvmti/PopFrame/popframe004.java.frames.html 
>>
>>
>>    While you are at these files, could you, fix several originally 
>> ugly indented comments?
>
> Done.
>
>>
>>    Could you, also, fix the incorrect spacing around '=' in the 
>> popframe004.java:
>>
>> 95 retValue=doPopFrame(true, popFrameClsThr);
>
> Done.
>
> Also fixed comment in popframe004/TestDescription.java (to be in sync 
> with comment change in popframe004.java)
>
>>
>>
>>    Could you give an idea about the motivation to remove the 
>> following fragment ?:
>
> This is "test case 2" in popframe004 which I mentioned in the review 
> request.
> The code is never executed (if it is, this means the test has already 
> failed) and I don't have an idea what other case can be tested here.
>
> --alex
>
>>
>> 167 if (popframe004.popFdone) { // popping has been done
>> 168 if (DEBUG_MODE)
>> 169 out.println("popFrameCls (" + this +
>> 170 "): enter activeMethod() after popping");
>> 171 // test case #2: popping from the current thread
>> 172 if (!popframe004.popF2done) {
>> 173 popframe004.popF2done = true;
>> 174 if (DEBUG_MODE) {
>> 175 out.println("popFrameCls (" + this +
>> 176 "): going to pop a frame from the current thread...");
>> 177 retVal = doPopFrame(3, popFrameClsThr);
>> 178 } else
>> 179 retVal = doPopFrame(2, popFrameClsThr);
>> 180 if (retVal != PASSED)
>> 181 popframe004.totRes = FAILED;
>> 182 }
>> 183 if (DEBUG_MODE)
>> 184 out.println("popFrameCls (" + this +
>> 185 "): leaving activeMethod()...");
>> 186 return;
>> 187 }
>>
>>
>> Otherwise, it looks good.
>>
>> Thanks,
>> Serguei
>>
>>
>> On 12/18/18 1:37 PM, Alex Menkov wrote:
>>> Hi all,
>>>
>>> please review the fix for
>>> https://bugs.openjdk.java.net/browse/JDK-8215425
>>> webrev:
>>> http://cr.openjdk.java.net/~amenkov/popframe_cleanup/webrev.01/
>>>
>>> The fix
>>> - turns on detailed output for the tests and cleaned related stuff;
>>> - for popframe002 deletes output from run() method as it caused 
>>> unexpected MethodExit event;
>>> - removes "test case 2" in popframe004 test as it's never executed.
>>>
>>> --alex
>>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20181218/f683c8ba/attachment.html>