Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8264268: Don't use oop types for derived pointers #3214

Closed
wants to merge 7 commits into from

Conversation

@stefank
Copy link
Member

@stefank stefank commented Mar 26, 2021

The JIT compiler embeds pointers to addresses within an object. These are called derived pointers. When the GC moves objects, these pointers need to be updated explicitly, because the GC only deals with the real oops of the objects (base pointer).

The code that deals with this uses oop* for the address containing the base pointer. This is fine, the address contains an oop. However, it also uses oop* for the interior pointer, even though the contents is not a valid oop.

This creates temporary oops that does not conform to the normal requirements for oops. For example, the lower three bits could be set. This makes it problematic to write stricter verification code.

I propose that we use intptr_t* instead of oop*, and only use oop* when the location is known to contain a valid oop.


Progress

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

Issue

  • JDK-8264268: Don't use oop types for derived pointers

Reviewers

Download

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

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Mar 26, 2021

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

Loading

@openjdk openjdk bot added the rfr label Mar 26, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 26, 2021

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

  • hotspot

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.

Loading

@openjdk openjdk bot added the hotspot label Mar 26, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 26, 2021

Webrevs

Loading

@openjdk openjdk bot mentioned this pull request Mar 26, 2021
3 tasks
rose00
rose00 approved these changes Mar 27, 2021
Copy link
Contributor

@rose00 rose00 left a comment

Looks good.

Maybe add typedef intptr_t derived_oop_t to make the operations on d-oops a little more distinctive.

Maybe add a static assert that sizeof(oop) == sizeof(intptr_t), just in case there's a problem with compressed oops in the future.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Mar 27, 2021

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

8264268: Don't use oop types for derived pointers

Reviewed-by: jrose, kbarrett

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.

Loading

@openjdk openjdk bot added the ready label Mar 27, 2021
Copy link

@kimbarrett kimbarrett left a comment

I agree with jrose that having a type name for derived pointers would be better. But I wondered if it might be possible to make it stronger than just an alias type for intptr_t. I tried making it an enum class with a few supporting operations, and it doesn't look too bad.
https://github.com/kimbarrett/openjdk-jdk/tree/derived_ptr

Loading

src/hotspot/share/compiler/oopMap.cpp Outdated Show resolved Hide resolved
Loading
src/hotspot/share/compiler/oopMap.cpp Outdated Show resolved Hide resolved
Loading
@stefank
Copy link
Member Author

@stefank stefank commented Mar 29, 2021

Looks good.

Thanks.

Maybe add typedef intptr_t derived_oop_t to make the operations on d-oops a little more distinctive.

I used Kim's enum suggestion.

Maybe add a static assert that sizeof(oop) == sizeof(intptr_t), just in case there's a problem with compressed oops in the future.

Done.

Loading

@stefank
Copy link
Member Author

@stefank stefank commented Mar 29, 2021

I agree with jrose that having a type name for derived pointers would be better. But I wondered if it might be possible to make it stronger than just an alias type for intptr_t. I tried making it an enum class with a few supporting operations, and it doesn't look too bad.
https://github.com/kimbarrett/openjdk-jdk/tree/derived_ptr

Sounds good. I've taken your patch and and did some minor cleanups.

Loading

@stefank
Copy link
Member Author

@stefank stefank commented Mar 29, 2021

/integrate

Loading

@openjdk openjdk bot closed this Mar 29, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Mar 29, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 29, 2021

@stefank The command integrate can only be used in open pull requests.

Loading

@stefank stefank deleted the 8264268_dervied_pointer_types branch May 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants