RFR: 8282773: Refactor parsing of integer VM options [v4]

[Kim Barrett]

On Fri, 11 Mar 2022 08:17:23 GMT, Ioi Lam <iklam at openjdk.org> wrote:

>> Background:
>> 
>> When a VM option is given an integer value (e.g., `-XX:ActiveProcessorCount=123`), we call the `set_numeric_flag` function in arguments.cpp. The old implementation always calls `strtoull()` to get an unsigned 64-bit number, and then tries to cast/truncate the result to the target type, performing sign conversion as needed.
>> 
>> This is cumbersome and buggy, because the target type (such as `size_t`) may have different bit-widths on different platforms. Also, manual conversion of unsigned to sign values can run into corner cases.
>> 
>> The fix:
>> 
>> - Use C++ templates and SFINAE to automatically choose the correct `parse_integer_impl()` function for parsing the input string. The VM should avoid doing cast and sign conversion as much as possible.
>> - Use a similar technique to write a comprehensive set of test cases to validate the implementation. (This has revealed several failures in the old implementation).
>> 
>> Notes on K/M/G/T suffix handling:
>> 
>> For input like `-XX:ActiveProcessorCount=2048m`, the old implementation checks for overflow with `(n * suffix / suffix) == n`. This works for unsigned types because the [C++ specification](https://en.cppreference.com/w/cpp/language/operator_arithmetic) allows unsigned arithmetic to overflow. However, overflowing a signed integer is UB. Therefore, I wrote a new function `multiply_by_1k()` to check for overflow without potentially undefined behavior. Thanks to @kimbarrett for helping me come up with the solution.
>> 
>> Tests:
>> 
>> tiers 1-2 passed. tiers 3-5 in progress.
>
> Ioi Lam has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Added all edge cases for k/m/g/t suffixes

Changes requested by kbarrett (Reviewer).

src/hotspot/share/runtime/arguments.cpp line 748:

> 746: #endif
> 747: 
> 748: template <typename T, ENABLE_IF(std::is_signed<T>::value), ENABLE_IF(sizeof(T) == 4)> // signed 32-bit

Missing `#include "metaprogramming/enableIf.hpp"` (presumably indirectly included).

src/hotspot/share/runtime/arguments.cpp line 755:

> 753:   if (errno != 0 LP64_ONLY(|| v < min_jint || v > max_jint)) {
> 754:     // long is 64-bit on LP64, so we need explicit range check.
> 755:     return false;

Windows is actually LLP64 rather than LP64 (but we generally pretend otherwise), so that `long` is 32bits on Windows.  That's why we avoid the use of `long` in shared code.  That makes the range tests tautological, and potentially warned about.  (gcc is known to warn about some tautological tests - I don't know if Visual Studio does.)  Maybe instead use `strtoll` and an unconditional range check?

src/hotspot/share/runtime/arguments.cpp line 770:

> 768:   unsigned long v = strtoul(s, endptr, base);
> 769:   if (errno != 0 LP64_ONLY(|| v > max_juint)) {
> 770:     // unsigned long is 64-bit on LP64, so we need explicit range check.

Same issue here about Windows `long` being 32 bits.

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

PR: https://git.openjdk.java.net/jdk/pull/7763