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

[Daniel D. Daugherty]

On 9/9/16 9:51 AM, Daniel D. Daugherty wrote:
> On 9/8/16 10:43 PM, David Holmes wrote:
>> Hi Dan,
>>
>> On 9/09/2016 11:05 AM, 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.
>>
>> Seems okay. Only comment on the new and existing test is that here:
>>
>> 86         System.out.println("FAILED: Could not get the stack size 
>> from the output");
>> 87         throw new RuntimeException("test fails");
>>
>> we really want to see the output that didn't have what we expected. 
>> Will it shown in the log anyway?
>
> I'll induce a failure in both tests and see what we get.

Forgot to say: Thanks for the review!

Dan


>
> Dan
>
>
>>
>> Thanks,
>> David
>>
>>
>>> 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)
>>>>
>>>
>