-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
8289060: Undefined Behaviour in class VMReg #9276
Conversation
👋 Welcome back aph! A progress list of the required criteria for merging this PR into |
@theRealAph The following label will be automatically applied to this pull request:
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. |
Webrevs
|
I've been thinking about this some more after you fixed the same issue for To avoid that, I think we could theoretically convert every member function to a static function that takes a [1] : #6280 (comment) I think the patch looks good overall, but it looks like there are some failures in some of the SA tests. |
Ah, I see. That makes sense.
Right. I'll start digging. |
Fixed now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@theRealAph 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:
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 99 new commits pushed to the
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. ➡️ To integrate this PR with the above commit message to the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks reasonable. Let me test it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My testing passed.
/integrate |
Going to push as commit dfb24ae.
Your commit was automatically rebased without conflicts. |
@theRealAph Pushed as commit dfb24ae. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Like class
Register
, classVMReg
exhibits undefined behaviour, in particular null pointer dereferences.The right way to fix this is simple: make instances of
VMReg
point to reified instances ofVMRegImpl
. We do this by creating a static array ofVMRegImpl
, and making allVMReg
instances point into it, making the code well defined.However, while
VMReg
instances are no longer null, and so do not generate compile warnings or errors, there is still a problem in that higher-numberedVMReg
instances point outside the static array ofVMRegImpl
. This is hard to avoid, given that (as far as I can tell) there is no upper limit on the number of stack slots that can be allocated asVMReg
instances. While this is in theory UB, it's not likely to cause problems. We could fix this by creating a much larger static array ofVMRegImpl
, up to the largest plausible size of stack offsets.We could instead make
VMReg
instances objects with a single numeric field rather than pointers, but some C++ compilers pass all such objects by reference, so I don't think we should.Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/9276/head:pull/9276
$ git checkout pull/9276
Update a local copy of the PR:
$ git checkout pull/9276
$ git pull https://git.openjdk.org/jdk pull/9276/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 9276
View PR using the GUI difftool:
$ git pr show -t 9276
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/9276.diff