RFR(L) 8140520 segfault on solaris-amd64 with "-XX:VMThreadStackSize=1" option

[Daniel D. Daugherty]

On 9/9/16 5:48 AM, Coleen Phillimore wrote:
> http://cr.openjdk.java.net/~dcubed/8140520-webrev/4-jdk9-hs-open/jdk/test/tools/launcher/TooSmallStackSize.java.udiff.html 
>
>
> + *
> + * Note: This repo's version of the test uses the '-Xss' option. The
> + * hotspot repo's version of the test uses the '-XX:ThreadStackSize=n'
> + * VM option.
>
>
> Can you add a sentence, that this is the same thing to make it clear?

I'll figure out a wording change and apply it to both tests.


>
> This test is a smaller version of the one in the runtime repository?   
> Do we need both?

The JDK repo test belongs to the launcher team. It intentionally only
tests the option (-Xss) that the launcher also interprets. Yes, the
-Xss option is interpreted by the launcher for initial thread stack
size and by the VM for subsequent Java thread stack sizes...

The hotspot repo version of the test covers the the three HotSpot
specific -XX options that control thread stack size. So the two tests
are each focused on their respective tech areas and we need them
both.


>
> http://cr.openjdk.java.net/~dcubed/8140520-webrev/4-jdk9-hs-open/hotspot/test/runtime/Thread/TooSmallStackSize.java.html 
>
>
> This looks great.   I think this is a good test:
>
>  177          * Try again with a the minimum stack size that was given 
> in the error message
>
>
> The change looks good.

Thanks for the review!

Dan


> Thanks!
> Coleen
>
>
> On 9/8/16 9:05 PM, Daniel D. Daugherty wrote:
>> > Future Work:
>> >
>> > - Switch TooSmallStackSize.java from local TestHelper.java to the 
>> newer
>> >   test framework
>> >
>> >   - TestHelper.java is a copy of the launcher's TestHelper.java since
>> >     that was the easiest way to bring up the new test. The 
>> dependencies
>> >     in TooSmallStackSize.java on TestHelper.java need to be identified
>> >     and replaced with newer test library equivalents if possible.
>>
>> hotspot/test/runtime/Thread/TooSmallStackSize.java has been rewritten
>> to use the newer test framework and 
>> hotspot/test/runtime/Thread/TestHelper.java
>> is no longer included in the path.
>>
>> Webrev URL: 
>> http://cr.openjdk.java.net/~dcubed/8140520-webrev/4-jdk9-hs-open/
>>
>> Other than timestamps, only 
>> hotspot/test/runtime/Thread/TooSmallStackSize.java
>> is changed in this round.
>>
>> I need just a couple of reviews of the new test.
>>
>> I added some of the test output to this comment in the bug report:
>>
>> https://bugs.openjdk.java.net/browse/JDK-8140520?focusedCommentId=13999090&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13999090 
>>
>>
>> so the new test passes on all platforms.
>>
>> Dan
>>
>>
>>
>> On 9/2/16 12:28 PM, Daniel D. Daugherty wrote:
>>> Greetings,
>>>
>>> I am shepherding the fix for the following bug for Ron Durbin:
>>>
>>>     JDK-8140520 segfault on solaris-amd64 with 
>>> "-XX:VMThreadStackSize=1" option
>>>     https://bugs.openjdk.java.net/browse/JDK-8140520
>>>
>>> Almost all of the product code is Ron's, all of the test code is 
>>> mine and
>>> all of the product code refactoring is Coleen's. I will be handling the
>>> responses to all code review comments and suggestions. Thanks to 
>>> Jerry for
>>> reviewing earlier versions of Ron's work on this bug. Thanks to 
>>> David H.
>>> and Coleen for reviewing versions after I started shepherding.
>>>
>>> The primary focus of this change is to decouple the various thread 
>>> stack
>>> sizes from each other. In particular, decouple CompilerThreadStackSize
>>> from VMThreadStackSize to avoid use of VMThreadStackSize defaults for
>>> CompilerThreadStackSize.
>>>
>>> There are almost no Win* changes here because the Win* thread stack 
>>> size
>>> implementation differs significantly from the *NIX implementation. I 
>>> have
>>> a first pass implementation for the Win* changes, but Win* stack pages
>>> don't behave like I expect so I will split Win* off into another bug.
>>>
>>> The AIX and PPC parts of the fix are "best guesses" because we don't 
>>> have
>>> a way to build and test AIX and/or PPC in Oracle. I'll need help 
>>> from the
>>> usual AIX and PPC folks for checking out those changes.
>>>
>>> Overview of the changes:
>>>
>>> - The CompilerThreadStackSize default has been changed from zero to
>>>   match the default VMThreadStackSize on each platform; see the various
>>>   globals_*.hpp files.
>>>
>>> - There are now three minimum stack sizes: 
>>> java_thread_min_stack_allowed
>>>   compiler_thread_min_stack_allowed, and 
>>> vm_internal_thread_min_stack_allowed.
>>>   In all cases except for Solaris X64, the two new minimums are 
>>> initialized
>>>   to same value as the original min_stack_allowed; see the various
>>>   */os_cpu/*/os_*.cpp files.
>>>
>>>   Key change: On Solaris X64 the new compiler_thread_min_stack_allowed
>>>   value is 394K instead of the old min_stack_allowed value of 224K.
>>>
>>> - os::create_thread()'s stack size parameter has been clarified to be
>>>   the "requested stack size". The stack size adjustment logic has been
>>>   updated to support the three minimum stack sizes.
>>>
>>>   Key change: the VMThreadStackSize default is no longer used as the
>>>   CompilerThreadStackSize default.
>>>
>>> - os::init_2()'s minimum stack size setting and checking logic has
>>>   been updated to support the three minimum stack sizes.
>>>
>>>   Key change: CompilerThreadStackSize and VMThreadStackSize are no 
>>> longer
>>>   silently rounded up when their specified value is too small. This 
>>> matches
>>>   the behavior for both '-Xss<n_bytes>' and 
>>> '-XX:ThreadStackSize=<k_bytes>'.
>>>
>>>   Note: The recent fix for the following bug:
>>>
>>>     8159335: Fix problems with stack overflow handling.
>>>
>>>   updated the existing minimum stack size calculations to ensure 
>>> that they
>>>   are aligned with os::vm_page_size(). I've updated Ron's solution for
>>>   this bug to include the new alignment work with the two new minimum
>>>   stack sizes.
>>>
>>> Bugs fixed:
>>>
>>> - The Solaris X64 crash when "-XX:VMThreadStackSize=1" is specified
>>>   without specifying "-XX:CompilerThreadStackSize=<k_bytes>" is fixed.
>>> - Newly defined ThreadType values are explicitly handled as VM internal
>>>   threads instead of skating through the case statements.
>>> - On platforms that use default_stack_size(), the bug where a default
>>>   value returned for a compiler_thread was ignored is fixed.
>>>
>>> Changes in behavior:
>>>
>>> - Use of -XX:VMThreadStackSize=<k_foo> will no longer affect the value
>>>   of CompilerThreadStackSize when -XX:CompilerThreadStackSize=<bar>
>>>   is not specified.
>>> - When neither -XX:VMThreadStackSize=<foo> or 
>>> -XX:CompilerThreadStackSize=<bar>
>>>   are specified on the command line, then CompilerThreadStackSize 
>>> will use
>>>   a distinct default value than VMThreadStackSize.
>>> - Use of a too small -XX:CompilerThreadStackSize=<bar> value or
>>>   a -XX:VMThreadStackSize=<foo> will generate an error message
>>>   instead of silently rounding up.
>>>
>>> Refactoring changes:
>>>
>>> - os::Posix::get_initial_stack_size() and 
>>> os::Posix::set_minimum_stack_sizes()
>>>   created as a place to land common *NIX code; once we figure out 
>>> how to add
>>>   support for the three thread stack sizes to Win*, this code will 
>>> move again.
>>> - default_stack_size() added to all platforms.
>>> - default_guard_size() removed from platforms that were not using 
>>> it; only
>>>   Linux uses it.
>>>
>>>
>>> Webrev URL: 
>>> http://cr.openjdk.java.net/~dcubed/8140520-webrev/3-jdk9-hs-open/
>>>
>>>
>>> Testing:
>>>
>>> - There is a temporary change in hotspot/test/TEST.groups that allows
>>> runtime/CommandLine/OptionsValidation/TestOptionsWithRanges.java to
>>>   run during JPRT jobs. That test is normally excluded from the
>>>   hotspot_fast_runtime group in JPRT jobs.
>>> - I've copied and modified the TooSmallStackSize.java test from the
>>>   launcher's tests in the jdk repo. The jdk repo version exercises the
>>>   '-Xss<n_bytes>' option. The hotspot repo version exercises the
>>>   '-XX:ThreadStackSize=<k_bytes>', 
>>> '-XX:CompilerThreadStackSize=<k_bytes>',
>>>   and '-XX:VMThreadStackSize=<k_bytes>' options.
>>> - RBT JDK9-hs nightly results were generated and no regressions were
>>>   observed; another round was started after the latest resync, but has
>>>   not yet finished
>>>
>>>
>>> Future Work:
>>>
>>> - Add {compiler,vm_internal}_thread_min_stack_allowed support to 
>>> Windows
>>>
>>>   - Have the beginnings of an implementation, but stack sizes and the
>>>     underlying pages aren't working as I expect.
>>>
>>> - Switch TooSmallStackSize.java from local TestHelper.java to the newer
>>>   test framework
>>>
>>>   - TestHelper.java is a copy of the launcher's TestHelper.java since
>>>     that was the easiest way to bring up the new test. The dependencies
>>>     in TooSmallStackSize.java on TestHelper.java need to be identified
>>>     and replaced with newer test library equivalents if possible.
>>>
>>> - Determine whether the minimum stack values can be reduced on the
>>>   different platforms.
>>>
>>>   - With a single minimum stack size value, it's possible that the
>>>     minimum has been bumped up over the years for one thread type,
>>>     e.g., Compiler threads, when the other thread types didn't need
>>>     the extra space.
>>>   - Will need to do some spelunking through the change histories
>>>     and then some old fashioned value range testing.
>>>
>>> - There is a block of Solaris specific code in
>>>   os::Posix::set_minimum_stack_sizes(); Coleen filed the following
>>>   bug to remove that code:
>>>
>>>   JDK-8161093 Solaris for >8k pagesize adds extra guard pages
>>>   https://bugs.openjdk.java.net/browse/JDK-8161093
>>>
>>> Thanks, in advance, for any comments, suggestions or questions!
>>>
>>> Dan (, Ron and Coleen)
>>>
>>
>