[9] RFR (S) 6762191: Setting stack size to 16K causes segmentation fault

[Kumar Srinivasan]

On 12/3/2014 11:26 AM, Chris Plummer wrote:
> Hi Kumar,
>
> On 12/3/14 10:58 AM, Kumar Srinivasan wrote:
>> Hi Chris,
>>
>> Approved with some minor nits, typos which needs correction.
>>
>> yes java.c follows the JDK indenting as Alan pointed out.
>>
>> TooSmallStackSize.java
>>
>> Copyright should be 2014,
> Copy/paste error from example test I was referred to. I will fix, and 
> also remove the import if not needed.
>>
>> 1.
>>
>>   37  * stack size for the platform (as provided by the JVM error 
>> message when a very
>>   38  * small is used), and then verify that the JVM can be launched 
>> with that stack
>>
>> s/small is/small stack is/
> ok
>>
>> 2.
>>
>>  54      * Returns the minimum stack size this platform will allowed 
>> based on the
>>
>> s/allowed/allow/
> ok
>>
>> 3.
>>
>> 58      * The TestResult argument must containthe result of having 
>> already run
>> s/containthe/contain the/
> ok
>>
>> 4.
>> 92         if (verbose) System.out.println("*** Testing " + stackSize);
>>
>> I know this is allowed in the HotSpot world but in JDK land we always
>> use with a LF + Indent, also in other places.
> Are curly braces needed then? I know some coding conventions require 
> them.

No not necessary for one liners.

>>
>> 5.
>> 85      * Returns the mimumum allowed stack size gleaned from the 
>> error message,
>> s/mimumum/minimum
> ok.
>>
>>
>> Finally I am concerned with the integration, since it spans both
>> HotSpot and JDK, and it appears the test will fail if the HotSpot
>> changes are not integrated first, or has it already ?
> There are no hotspot changes. java.c is where the fix is.

Great!.

Thanks
Kumar

>
> thanks,
>
> Chris
>>
>> Thanks
>> Kumar
>>
>>
>>
>>
>>
>>
>>
>> On 12/1/2014 6:39 PM, Chris Plummer wrote:
>>> Sorry about the long delay in getting back to this. I ran into two 
>>> separate JPRT issues that were preventing me from testing these 
>>> changes, plus I was on vacation last week. Here's an updated webrev. 
>>> I'm not sure where we left things, so I'll just say what's changed 
>>> since the original version:
>>>
>>> 1. Rewrote the test to be in Java instead of a shell script.
>>> 2. Moved the test from hotspot/test/runtime/memory to 
>>> jdk/test/tools/launcher
>>> 3. Added STACK_SIZE_MINIMUM to java.c, allowing a makefile to 
>>> override the default 32k minimum value.
>>>
>>> https://bugs.openjdk.java.net/browse/JDK-6762191
>>> http://cr.openjdk.java.net/~cjplummer/6762191/webrev.02/
>>>
>>> thanks,
>>>
>>> Chris
>>>
>>> On 11/19/14 7:52 AM, Chris Plummer wrote:
>>>> On 11/19/14 2:12 AM, David Holmes wrote:
>>>>> On 19/11/2014 6:49 PM, Chris Plummer wrote:
>>>>>> I've update the webrev to add STACK_SIZE_MINIMUM in place of the 32k
>>>>>> references, and also moved the test from hotspot/test/runtime to
>>>>>> jdk/test/tools/launcher as David requested. That required some
>>>>>> adjustments to the test script, since test_env.sh does not exist in
>>>>>> jdk/test, so I had to pull in the bits I needed into the script.
>>>>>
>>>>> Is there a reason this needs a shell script instead of using the 
>>>>> testlibrary tools to launch the VM and check the output?
>>>> Not that I'm aware of. I guess I just really didn't look at what it 
>>>> would take to make it all in java. I'll have a look at java 
>>>> examples and convert it.
>>>>
>>>> Chris
>>>>>
>>>>> Sorry that should have been mentioned much earlier.
>>>>>
>>>>> David
>>>>>
>>>>>
>>>>>> http://cr.openjdk.java.net/~cjplummer/6762191/webrev.01/
>>>>>>
>>>>>> I still need to rerun through JPRT. I'll do so once there are no 
>>>>>> more
>>>>>> suggested changes.
>>>>>>
>>>>>> thanks,
>>>>>>
>>>>>> Chris
>>>>>>
>>>>>> On 11/18/14 2:08 PM, Chris Plummer wrote:
>>>>>>> Adding core-libs-dev at openjdk.java.net, since one of the changes 
>>>>>>> is in
>>>>>>> java.c.
>>>>>>>
>>>>>>> Chris
>>>>>>>
>>>>>>> On 11/12/14 6:43 PM, David Holmes wrote:
>>>>>>>> Hi Chris,
>>>>>>>>
>>>>>>>> Sorry for the delay.
>>>>>>>>
>>>>>>>> On 13/11/2014 5:44 AM, Chris Plummer wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> I'm still looking for reviewers.
>>>>>>>>
>>>>>>>> As the change is to the launcher it needs to be reviewed by the
>>>>>>>> launcher owner - which I think is serviceability (though also cc'd
>>>>>>>> Kumar :) ).
>>>>>>>>
>>>>>>>> Launcher change, and your rationale, seems okay to me. I'd 
>>>>>>>> probably
>>>>>>>> put the test in to jdk/test/tools/launcher/ though.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> David
>>>>>>>>
>>>>>>>>> thanks,
>>>>>>>>>
>>>>>>>>> Chris
>>>>>>>>>
>>>>>>>>> On 11/7/14 7:53 PM, Chris Plummer wrote:
>>>>>>>>>> This is an initial review for 6762191. I'm guessing there 
>>>>>>>>>> will be
>>>>>>>>>> recommendations to fix in a different way, but thought this 
>>>>>>>>>> would be a
>>>>>>>>>> good time to start the discussion.
>>>>>>>>>>
>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-6762191
>>>>>>>>>> http://cr.openjdk.java.net/~cjplummer/6762191/webrev.00.jdk/
>>>>>>>>>> http://cr.openjdk.java.net/~cjplummer/6762191/webrev.00.hotspot/
>>>>>>>>>>
>>>>>>>>>> The bug is that if the -Xss size is set to something very 
>>>>>>>>>> small (like
>>>>>>>>>> 16k), on linux there will be a crash due to overwriting the 
>>>>>>>>>> end of the
>>>>>>>>>> stack. This happens before hotspot can compute its stack 
>>>>>>>>>> needs and
>>>>>>>>>> verify that the stack is big enough.
>>>>>>>>>>
>>>>>>>>>> It didn't seem viable to move the hotspot stack size check 
>>>>>>>>>> earlier. It
>>>>>>>>>> depends on too much other work done before that point, and 
>>>>>>>>>> the changes
>>>>>>>>>> would have been disruptive. The stack size check is currently 
>>>>>>>>>> done in
>>>>>>>>>> os::init_2().
>>>>>>>>>>
>>>>>>>>>> What is needed is a check before the thread is created. That 
>>>>>>>>>> way we
>>>>>>>>>> can create a thread with a big enough stack to handle all 
>>>>>>>>>> needs up to
>>>>>>>>>> the point of the check in os::init_2(). This initial check 
>>>>>>>>>> does not
>>>>>>>>>> need to be the final check. It just needs to confirm that we 
>>>>>>>>>> have
>>>>>>>>>> enough stack to get us to the check in os::init_2().
>>>>>>>>>>
>>>>>>>>>> I decided to check in java.c if the -Xss size is too small, 
>>>>>>>>>> and set it
>>>>>>>>>> to a larger size if it is. I hard coded this size to 32k 
>>>>>>>>>> (I'll explain
>>>>>>>>>> why 32k later). I suspect this is the part that will result 
>>>>>>>>>> in some
>>>>>>>>>> debate. If you have better suggestions let me know. If it 
>>>>>>>>>> does stay
>>>>>>>>>> here, then probably the 32k needs to be a #define, and maybe 
>>>>>>>>>> even an
>>>>>>>>>> OS porting interface, but I'm not sure where to put it.
>>>>>>>>>>
>>>>>>>>>> The reason I chose 32k is because this is big enough for all 
>>>>>>>>>> platforms
>>>>>>>>>> to get to the stack size check in os::init_2(). It is also 
>>>>>>>>>> smaller
>>>>>>>>>> than the actual minimum stack size allowed on any platform. 
>>>>>>>>>> 32-bit
>>>>>>>>>> windows has the smallest requirement at 64k. I add some 
>>>>>>>>>> printfs to
>>>>>>>>>> print the minimum stack requirement, and then ran a simple 
>>>>>>>>>> JTReg test
>>>>>>>>>> with every JPRT supported platform to get the results.
>>>>>>>>>>
>>>>>>>>>> The TooSmallStackSize.sh will run "java -version" with -Xss16k,
>>>>>>>>>> -Xss32k, and -XXss<minsize>, where <minsize> is the size from 
>>>>>>>>>> the
>>>>>>>>>> error message produced by the JVM, such as in the following:
>>>>>>>>>>
>>>>>>>>>> $ java -Xss32k -version
>>>>>>>>>> The stack size specified is too small, Specify at least 100k
>>>>>>>>>> Error: Could not create the Java Virtual Machine.
>>>>>>>>>> Error: A fatal exception has occurred. Program will exit.
>>>>>>>>>>
>>>>>>>>>> I ran this test through JPRT on all platforms, and they all 
>>>>>>>>>> pass.
>>>>>>>>>>
>>>>>>>>>> One thing to point out is that Windows behaves a bit 
>>>>>>>>>> different than
>>>>>>>>>> the other platforms. It always rounds the stack size up to a 
>>>>>>>>>> multiple
>>>>>>>>>> of 64k , so even if you specify -Xss16k, you get a 64k stack. On
>>>>>>>>>> 32-bit Windows with C1, 64k is also the minimum requirement, 
>>>>>>>>>> so there
>>>>>>>>>> is no error produced in this case. However, on 32-bit Windows 
>>>>>>>>>> with C2,
>>>>>>>>>> 68k is the minimum, so an error is produced since the stack 
>>>>>>>>>> will only
>>>>>>>>>> be 64k. There is no bug here. It's just a bit confusing.
>>>>>>>>>>
>>>>>>>>>> thanks,
>>>>>>>>>>
>>>>>>>>>> Chris
>>>>>>>>>
>>>>>>>
>>>>>>
>>>>
>>>
>>
>