-
Notifications
You must be signed in to change notification settings - Fork 6.2k
8356324: JVM crash (SIGSEGV at ClassListParser::resolve_indy_impl) during -Xshare:dump starting from 21.0.5 #26770
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
Conversation
👋 Welcome back ksakata! A progress list of the required criteria for merging this PR into |
@jyukutyo 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 62 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 |
Have you figured out why it started crashing with 21.0.5? I.e. what was the change that triggered it? |
Thank you for your question. I tried to identify the change that caused the crash by comparing 21.0.4 and 21.0.5, but I wasn't able to pinpoint it with my initial check. |
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. I think resolved_indy_entry_at() should be valid because the index is a known indy so it would not have a null _resolved_indy_entries. Calling the length function may be called when dumping, so does need to defend against null as you saw in the crash.
The resolved indy work went in JDK 21 baselevel 16 - don't know which update that would be though.
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.
I believe the constant pool cache refactor was introduced in JDK 21 which explains why older classlist files may cause some trouble. The fix looks good and I believe it should be sufficient as we don't tend to call resolved_indy_entry_at()
if we know that _resolved_indy_entries
is null. Thanks!
Thank you for the reviews, Coleen and Matias! |
/integrate |
Going to push as commit 506625b.
Your commit was automatically rebased without conflicts. |
A crash occurs when running with
-Xshare:dump
and specifying a class list file generated by an older JDK (e.g. JDK 17) via-XX:SharedClassListFile
.This pull request fixes the issue and prevents the crash.
Details
Example command to reproduce:
The class list file that triggers the problem, generated by JDK 17, looks like this:
In contrast, the recent JDK generates class list contents as follows:
When an old-style class list file is used, the
_resolved_indy_entries
variable remains null, which leads to a crash during-Xshare:dump
.This PR adds a null check to avoid the crash.
After applying this fix, running with the same options no longer results in a crash. Instead, a warning is shown as expected:
Additional note
We may also want to apply the same fix to the
resolved_indy_entry_at
function in theConstantPoolCache
class. Please let me know if that is desirable.For reference, the current implementation is:
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/26770/head:pull/26770
$ git checkout pull/26770
Update a local copy of the PR:
$ git checkout pull/26770
$ git pull https://git.openjdk.org/jdk.git pull/26770/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 26770
View PR using the GUI difftool:
$ git pr show -t 26770
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/26770.diff
Using Webrev
Link to Webrev Comment