8263721: Unify oop casting

[stefank]

In fastdebug builds we replace the "oopDesc* to oop" typedef with a wrapper class that holds an oopDesc*. This wrapper class allows any kind of pointer to be implicitly converted to an oop. So you can write code like this:

  Metadata* m = (Metadata*)0x123;
  oop o = m;

and the compiler will unfortunately accept it. Fortunately, this will be caught in release builds, because you can't convert a Method* into an oopDesc*.

One interesting thing is that you can't convert values of integral type too oops:

  uintptr_t m = uintptr_t(123);
  oop o = m;

This fails in both fastdebug and release builds. To be able to convert integral values to oops, there are two helper functions:

// For CHECK_UNHANDLED_OOPS, it is ambiguous C++ behavior to have the oop
// structure contain explicit user defined conversions of both numerical
// and pointer type. Define inline methods to provide the numerical conversions.
template <class T> inline oop cast_to_oop(T value) {
  return (oop)(CHECK_UNHANDLED_OOPS_ONLY((void *))(value));
}
template <class T> inline T cast_from_oop(oop o) {
  return (T)(CHECK_UNHANDLED_OOPS_ONLY((oopDesc*))o);
}

So, the above example would have to be written as:

  uintptr_t m = uintptr_t(123);
  oop o = cast_to_oop(m);

My proposal is that we stop allowing implicit (and explicit) casts from void*, and instead use cast_to_oop whenever we want to cast to oops. We would still allow oopDesc* to be implicitly converted to oop. This would also allow NULL to be converted too oop without casting:

  oop o = NULL;

This will make the code to convert oops a little bit longer. It could be argued that that's a good thing, because everyone should be cautious about converting things into oops. This will also give us one entry-point where we could add (probably temporary) verification code.

An alternative to the suggestion above, could be to completely get rid of cast_to_oop and cast_from_oop. But for that to work we need to stop using NULL, which is an integral 0, and start to use nullptr for oops. I've prototyped this as well, but initial investigations showed that some tended to prefer having the cast_to_oop function. (We could still move from NULL to nullptr, if we think that is a good idea).

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed

Issue

• JDK-8263721: Unify oop casting

Reviewers

• Kim Barrett (@kimbarrett - Reviewer) ⚠️ Review applies to 96d8ca3
• Coleen Phillimore (@coleenp - Reviewer) ⚠️ Review applies to 110ecd6

Download

To checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3047/head:pull/3047
$ git checkout pull/3047

To update a local copy of the PR:
$ git checkout pull/3047
$ git pull https://git.openjdk.java.net/jdk pull/3047/head

[stefank]

/label hotspot

[bridgekeeper]

👋 Welcome back stefank! 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]

@stefank this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout 8263721_unify_oop_casting
git fetch https://git.openjdk.java.net/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

[openjdk]

@stefank
The hotspot label was successfully added.

[mlbridge]

Webrevs

• 02: Full (bd0ab14)
• 01: Full (110ecd6)
• 00: Full (96d8ca3)

[kimbarrett]

Looks good. Just the one minor nit.

There are a lot of casts to void* as an argument that ought to be cleaned up. And the set of valid types for the cast functions ought to be restricted to "reasonable" types. But that can be a followup.

[kimbarrett]

NULL -> nullptr

[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:

8263721: Unify oop casting

Reviewed-by: kbarrett, coleenp

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 no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[coleenp]

Looks like a safe to me. I didn't realize there were still so many files with oop casts. I only scrolled by the GC ones though.

[stefank]

Thanks for reviewing!
/integrate

[openjdk]

@stefank Since your change was applied there have been 4 commits pushed to the master branch:

• 329697b: 8263358: Update java.lang to use instanceof pattern variable
• ae9af57: 8264001: JFR: Modernize implementation
• fad8484: 8263411: Convert jshell tool to use Stream.toList()
• 06d46d6: 8264008: Incorrect metaspace statistics after JEP 387 when UseCompressedClassPointers is off

Your commit was automatically rebased without conflicts.

Pushed as commit a79f095.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.