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

[Kim Barrett]

> 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.

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

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.

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

------------------------------------------------------------------------------ 
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.

------------------------------------------------------------------------------ 
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.

------------------------------------------------------------------------------
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.

------------------------------------------------------------------------------