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

Ioi Lam iklam at openjdk.java.net
Thu Mar 17 05:17:46 UTC 2022


> 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 with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 13 additional commits since the last revision:

 - Merge branch 'master' of https://github.com/openjdk/jdk into new-interger-argument-parser
 - @kimbarrett comments -- avoid using "long" type and revert the implementation to 2a5e3e6df3e61b15126ca0582aada57955a25a07, and updated comments
 - Added all edge cases for k/m/g/t suffixes
 - @dholmes-ora comments
 - Merge branch 'master' into new-interger-argument-parser
 - fixed handling of errno
 - add #include <limits> for std::numeric_limits<T>
 - avoid UB in multiply_by_1k(); more clean-up
 - avoid UB operations in k/m/g/t muplication
 - refactored test cases
 - ... and 3 more: https://git.openjdk.java.net/jdk/compare/87e74c03...e9273c31

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/7763/files
  - new: https://git.openjdk.java.net/jdk/pull/7763/files/449c4ca2..e9273c31

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7763&range=05
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7763&range=04-05

  Stats: 5020 lines in 171 files changed: 2528 ins; 1800 del; 692 mod
  Patch: https://git.openjdk.java.net/jdk/pull/7763.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/7763/head:pull/7763

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


More information about the hotspot-runtime-dev mailing list