RFR: 8371762: Incorrect use of checked_cast in Arguments::process_settings_file [v2]

[Axel Boldt-Christmas]

> [JDK-8313882](https://bugs.openjdk.org/browse/JDK-8313882) introduced a `checked_cast` when to silence the Wconversion warnings when we store an `int` in a `char`. The problem is that the assumption that the value in the `int` we get from `getc` is compatible with `char` is incorrect.
> 
> The specification for `getc` is that it returns either `EOF` or the next read `unsigned char` converted to an `int`. This means we have the possible values of `{EOF, 0-255}`, we always check for `EOF` first, so we end up only having `{0-255}` as the possible values. (While a char has the possible values of `{-128-127}`.)
> 
> So first the problem with `checked_cast` (which ensures that the type roundtrip is lossless) will end up converting any value in `{128-255}` to `{-128, -1}` which when converted back to an `int` is `{-128, -1}` and not `{128-255}`.
> 
> The thing is that this is not a problem except for the assert as we do not care if the conversion roundtrip is lossless. We only want to reinterpret the value as a `char`, and never try to regain the unsigned representation converted to an `int` that `getc` uses.
> 
> Another issue with keeping the value inside an `int` which could lead to bugs in the future is that when we use it in equality comparison agains a `char` we will encounter an integer promotion of the `char` which will lead to surprising incorrect results if the character is non-ascii.
> 
> Currently we have no such comparisons of this `int` vs a non-ascii character. So the only issue is the assert inside `checked_cast` which will trigger if the settings file contains a non-ascii character.
> 
> I suggest we convert the return value from `getc` to a `char` after we have check for `EOF` and before we use it as a character.
> 
> It is worth noting that the `(unsigned char)` casts for the calls into `isspace` (introduced by [JDK-8332400](https://bugs.openjdk.org/browse/JDK-8332400)) is very important to get the correct value for its `int` input parameter. (So we get the correct non-sign extended cast `char -> unsigned char -> int`.) That issue mentions the possibility of introducing our own `isspace` (suggestion was `os::isspace`) which we could overload the different types and do the correct thing with bound checks. This might be something we want to revisit.
> 
> _`isspace` specification:_
>>The behavior is undefined if the value of ch is not representable as unsigned char and is not equal to [EOF](https://en.cppreference.com/w/c/io.html).
> 
> * Testing
>   * GHA
>   * Running tier 1-3 on Oracl...

Axel Boldt-Christmas has updated the pull request incrementally with one additional commit since the last revision:

  Update src/hotspot/share/runtime/arguments.cpp
  
  Co-authored-by: David Holmes <62092539+dholmes-ora at users.noreply.github.com>

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

Changes:
  - all: https://git.openjdk.org/jdk/pull/28283/files
  - new: https://git.openjdk.org/jdk/pull/28283/files/6deed6fb..8d9337d4

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=28283&range=01
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=28283&range=00-01

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.org/jdk/pull/28283.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/28283/head:pull/28283

PR: https://git.openjdk.org/jdk/pull/28283