-
Notifications
You must be signed in to change notification settings - Fork 103
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
8345296: AArch64: VM crashes with SIGILL when prctl is disallowed #1222
Conversation
👋 Welcome back krk! A progress list of the required criteria for merging this PR into |
@krk 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 3 new commits pushed to the
Please see this link for an up-to-date comparison between the source branch of this pull request and the As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@phohensee, @shipilev) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
This backport pull request has now been updated with issue from the original commit. |
Webrevs
|
/issue JDK-8339063 |
@phohensee Only the author (@krk) is allowed to issue 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.
Please add "/issue JDK-8339063" (I'm unsure of the exact syntax, but it's something like that :)), since it's included. Otherwise lgtm.
/issue JDK-8339063 |
@krk |
|
Seeing the report for Corretto 21 that demonstrates this issue affects some MacOS and Alpine configs: |
I am not entirely convinced the modifications that JDK-8339063 does to this code are 100% safe. It tries to install the max vector size and exits the VM if not possible? That sounds like a behavioral change with unknown consequences. And unfortunately, that patch is only in JDK 24, so we do not yet know the impact well. Looks like we are mixing that backport because we need access to new If so, I think it would be safer to just take that particular hunk in |
Thanks for the details. Yes, it was to access the new |
8ec87dc
to
3fc2808
Compare
@krk Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information. |
Picked only the necessary hunks from JDK-8339063. |
Also need |
I only included an independeny chunk, not implementing JDK-8339063. It could be independently reviewed. |
/issue remove JDK-8339063 |
@krk |
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 correct.
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.
Right, include as well.
/approval request Fix VM crash on production workloads when prctl is disallowed, on AArch64 with SVE. |
Yes, we are planning to pick this fix in Corretto release for January. I think this patch is beneficial to have in all January releases as well. This is ultimately 21u and 17u maintainers' call. |
/approve yes Since this is related to prctl returning |
@jerboaa |
/sponsor |
@krk Only Committers are allowed to sponsor changes. |
/integrate |
/sponsor |
Going to push as commit 21b76f3.
Your commit was automatically rebased without conflicts. |
Looking a bit further into this, I do not understand what the "root cause" of this issue is. What architecture does this support currently, if any? Thank you |
This fix makes sure VM does not crash if lower layer (whether native OS or whatever sandbox/virtualization layer) does not allow us to execute
By "this" you mean SVE? SVE is a feature of modern AArch64 CPUs. AFAIU, this means Apple M4+, Graviton 3+, etc. |
Ok. I think I get it now. So the root cause is that Sonoma 15.2 introduced a regression that breaks SVE. thank you. |
Ref. openjdk/jdk#22479
Does not apply cleanly, the backport depends on
FloatRegister
changes from JDK-8339063 which are included insrc/hotspot/cpu/aarch64/register_aarch64.hpp
.Additional testing:
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk21u-dev.git pull/1222/head:pull/1222
$ git checkout pull/1222
Update a local copy of the PR:
$ git checkout pull/1222
$ git pull https://git.openjdk.org/jdk21u-dev.git pull/1222/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1222
View PR using the GUI difftool:
$ git pr show -t 1222
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk21u-dev/pull/1222.diff
Using Webrev
Link to Webrev Comment