-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
JDK-8322163: runtime/Unsafe/InternalErrorTest.java fails on Alpine after JDK-8320886 #17175
Conversation
👋 Welcome back mbaesken! A progress list of the required criteria for merging this PR into |
Webrevs
|
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 think it's very bad to let C code run into signals (SIGBUS or SIGSEGV) regardless of your change. In addition, the signal handler just skips the faulting instruction and continues with the next one. That does not yield defined behavior!
(On linux aarch64, the new loop doesn't work, because gcc generates an strb instruction with internal address increment. The signal prevents both, the store and the address increment, so we end up in an endless loop.)
If we only want to do a simple fix for Alpine x86_64, I suggest to use the new code only on that platform. It's bad for other platforms, too, but it works because it gets tested. (Doesn't mean that it will always work with every compiler. So, there's still room for improvement.)
Hi Martin,
I adjusted the code so that the loop is only done on Alpine/MUSL . |
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 think this is an acceptable solution.
@MBaesken 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 7 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 ➡️ 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.
Sounds like a reasonable fix.
Hi Martin and Christoph, thanks for the reviews ! /integrate |
Going to push as commit 1230853.
Your commit was automatically rebased without conflicts. |
We notice failures/crashes on Alpine Linux, maybe after JDK-8320886.
test runtime/Unsafe/InternalErrorTest.java crashes on Alpine (works fine on other test OS/CPU platforms) :
Looks like the Alpine memset triggers unexpected SIGSEGV (not the expected SIGBUS). So we switch to a loop instead of memset. However I noticed that on Linux aarch64 the test starts to fail when the loop is used instead of the memset, so I keep the old coding on this platform .
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/17175/head:pull/17175
$ git checkout pull/17175
Update a local copy of the PR:
$ git checkout pull/17175
$ git pull https://git.openjdk.org/jdk.git pull/17175/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 17175
View PR using the GUI difftool:
$ git pr show -t 17175
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/17175.diff
Webrev
Link to Webrev Comment