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

[George Triantafillou]

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