RFR (S) 7179701: MaxJavaStackTraceDepth of zero is not handled correctly/consistently in the VM

[harold seigel]

Your proposed new 'for' loop is better.

The change looks good.

Thanks, Harold


On 1/30/2018 10:44 AM, coleen.phillimore at oracle.com wrote:
>
>
> On 1/30/18 9:44 AM, Daniel D. Daugherty wrote:
>> On 1/30/18 9:39 AM, coleen.phillimore at oracle.com wrote:
>>>
>>>
>>> On 1/29/18 8:02 PM, David Holmes wrote:
>>>> Hi Coleen,
>>>>
>>>> Wow this one is a blast from the past! :)
>>>
>>> Yeah, cleaning up some backlog ...
>>>>
>>>> On 30/01/2018 9:54 AM, coleen.phillimore at oracle.com wrote:
>>>>> Summary: Value of zero means unlimited stack trace.  If you want 
>>>>> no stack trace, use -XX:-StackTraceInThrowable
>>>>>
>>>>> I think I broke this with rework to Throwable fill_in_stack_trace. 
>>>>> Tested with jvmti, jdi and stack stress tests.   Also tested 
>>>>> manually with version of the new test with kill -3 to test the 
>>>>> thread.cpp code.
>>>>>
>>>>> open webrev at http://cr.openjdk.java.net/~coleenp/7179701.01/webrev
>>>>> bug link https://bugs.openjdk.java.net/browse/JDK-7179701
>>>>
>>>> Overall looks fine. One nit ...
>>>>
>>>> src/hotspot/share/classfile/javaClasses.cpp
>>>>
>>>> This looks really awkward for a loop condition:
>>>>
>>>> 1971   for (frame fr = thread->last_frame(); max_depth > 0 ? 
>>>> max_depth != total_count : true;) {
>>>>
>>>> how about changing:
>>>>
>>>> 1936   int max_depth = MaxJavaStackTraceDepth;
>>>>
>>>> to
>>>>
>>>> 1936   int max_depth = MaxJavaStackTraceDepth > 0 ?
>>>>                        MaxJavaStackTracedepth : INT_MAX;
>>>>
>>>> instead? Or, in the style of the other changes:
>>>>
>>>> 1971   for (frame fr = thread->last_frame();
>>>>             max_depth == 0 || max_depth < total_count;) {
>>>
>>> I'll change it to like this, which does look nicer:
>>>   for (frame fr = thread->last_frame(); max_depth == 0 || max_depth 
>>> != total_count;) {
>>>
>>>>
>>>> ?
>>>>
>>>> ---
>>>>
>>>> src/hotspot/share/prims/forte.cpp
>>>>
>>>> I'm wondering what tests actually hit this code?
>>>>
>>>
>>> I couldn't find any, and don't really know how to test this code, 
>>> which is why I ran a lot of serviceability tests.
>>
>> AsyncGetCallTrace() is exercised by the analyzer tool. I'm not
>> aware of any open (or closed) test suites that exercise it.
>
> Thanks, Dan. I  did a search for any tests containing 
> AsyncGetCallTrace() and didn't find any.
>
> Coleen
>>
>> Dan
>>
>>
>>>
>>> Thanks,
>>> Coleen
>>>
>>>> Thanks,
>>>> David
>>>>
>>>>
>>>>> Thanks,
>>>>> Coleen
>>>
>>
>