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

[Gerard Ziemski]

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.


#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?


#3 Can you verify with “hotspot/test/runtime/CommandLine/OptionsValidation” test?


cheers