-
Notifications
You must be signed in to change notification settings - Fork 1.2k
[AIX] Use sa_sigaction instead of the union #4250
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
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @tgross35 (or someone else) some time within the next two weeks. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
|
close GH-4249 |
03b30be to
597c610
Compare
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 from the AIX perspective. This makes the implementation common with what is done on other targets
src/unix/aix/mod.rs
Outdated
|
|
||
| pub struct sigaction { | ||
| pub sa_union: __sigaction_sa_union, | ||
| pub sa_sigaction: crate::sighandler_t, //actually a union with sa_handle |
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.
Is this something that should be changed once we are able to use unions (1.0)? If so, could you label the comment FIXME(union)?
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.
Changed as suggested, thanks so much!
597c610 to
2127ff4
Compare
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.
Needs a rebase but the change LGTM. @xingxue-ibm @daltenty do you want this in a stable release since it is a breaking change?
944b7ad to
f49be46
Compare
Done rebasing. Yeah, it would be great if this change could be included in a stable release. What do you think, @daltenty? |
Head branch was pushed to by a user without write access
f49be46 to
fcb9df0
Compare
|
I'll wait for @daltenty to ack before backporting. Just add the label back ( |
|
Thanks! Yep, it'd help a lot of crates to have this in stable as well. It is a breaking change, but honestly it fixes more crates than it breaks since it makes the member access uniform to what we expect from other platforms. @rustbot label +stable-nominated |
(backport <rust-lang#4250>) (cherry picked from commit fcb9df0)
(backport <rust-lang#4250>) (cherry picked from commit fcb9df0)
(backport <rust-lang#4250>) (cherry picked from commit fcb9df0)
(backport <rust-lang#4250>) (cherry picked from commit fcb9df0)
…ler-errors Use sa_sigaction instead of sa_union.__su_sigaction for AIX Revert test cases to use `sa_sigaction` instead of `sa_union.__su_sigaction`, now that the `libc` crate implementation for AIX defines `sa_sigaction` as a direct member of `struct sigaction`, aligning it with implementations on other similar platforms. ([[AIX] Use sa_sigaction instead of the union](rust-lang/libc#4250)).
Rollup merge of rust-lang#138409 - xingxue-ibm:use-sigaction, r=compiler-errors Use sa_sigaction instead of sa_union.__su_sigaction for AIX Revert test cases to use `sa_sigaction` instead of `sa_union.__su_sigaction`, now that the `libc` crate implementation for AIX defines `sa_sigaction` as a direct member of `struct sigaction`, aligning it with implementations on other similar platforms. ([[AIX] Use sa_sigaction instead of the union](rust-lang/libc#4250)).
Use sa_sigaction instead of sa_union.__su_sigaction for AIX Revert test cases to use `sa_sigaction` instead of `sa_union.__su_sigaction`, now that the `libc` crate implementation for AIX defines `sa_sigaction` as a direct member of `struct sigaction`, aligning it with implementations on other similar platforms. ([[AIX] Use sa_sigaction instead of the union](rust-lang/libc#4250)).
Description
The current
libccrate implementation for AIX definesstruct sigactionas containing a unionsa_unionwith two members:__su_handlerand__su_sigaction. This mirrors thestruct sigactiondefinition in the AIX system header. Consequently, any reference tosa_sigactionin Rust code, whether in shipped crates or user code, must be replaced withsa_union.__su_sigactionfor AIX. Additionally, the types of these two union members are declared as function pointers rather thansighandler_t(i.e.,usize), as is the case on other platforms. This discrepancy causes compiler errors whensighandler_tis used as the type for signal handlers. Other operating systems, such as Linux, also define a union forsa_handlerandsa_sigactioninstruct sigaction. However, thelibccrate implementations on these platforms definesa_sigactiondirectly as a member ofstruct sigactionrather than as part of a union. This patch modifies thelibccrate implementation for AIX to definesa_sigactionas a direct member ofstruct sigaction, aligning it with implementations for other similar platforms. We will also update affected crates and test cases to reflect this change.Sources
Checklist
libc-test/semverhave been updated*LASTor*MAXareincluded (see #3131)
cd libc-test && cargo test --target mytarget);especially relevant for platforms that may not be checked in CI