-
Notifications
You must be signed in to change notification settings - Fork 6.2k
8294211: Zero: Decode arch-specific error context if possible #10397
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 shade! A progress list of the required criteria for merging this PR into |
Webrevs
|
tstuefe
left a comment
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.
Good! But why make this conditional with a switch? Who would not want to have better error information?
Because I want to be able to test the generic error handling paths that would run on "generic" arch, without leaving the comfort of my x86_64 machine :) |
:-) Okay. Like zero-in-zero. |
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.
BTW, if you want to test error handling, you don't have to use Unsafe put, you can just use -XX:ErrorHandlerTest=number, e.g. 15 should give you a Segfault I believe. Only works in debug VMs though.
|
@shipilev 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 no new commits pushed to the ➡️ To integrate this PR with the above commit message to the |
|
I think this still works. Any other reviews, please? |
Ping. :) |
|
/integrate |
|
Going to push as commit 3f3d63d.
Your commit was automatically rebased without conflicts. |
After POSIX signal refactorings, Zero error handling had "regressed" a bit: Zero always gets
NULLaspcin error handling code, and thus it fails with SEGV at pc=0x0. We can do better by implementing context decoding where possible.Unfortunately, this introduces some arch-specific code in Zero code. The arch-specific code is copy-pasted (with inline definitions, if needed) from the relevant
os_linux_*.cppfiles. The unimplemented arches would still report the same confusinghs_err-s. We can emulate (and thus test) the generic behavior using new diagnostic VM option.This reverts parts of JDK-8259392.
Sample test:
Linux x86_64 Zero fastdebug crash currently:
Linux x86_64 Zero fastdebug crash with this patch:
Linux x86_64 Zero fastdebug crash with this patch and
-XX:-DecodeErrorContext:Additional testing:
tier1Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/10397/head:pull/10397$ git checkout pull/10397Update a local copy of the PR:
$ git checkout pull/10397$ git pull https://git.openjdk.org/jdk pull/10397/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 10397View PR using the GUI difftool:
$ git pr show -t 10397Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/10397.diff