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

[George Triantafillou]

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
>>>>>>
>>>>
>>
>