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

[Kim Barrett]

On Fri, 2 Jan 2026 04:26:57 GMT, Axel Boldt-Christmas <aboldtch at openjdk.org> wrote:

>> [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).
>> 
>> * Testin...
>
> Axel Boldt-Christmas 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 four additional commits since the last revision:
> 
>  - Merge tag 'jdk-27+3' into JDK-8371762
>    
>    Added tag jdk-27+3 for changeset 4f283f18
>  - Merge tag 'jdk-26+26' into JDK-8371762
>    
>    Added tag jdk-26+26 for changeset 847fbab7
>  - Update src/hotspot/share/runtime/arguments.cpp
>    
>    Co-authored-by: David Holmes <62092539+dholmes-ora at users.noreply.github.com>
>  - 8371762: Incorrect use of checked_cast in Arguments::process_settings_file

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

> 1217: 
> 1218:   int c_or_eof = getc(stream);
> 1219:   while (c_or_eof != EOF && pos < (int)(sizeof(token) - 1)) {

[pre-existing] We wouldn't need the cast if `pos` were correctly typed as `size_t`.

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

> 1223:     // below (which would be incorrect if we ever compared to a non-ascii char),
> 1224:     // and the int to char conversions we would otherwise do in the assignments.
> 1225:     const char c = checked_cast<char>(checked_cast<unsigned char>(c_or_eof));

Why is this using `checked_cast` at all?  Just assume `getc` correctly
implements its documented behavior, and use `static_cast<char>(c_or_eof)` to
silence (hopefully eventual) `-Wconversion` warnings.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/28283#discussion_r2658318458
PR Review Comment: https://git.openjdk.org/jdk/pull/28283#discussion_r2658316948