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

[Stefan Karlsson]

On 2017-06-30 00:56, Kim Barrett wrote:
>> On Jun 29, 2017, at 5:39 PM, Stefan Karlsson <stefan.karlsson at oracle.com> wrote:
>>
>> […]
>> 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
> ------------------------------------------------------------------------------
> src/os/windows/vm/os_windows.cpp
> 4046   size_t stack_commit_size = align_size_up_((size_t)ThreadStackSize * K, (size_t)os::vm_page_size());
>
> Why is this using the aligning macro? It was my impression that the
> macros were supposed to be implementation details that are also
> exposed for use in constexpr contexts, since we don't have C++11/14
> constexpr functions.  The functions are preferred in other contexts.

I vaguely explained it in bullet (4) in my original mail. The reason was 
that (size_t)ThreadStackSize * K doesn't necessarily fit an intptr_t, 
and that round_up takes an intptr_t as parameter. I also said:
"In future patches, when the input parameter types of our align 
functions have been fixed, I'll convert the usages of these macros."

>
> (Of course, the functions have type issues, and I think in many cases
> really need templating, but that's a problem for another day.)

Yes, as soon as I get through this and the align macro bug, I'll send out:
https://bugs.openjdk.java.net/browse/JDK-8178489

>
> And why is this being changed at all?  Maybe ThreadStackSize * K can
> overflow intptr_t?  Though not with the latest change for max
> ThreadStackSize = 1 * M.

Right. I'll revert this change.

>
> Of course, there's the interesting question of why we have both
> align_size_up/down and round_to/down.

I'm taking care of that with:
https://bugs.openjdk.java.net/browse/JDK-8178500

>
> ------------------------------------------------------------------------------
> src/share/vm/runtime/arguments.cpp
> 2760   const julong min_ThreadStackSize = 0;
> 2761   const julong max_ThreadStackSize = 1 * M;
>
> Isn't there some data structure in the argument checking code that
> contains the values registered in globals.hpp?
>
> Hm, the data structure I was thinking of can obtained via
>
>    CommandLineFlagRangeList::find("ThreadStackSize");
>
> But it doesn't provide access to the range values. It only provides a
> suite of check functions.
>
> But that might be enough. Change the call to parse_memory_size to use
> the type's min/max rather than (scaled) min/max for ThreadStackSize,
> to just do parsing. Then look up the CommandLineFlagRange object, and
> use it to perform the range check on the (aligned and descaled) parsed
> value.
>
> The point of all this is to avoid (hopefully) duplicating the range
> information for ThreadStackSize here.

I understand the concern of the duplication, but unless there is a 
straight forward way to extract the range values out of the command line 
classes, then I'd like to leave this as an RFE for the maintainers of 
the command line range checking.

>
> ------------------------------------------------------------------------------
> test/native/runtime/test_arguments.cpp
>    45 TEST_F(ArgumentsTest, atojulong) {
>
> Why is this test being fixturized?  It doesn't refer to anything
> provided by the fixture, so far as I can tell.
I want to put all the tests in the same test case (ArgumentsTest). This 
is what you get if you use TEST here:

"/home/stefank/hg/jdk10/hs/test/fmw/gtest/src/gtest.cc:1977: Failure
Failed
All tests in the same test case must use the same test fixture
class, so mixing TEST_F and TEST in the same test case is
illegal.  In test case ArgumentsTest,
test parse_xss_test_vm is defined using TEST_F but
test atojulong_test is defined using TEST.  You probably
want to change the TEST to TEST_F or move it to another test
case."

>
> ------------------------------------------------------------------------------
> test/native/runtime/test_arguments.cpp
>   148 // Wrapper around the help function - gives file and line number when a test failure occurs.
>   149 #define parse_xss_inner(str, expected_err) ArgumentsTest::parse_xss_inner_annotated(str, expected_err, __FILE__, __LINE__)
>
> It seems like SCOPED_TRACE ought to be helpful here, but I haven't
> come up with a way to use it that seems better than what you have with
> this helper macro.  But maybe something to think about.

I didn't know about that feature. Thanks.

StefanK
>
> ------------------------------------------------------------------------------
>