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

Wed Dec 3 19:26:41 UTC 2014

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

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