RFR 8241053: Hotspot runtime/CommandLine/OptionsValidation/TestOptionsWithRanges.java test fails on Alpine Linux with debug build

[Alexander Scherbatiy]

On 05.08.2020 11:41, Alexander Scherbatiy wrote:

> On 05.08.2020 10:16, Dmitry Samersoff wrote:
>
>> Hello Alexander,
>>
>> Sorry for being later to the party. It might be better to write it as:
>>
>> if (status == EINVAL) {
>>       // pthread_attr_setstacksize() function can fail
>>       // if the stack size exceeds a system-imposed limit.
>>       //
>> ...
>>
>>     return false;
>> }
>>
>> assert_status(status == 0, status, "pthread_attr_setstacksize");
>>
>> I.e. just move "assert" after "if" in your initial fix.
>
>   It looks like the behavior would be different when 
> pthread_attr_setstacksize() returns error other than EINVAL on 
> non-debug build.
>   In the former case there will be the thread cleanup and return from 
> the os::create_thread().
>   In the later case the execution will be continued.
>   I am not sure which one is better.

   I would prefer to leave the current fix because it follows the fail 
fast principle.

   Thanks,
   Alexander.
>
>   Thanks,
>   Alexander.
>
>>
>> But I will not push for it. Fix looks good to me.
>>
>> -Dmitry
>>
>> On 04.08.2020 12:11, Alexander Scherbatiy wrote:
>>> Could you review the updated fix:
>>>    http://cr.openjdk.java.net/~alexsch/8241053/webrev.01
>>>
>>> The assert_status() and the thread cleanup is moved under the "if 
>>> (status != 0)" check.
>>>
>>> Thanks,
>>> Alexander.
>>>
>>> On 04.08.2020 01:51, David Holmes wrote:
>>>> Hi Alexander,
>>>>
>>>> Thanks for fixing this.
>>>>
>>>> On 4/08/2020 1:33 am, Alexander Scherbatiy wrote:
>>>>> Hello,
>>>>>
>>>>> Could you review the fix for the issue:
>>>>>    Bug: https://bugs.openjdk.java.net/browse/JDK-8241053
>>>>>    Webrev: http://cr.openjdk.java.net/~alexsch/8241053/weberv.00
>>>>
>>>> I would simplify this a little:
>>>>
>>>>    assert_status(status == 0 || status == EINVAL, status, 
>>>> "pthread_attr_setstacksize");
>>>>    // pthread_attr_setstacksize() function can fail
>>>>    // if the stack size exceeds a system-imposed limit.
>>>>    if (status == EINVAL) {
>>>>
>>>> to
>>>>    if (status != 0) {
>>>>      // pthread_attr_setstacksize() function can fail
>>>>      // if the stack size exceeds a system-imposed limit.
>>>>      assert_status(status == EINVAL, status, 
>>>> "pthread_attr_setstacksize");
>>>>
>>>> Thanks,
>>>> David
>>>> -----
>>>>
>>>>
>>>>> The following tests fail on Linux Alpine with musl libc:
>>>>> test/hotspot/jtreg/runtime/Thread/TestThreadStackSizes.java
>>>>> test/hotspot/jtreg/runtime/CommandLine/OptionsValidation/TestOptionsWithRanges.java 
>>>>>
>>>>>
>>>>> This is because muls pthread_attr_setstacksize(3) function 
>>>>> implementation [1] returns EINVAL when stack size exceeds the 
>>>>> certain limit.
>>>>>
>>>>> According to POSIX pthread_attr_setstacksize() function 
>>>>> description [2]:
>>>>>   The pthread_attr_setstacksize() function will fail if:
>>>>>      [EINVAL]  The value of stacksize is less than 
>>>>> PTHREAD_STACK_MIN or exceeds a system-imposed limit.
>>>>>
>>>>> The proposed fix excludes EINVAL value from assert_status() and 
>>>>> for the EINVAL return value makes the same cleanup actions and 
>>>>> returns false from os::create_thread() function as it is done for 
>>>>> AIX[3].
>>>>>
>>>>> To reproduce the issue it needs to build Portola from repository 
>>>>> [4] using instructions from JEP 386: Alpine Linux/x64 Port [5].
>>>>> After the fix both TestThreadStackSizes.java and 
>>>>> TestOptionsWithRanges.java tests pass on Alpine Linux with musl 
>>>>> libc and on Ubuntu 18.0.4.
>>>>>
>>>>> [1] 
>>>>> https://git.musl-libc.org/cgit/musl/tree/src/thread/pthread_attr_setstacksize.c 
>>>>>
>>>>> [2] 
>>>>> https://pubs.opengroup.org/onlinepubs/007908799/xsh/pthread_attr_setstacksize.html 
>>>>>
>>>>> [3] 
>>>>> https://hg.openjdk.java.net/jdk/jdk/file/c8102e6fc512/src/hotspot/os/aix/os_aix.cpp#l890 
>>>>>
>>>>> [4] https://github.com/openjdk/portola
>>>>> [5] https://openjdk.java.net/jeps/386
>>>>>
>>>>> Thanks,
>>>>> Alexander.
>>>>>
>>