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

[George Triantafillou]

Thanks David!

-George

On 6/20/2016 12:50 AM, David Holmes wrote:
> 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
>>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>