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

Fri Sep 9 18:09:27 UTC 2016

Dan, thank you for the updated comment.   It looks good!
Coleen

On 9/9/16 2:02 PM, Daniel D. Daugherty wrote:
> Greetings,
>
> I've updated both tests to address comments from David H and Coleen.
>
> Changes made in this (hopefully final) round:
>
> hotspot/test/runtime/Thread/TooSmallStackSize.java
>
> - updated options comment (Coleen)
> - added more diagnostics for RuntimeException failure mode (David)
>
> hotspot/test/TEST.groups
>
> - backed out the temporary change for testing during JPRT runs
> - this file is no longer changed by this bug fix
>
> jdk/test/tools/launcher/TooSmallStackSize.java
>
> - updated options comment (Coleen)
> - added more diagnostics for RuntimeException failure modes
>   in three places (David)
>
> The diagnostics were tested by temporarily inducing failures
> at each point and verifying that the extra output was produced.
>
>
> Webrev URL: 
> http://cr.openjdk.java.net/~dcubed/8140520-webrev/5-jdk9-hs-open/
>
> Dan
>
>
>
>
> On 9/9/16 10:00 AM, Daniel D. Daugherty wrote:
>> 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)
>>>>>
>>>>
>>>
>>
>>
>