8371762: Incorrect use of checked_cast in Arguments::process_settings_file

[xmas92]

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

• Testing
  • GHA
  • Running tier 1-3 on Oracle supported platforms
  • Manual verification with settings file containing non-ascii characters

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8371762: Incorrect use of checked_cast in Arguments::process_settings_file (Bug - P4)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/28283/head:pull/28283
$ git checkout pull/28283

Update a local copy of the PR:
$ git checkout pull/28283
$ git pull https://git.openjdk.org/jdk.git pull/28283/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 28283

View PR using the GUI difftool:
$ git pr show -t 28283

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/28283.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back aboldtch! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[openjdk]

@xmas92 The following label will be automatically applied to this pull request:

• hotspot-runtime

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 00: Full (6deed6fb)