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
8251438: Issues with our POSIX set_signal_handler() #1680
Conversation
|
@gerard-ziemski The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
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.
Hi Gerard,
Functionally this is fine. Stylistically I would have just done simply:
return (act.sa_flags & SA_SIGINFO) != 0 ? CAST_FROM_FN_PTR(address, act.sa_sigaction) : CAST_FROM_FN_PTR(address, act.sa_handler);
Thanks,
David
@gerard-ziemski This change now passes all automated pre-integration checks. 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 130 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.
|
Thank you David for the review, but I need to pull it out of JDK16 and redo for JDK17. I think I need to withdraw and close it. |
I was told that all I need to do is to wait till official JDK16->JDK17 switch over, and can use this PR for JDK17. |
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.
Hi Gerard,
I was confused about the patch. Its fine from a simplification standpoint - putting all those ? expressions into one place - but where were we relying on different storage?
Cheers, Thomas
Mailing list message from David Holmes on hotspot-runtime-dev: Hi Thomas, On 11/12/2020 5:22 pm, Thomas Stuefe wrote:
We were reading both fields from the same structure without checking the void* oldhand = oldAct.sa_sigaction Cheers, |
Ah, I missed that. Thanks! |
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 good now!
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 good.
Thanks,
David
/integrate |
@gerard-ziemski Since your change was applied there have been 138 commits pushed to the
Your commit was automatically rebased without conflicts. Pushed as commit 70183f4. |
Thank you David and Thomas for the reviews! |
This is a fix for a potential issue involving "The storage occupied by sa_handler and sa_sigaction may overlap, and a conforming application shall not use both simultaneously." https://pubs.opengroup.org/onlinepubs/009695399/functions/sigaction.html, when we in fact do assume different storages.
Progress
Issue
Reviewers
Download
$ git fetch https://git.openjdk.java.net/jdk pull/1680/head:pull/1680
$ git checkout pull/1680