RFR 8157592: StackTraceLogging fails with stack overflow on 32-bit Windows

[David Holmes]

Thanks George this looks fine to me.

To clarify, we test 1023, 1024 and 1025 to watch for one-off errors 
related to the value of MaxJavaStackTraceDepth. At the same time 1025 
tests the case of exceeding MaxJavaStackTraceDepth without requiring 
potentially excessive additional stack.

Cheers,
David

On 18/06/2016 2:18 AM, George Triantafillou wrote:
> Hi Coleen,
>
> Thanks for your comments.
>
> New webrev:
> http://cr.openjdk.java.net/~gtriantafill/8157592/webrev.01/index.html
> <http://cr.openjdk.java.net/%7Egtriantafill/8157592/webrev.01/index.html>
>
> -George
>
> On 6/17/2016 11:48 AM, Coleen Phillimore wrote:
>>
>> For this
>>
>> http://cr.openjdk.java.net/~gtriantafill/8157592/webrev/test/runtime/Throwable/StackTraceLogging.java.udiff.html
>>
>>
>> Can you change the comment:
>>
>>   44         // These depths match the ones in TestThrowable.java
>>
>>
>> to
>>
>>   44         // These depths match the ones in TestThrowable.java,
>> except the one greater than 1024
>>
>>
>> And add 1023 to the list in 45?
>>
>> thanks,
>> Coleen
>>
>>
>> On 6/17/16 11:36 AM, George Triantafillou wrote:
>>> New webrev: http://cr.openjdk.java.net/~gtriantafill/8157592/webrev/
>>> <http://cr.openjdk.java.net/%7Egtriantafill/8157592/webrev/>
>>>
>>> -George
>>>
>>> On 6/17/2016 10:43 AM, George Triantafillou wrote:
>>>> Hi David,
>>>>
>>>> Thanks for your comments.  Based on your comments and Coleen's
>>>> comments on the purpose of the test, I've made the following changes:
>>>>
>>>> TestThrowable.java:
>>>> - removed the explicit check for StackOverflowError
>>>> - changed the maximum depth check from 2042 to 1025
>>>>
>>>> StackTraceLogging.java:
>>>> - removed unused updateEnvironment method
>>>>
>>>> Tested locally with 32-bit Windows and Linux.
>>>>
>>>> -George
>>>>
>>>> On 6/16/2016 5:11 PM, Coleen Phillimore wrote:
>>>>>
>>>>>
>>>>> On 6/16/16 5:02 PM, David Holmes wrote:
>>>>>> Hi George,
>>>>>>
>>>>>> On 17/06/2016 5:22 AM, George Triantafillou wrote:
>>>>>>> Hi David,
>>>>>>>
>>>>>>> I did an experiment by printing the stack depth at smaller
>>>>>>> intervals and
>>>>>>> running TestThrowable.java with
>>>>>>> -XX:AbortVMOnException=java.lang.StackOverflowError.  In one failing
>>>>>>> run, there were 1888 recursions.  Here's the stack of the failing
>>>>>>> thread
>>>>>>> from the hs_err file:
>>>>>>>
>>>>>>> Stack: [0x15980000,0x159d0000],  sp=0x1598b9bc,  free space=46k
>>>>>>>
>>>>>>> If you divide by the stack size on windows 32, that amounts to
>>>>>>> approximately 173 bytes per frame, which doesn’t seem unreasonable.
>>>>>>>
>>>>>>> (gdb) print (0x159d0000-0x15980000)/1888
>>>>>>> $2 = 173
>>>>>>>
>>>>>>> Factor in that the frames are compiled, which use less space than
>>>>>>> interpreted and whether the frames are compiled in time for the
>>>>>>> test to
>>>>>>> run can also add variability.  C2 optimizes stack usage more than
>>>>>>> C1,
>>>>>>> and the test case appears to compile these methods with C1. Also,
>>>>>>> when
>>>>>>> run with jtreg, there are frames on the stack for the jtreg test
>>>>>>> framework.
>>>>>>>
>>>>>>> The stack usage for this test isn’t unreasonable. There’s no
>>>>>>> change to
>>>>>>> stack usage that would have made this test fail now. It’s just
>>>>>>> normal
>>>>>>> variability on 32-bit Windows:
>>>>>>>
>>>>>>> java -XX:+PrintFlagsFinal -version | grep Stack | grep Pages
>>>>>>>
>>>>>>> intx StackRedPages                             = 1
>>>>>>> intx StackShadowPages                          = 9
>>>>>>> intx StackYellowPages                          = 3
>>>>>>>
>>>>>>> 13*4k = 52k which results in stack overflow.
>>>>>>
>>>>>> Thanks for doing the experiment. Do we have numbers for other
>>>>>> 32-bit platforms to compare against?
>>>>>>
>>>>>> I'd still rather see the test keep well away from triggering SOE
>>>>>> than to just catch it and ignore it. That way the test continues
>>>>>> to act as a means to detect excessive stack use changes.
>>>>>
>>>>> The purpose of the test is not to generate stack overflow exception
>>>>> or check the stack.   It's supposed to check that logging the stack
>>>>> depth is correct.   Since I used recursion when writing this test,
>>>>> adding a catch for SOE for cases where it occurs will make it
>>>>> robust against other changes in the VM. We have other stack
>>>>> overflow tests in the system. This is not one of them!
>>>>>
>>>>> I think George's changes are fine and fix the test, for which I'm
>>>>> grateful.
>>>>>
>>>>> Coleen
>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> David
>>>>>> -----
>>>>>>
>>>>>>> -George
>>>>>>>
>>>>>>> On 6/9/2016 9:08 PM, David Holmes wrote:
>>>>>>>> Hi George,
>>>>>>>>
>>>>>>>> On 10/06/2016 4:09 AM, George Triantafillou wrote:
>>>>>>>>> Please review this small change to fix test failures on 32-bit
>>>>>>>>> Windows.
>>>>>>>>>
>>>>>>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8157592
>>>>>>>>> Open webrev:
>>>>>>>>> http://cr.openjdk.java.net/~gtriantafill/8157592/webrev/
>>>>>>>>> <http://cr.openjdk.java.net/%7Egtriantafill/8157592/webrev/>
>>>>>>>>>
>>>>>>>>> TestThrowable.java: Added catch for intermittent
>>>>>>>>> StackOverflowError
>>>>>>>>> errors on 32-bit Windows.
>>>>>>>>
>>>>>>>> This does not seem to be addressing the problem - why do we
>>>>>>>> suddenly
>>>>>>>> start getting SOE when running this test? I asked that question
>>>>>>>> in the
>>>>>>>> bug report. If something has changed to trigger this then we
>>>>>>>> need to
>>>>>>>> ensure that change was intended/desirable and then look at how to
>>>>>>>> adjust the test - not simply catch the SOE and ignore it.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> David
>>>>>>>> -----
>>>>>>>>
>>>>>>>>
>>>>>>>>> StackTraceLogging.java: Removed unused updateEnvironment method.
>>>>>>>>>
>>>>>>>>> Tested locally on 32-bit Windows as well as RBT.
>>>>>>>>>
>>>>>>>>> -George
>>>>>>>>>
>>>>>>>
>>>>>
>>>>
>>>
>>
>