RFR (S) 8205608: Fix 'frames()' in ThreadReferenceImpl.c to prevent quadratic runtime behavior

[serguei.spitsyn at oracle.com]

On 7/20/18 13:44, Chris Plummer wrote:
> On 7/20/18 1:40 PM, serguei.spitsyn at oracle.com wrote:
>> Hi Ralf,
>>
>>
>> On 7/20/18 07:28, Schmelter, Ralf wrote:
>>> Hi Sergue,
>>>
>>> I’ve updated the webref: 
>>> http://cr.openjdk.java.net/~simonis/webrevs/2018/8205608.v4/
>>
>> The copyright year in ThreadReferenceImpl.c still has to be 2018, not 
>> 2008.
>>
>> http://cr.openjdk.java.net/~simonis/webrevs/2018/8205608.v4/test/jdk/com/sun/jdi/Frames2Test.java.html 
>>
>>
>>   72             if (newDepth == -1_000) {
>>   73                 // Pop some frames so there is room on the stack 
>> for the
>>   74                 // call (including println()).
>>   75                 notifyRecursionEnded();
>>   76             }
>>
>>   I have a concern on potential issue mentioned in the comment above.
>>   Should a StackOverflowError be expected here?
>>
>>   79         } catch (StackOverflowError e) {
>>   80             // Use negative depth to indicate the recursion has 
>> ended.
>>   81             return -1;
>>   82         }
>>
>>   What is going to happen if the StackOverflowError was really caught 
>> above?
> The SOE is really caught in the above code. I returns -1, and starts 
> the unwinding of the stack. After 1000 frames have been popped via 
> returns, notifyRecursionEnded() will be called. The pops are so 
> notifyRecursionEnded() can be called without worry of another SOE.

Got it, thanks Chris.

So, I'm Okay with the fix assuming the copyright year is fixed.

Thanks,
Serguei

>
> Chris
>>   If I understand it correctly, the notifyRecursionEnded() call will 
>> be missed then.
>>   This breakpoint will be missed as well:
>>
>>   107         bpe = resumeTo("Frames2Targ", "notifyRecursionEnded", 
>> "()V");
>>
>>
>>
>>> JVMTI_ERROR_OPAQUE_FRAME was never returned from GetFrameLocation(). 
>>> If it would have, the old code would have removed all native methods 
>>> from the call stack. The original JVMDI call did indeed return 
>>> JVMDI_ERROR_OPAQUE_FRAME, so maybe it was a leftover from the 
>>> JVMDI->JVMTI transition.
>>
>> Agreed.
>>
>>> I’ve tried to make the test more readable and added some comments to 
>>> explain why it is done the way it is.
>>
>> Thank you for the update!
>>
>>
>> Thanks,
>> Serguei
>>
>>> Best regards,
>>> Ralf
>>>
>>>
>>>
>>>
>>> From: serguei.spitsyn at oracle.com [mailto:serguei.spitsyn at oracle.com]
>>> Sent: Mittwoch, 18. Juli 2018 22:57
>>> To: Chris Plummer <chris.plummer at oracle.com>; Schmelter, Ralf 
>>> <ralf.schmelter at sap.com>; serviceability-dev at openjdk.java.net; 
>>> Stuefe, Thomas <thomas.stuefe at sap.com>
>>> Subject: Re: RFR (S) 8205608: Fix 'frames()' in 
>>> ThreadReferenceImpl.c to prevent quadratic runtime behavior
>>>
>>> Hi Ralf,
>>>
>>> The fix itself looks pretty good to me.
>>> Some minor comments.
>>>
>>> The copyright year needs an update.
>>>   218     jint count, filledIn;
>>>
>>>   Could you, please, split the declarations above into different 
>>> lines to follow the local style?
>>> Ii is interesting that the original implementation checked the error 
>>> code returned
>>> from the JVMTI GetFrameLocation for being equal to 
>>> JVMTI_ERROR_OPAQUE_FRAME.
>>> However, the GetFrameLocation spec does not list this error code as 
>>> possible.
>>>
>>>
>>> Some comments about the test.
>>>    52     static void callEnded() {
>>>    53         System.out.println("SOE occurred as expected");
>>>    54     }
>>>    55
>>>    56     static int call(int depth) {
>>>    57         if (depth == 0) {
>>>    58             // Should have seen a stack overflow by now.
>>>    59             System.out.println("Exited without creating SOE");
>>>    60             System.exit(0);
>>>    61         }
>>>    62
>>>    63         try {
>>>    64             int newDepth = call(depth - 1);
>>>    65
>>>    66             if (newDepth == -1_000) {
>>>    67                 // Pop some frames so there is room on the 
>>> stack for the
>>>    68                 // println()
>>>    69                 callEnded();
>>>    70             }
>>>    71
>>>    72             return newDepth - 1;
>>>    73         } catch (StackOverflowError e) {
>>>    74             return -1;
>>>    75         }
>>>    76     }
>>>    77 }
>>>    I'd suggest to rename the methods call() and callEnded() to 
>>> something like
>>>    recursiveMethod() and recursionEnd().
>>>    Also, the manipulations with SOE create a complexity and are 
>>> confusing.
>>>    Could it be more simple to let it propagated and then catch in 
>>> main()?
>>>    What is the point for all these checks at the lines 104-119?
>>>    In general, I'm looking for some ways to make it more clear, 
>>> simple and stable.
>>>
>>> Thanks,
>>> Serguei
>>
>