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

[David Holmes]

Looks good Dan! Thanks to Ron for all the work on this.

I look forward to the change that gets rid of os::posix definitions from 
OS specific files! (That one really makes me cringe!)

Cheers,
David

On 3/09/2016 4:28 AM, 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)