RFR: 8178491: -Xss and -XX:ThreadStackSize argument parsing truncates bits

[Stefan Karlsson]

Hi Gerard,

I ran the suggested flag boundary tests and found that some stack setup 
code gets an intermittent overflow/underflow because I increased the 
maximum value for ThreadStackSize.

I've a new proposal: Let's set the max limit of ThreadStackSize to 1 * 
M. This value is aligned to our page sizes, and it's small enough that 
the K scaled Xss values fit in a 32-bit signed type. This way we can get 
rid of the complicated alignment conversions and overflow checks in 
parse_xss and it's test.

I also changed so that -Xss now support 0 as an input value, so now the 
min and max sizes truly match the globals.hpp values, but scaled with K.

Here are the patches for that:
  http://cr.openjdk.java.net/~stefank/8178491/webrev.02
  http://cr.openjdk.java.net/~stefank/8178491/webrev.02.delta

What do you think?

Thanks,
StefanK

On 2017-06-29 21:33, Stefan Karlsson wrote:
> Hi Gerard,
>
> On 2017-06-29 17:49, Gerard Ziemski wrote:
>> hi Stefan,
>>
>> Thank you for doing this.
>>
>> Please see below for my comments.
>>
>>> From: Stefan Karlsson <stefan.karlsson at oracle.com>
>>> Subject: Re: RFR: 8178491: -Xss and -XX:ThreadStackSize argument 
>>> parsing truncates bits
>>> Date: June 29, 2017 at 6:33:19 AM CDT
>>> To: "hotspot-runtime-dev at openjdk.java.net" 
>>> <hotspot-runtime-dev at openjdk.java.net>
>>>
>>> Hi all,
>>>
>>> I got a request form Kim to remove the friend macrology in the test. 
>>> Here's the update webrevs:
>>> http://cr.openjdk.java.net/~stefank/8178491/webrev.01
>>> http://cr.openjdk.java.net/~stefank/8178491/webrev.01.delta
>>>
>>> StefanK
>>>
>>> On 2017-06-28 16:05, Stefan Karlsson wrote:
>>>> Hi all,
>>>>
>>>> Please review this patch to fix bugs in the -Xss parsing.
>>>>
>>>> http://cr.openjdk.java.net/~stefank/8178491/webrev.00/
>>>> https://bugs.openjdk.java.net/browse/JDK-8178491
>>>>
>>>> I found this bug while working on an Enhancement to the align 
>>>> functions:
>>>> https://bugs.openjdk.java.net/browse/JDK-8178489
>>>>
>>>> In arguments.cpp we have the following code in the section that 
>>>> converts the Xss value to a ThreadStackSize value:
>>>> round_to((int)long_ThreadStackSize, K) / K) != Flag::SUCCESS)
>>>>
>>>> This explicit narrowing cast throws away the upper 32 bits of 
>>>> long_ThreadStackSize, the resulting int is then promoted to an 
>>>> intptr_t used as a argument to round_to. This causes weird error 
>>>> messages and behavior. For example, running:
>>>> java -Xss2147483648 -version
>>>>
>>>> Yields:
>>>> intx ThreadStackSize=18014398507384832 is outside the allowed range 
>>>> [ 0 ... 9007199254740987 ]
>>>>
>>>> My initial reaction was that this could be solved by simply fixing 
>>>> the cast, but further inspection of the code parsing -Xss and 
>>>> ThreadStackSize revealed more issues.
>>>>
>>>> Changes in this patch:
>>>>
>>>> 1) Extract the -Xss parsing code into a separate, testable 
>>>> function. Unit tests added.
>>>>
>>>> 2) Extended parse_memory_size and check_memory_size, to take the 
>>>> maximum input value. Unit tests added.
>>>>
>>>> 3) Provide an upper limit for the values allowed in the call to 
>>>> parse_memory_size in the -Xss parsing. The upper limit is 
>>>> calculated backwards from the the upper limit of the ThreadStackSize.
>>>>
>>>> 4) The max input value for ThreadStackSize was artificially limited 
>>>> by the fact that round_to was used, and that limited values to fit 
>>>> in an intptr_t, even though size_t is used when the ThreadStackSize 
>>>> is finally expanded into page size aligned bytes. This meant that 
>>>> we had an extremly high upper limit, but it wasn't the absolute 
>>>> upper limit. I changed this to the absolute upper limit, so that 
>>>> it's easier to reason about why this limit was chosen. To make this 
>>>> possible, I replaced the usages round_to and round_down, with 
>>>> align_size_up_ and align_size_down_ macros. In future patches, when 
>>>> the input parameter types of our align functions have been fixed, 
>>>> I'll convert the usages of these macros.
>>>>
>>>> 5) More asserts to check for unintended overflows.
>>>>
>>>> IMHO, It's a bit silly to allow this high values for -Xss, and we 
>>>> could probably limit it to more practical values. I experimented 
>>>> with that, but I found that the code and tests became more 
>>>> unintuitive than they already are. So, I'd like to leave that as a 
>>>> future enhancement if the maintainers of this code allows it.
>>>>
>>>> Tested with JPRT, TooSmallStackSize.java, and 
>>>> TestThreadStackSizes.java. Any suggestions on how to further test 
>>>> this?
>> #1 Re src/share/vm/runtime/arguments.cpp
>>
>> +  // The min and max sizes match the values in globals.hpp.
>> +  const julong min_size = 1000;
>>
>> The comment refers the reader to globals.hpp for min, but where 
>> exactly in globals.hpp is this value?
>>
>> I see this line in globals.hpp file:
>>
>> +          range(0, intx(align_size_down_(max_uintx, 
>> os::vm_page_size())/K)) \
>>
>> which would imply 0 as the min, and I also see this line in 
>> arguments.cpp:
>>
>> -      ArgsRange errcode = parse_memory_size(tail, 
>> &long_ThreadStackSize, 1000);
>>
>> which uses 1000 as min.
>>
>> Should we change this line in globals.hpp, from:
>>
>> +          range(0, intx(align_size_down_(max_uintx, 
>> os::vm_page_size())/K)) \
>>
>> to:
>>
>> +          range(1000, intx(align_size_down_(max_uintx, 
>> os::vm_page_size())/K)) \
>>
>> to make things consistent? Then the comment can stay as is.
>
> No, the comment is confusing.
>
> We have two different ranges, one for ThreadStackSize (scaled down by 
> 1024) and another for -Xss (unscaled). I want to somehow direct the 
> reader over to the globals.hpp file and read the range for 
> ThreadStackSize, to be able to understand the ranges in parse_xss. I 
> think the max values are somewhat understandable, but the min_size = 
> 1000 isn't. Note, that parse_xss aligns all input values against 1024, 
> so we could actually set the min_size to 1 instead of 1000. Maybe we 
> could even allow 0, and at that point the two ranges would match 
> better. Is it important that ThreadStackSize can be set to 0? IIRC, 
> there are some code paths that interpret ThreadStackSize == 0 as a way 
> for the heuristics to choose value.
>
> I could drop the comment about min size, if we don't want to spend 
> more time on that part:
>
> +  // The max size match the value in globals.hpp.
>
>>
>>
>> #2 Re src/share/vm/runtime/arguments.cpp
>>
>> +    bool silent = option == NULL; // Allow testing to silence error 
>> messages
>>
>> Would adding brackets like:
>>
>> +    bool silent = (option == NULL); // Allow testing to silence 
>> error messages
>>
>> make things a bit easier to read?
>
> I can make that change.
>
>>
>>
>> #3 Can you verify with 
>> “hotspot/test/runtime/CommandLine/OptionsValidation” test?
>
> Sure.
>
> StefanK
>>
>>
>> cheers
>>
>