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
8305066: [JVMCI] guarantee(ik->is_initialized()) failed: java/lang/Long$LongCache must be initialized #13229
Conversation
👋 Welcome back dnsimon! A progress list of the required criteria for merging this PR into |
@@ -192,15 +192,6 @@ public static HotSpotJVMCIRuntime runtime() { | |||
// initialized. | |||
JVMCI.getRuntime(); | |||
} | |||
// Make sure all the primitive box caches are populated (required to properly |
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.
This code has not been necessary ever since
JVMCI::ensure_box_caches_initialized(CHECK_(JVMCI::ok)); |
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.
How are the caches uninitialized if we have forced their initialization?
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.
A box cache initialization can fail due to StackOverflowError
. In this case, we must not rematerialize a virtual object corresponding to a value that would be read from a box cache.
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.
This looks good to me. There's really no other way to handle it.
@dougxc 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 52 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 ok
Thanks @veresov and @tkrodriguez for reviewing. /integrate |
Going to push as commit e012685.
Your commit was automatically rebased without conflicts. |
Class initialization can fail for the boxing caches such as
Long$LongCache
due to things such as aStackOverflowError
.When rematerializing boxed values during deoptimization, a failed cache initialization must be handled. This PR handles it by throwing an OOME which is always semantically correct.
Note that a VM with a broken boxing cache is going to be in a fairly miserable state (i.e. throwing
NoClassDefFoundError
on every call toLong.valueOf
) so the OOME shouldn't cause any noticeable problem.Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/13229/head:pull/13229
$ git checkout pull/13229
Update a local copy of the PR:
$ git checkout pull/13229
$ git pull https://git.openjdk.org/jdk.git pull/13229/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 13229
View PR using the GUI difftool:
$ git pr show -t 13229
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/13229.diff