8374016: [lworld] Too late checked_cast in ResolvedFieldEntry::set_flags

[stefank]

In the lworld branch the new_flags type has been changed to u1 instead of int. We now perform a bunch of bit operations, which results in an int, and then we narrow that down into a u1. After the u1 has been created we check that this doesn't overflow a u1 with the checked_cast check. That's too late.

Mainline code looks like this:

int new_flags = ...
_flags = checked_cast<u1>(new_flags);

and Valhalla code looks like this:

u1 new_flags = ...
_flags = checked_cast<u1>(new_flags);

This voids the benefit of having a checked_cast here. I propose that we revert back to using int new_flags and keep the checked_cast.

While reading this code there were a few things that made the code harder to follow and I've tweaked it.

1. The bool is_volatile_shift is special-treated and instead of shifting it is directly casted to int. That hard codes a knowledge/assumption that the flag is always going to be 1 (and maybe also what value bools have).

That made me have to look at the actual values of these and at one point I thought that is_volatile_shift was 1 and not 0 and that the above was a bug. The main reason I thought that was this odd order here:

   u1 new_flags = ((is_final_flag ? 1 : 0) << is_final_shift) | static_cast<int>(is_volatile_flag) |
      ((is_flat_flag ? 1 : 0) << is_flat_shift) |
      ((is_null_free_inline_type_flag ? 1 : 0) << is_null_free_inline_type_shift) |
      ((has_null_marker_flag ? 1 : 0) << has_null_marker_shift);

Here the order of usage is

is_final_flag
is_volatile_flag
is_flat_flag
is_null_free_inline_type_flag
has_null_marker_flag

but the enum order has final and volatile swapped:

is_volatile_shift
is_final_shift
is_flat_shift
is_null_free_inline_type_shift
has_null_marker_shift

I'd prefer if we stayed consistent with the order, to lower the risk that some bugs sneak in here. So, I've replaced the cast with a shift (just like the other values are handled) and I ordered functions, parameters, and operations in the same order as the enums.

2. Tweaked some alignments to make these kind of issues clearer.

Please tell me if I went to far with the style changes.

---

Progress

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

Issue

• JDK-8374016: [lworld] Too late checked_cast in ResolvedFieldEntry::set_flags (Enhancement - P4)

Reviewers

• Frederic Parain (@fparain - Committer)
• Matias Saavedra Silva (@matias9927 - Committer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1844

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/valhalla/pull/1844.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

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

[openjdk]

@stefank This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8374016: [lworld] Too late checked_cast in ResolvedFieldEntry::set_flags

Reviewed-by: fparain, matsaave

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 111 new commits pushed to the lworld branch:

• 4b318c8: Merge
• 4f283f1: 8373820: C2: Robust Node::uncast_helper infinite loop check
• d8eb125: 8373866: Refactor java/net/httpclient/ThrowingSubscribers*.java tests to use JUnit5
• ... and 108 more: https://git.openjdk.org/valhalla/compare/69399cedf6fe208832a66c134d370af860154bc2...lworld

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@fparain, @matias9927) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

[mlbridge]

Webrevs

• 00: Full (c7001418)

[fparain]

Good point about the checked_cast.
Style changes are good if they make the code more explicit to the reader.
Thank you for these fixes.

[matias9927]

Change looks good and makes the code more clear, thanks!