Skip to content
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

abis/mlibc: make struct sigaction and siginfo_t match linux ABI #594

Closed
wants to merge 2 commits into from

Conversation

64
Copy link
Member

@64 64 commented Jun 30, 2022

Fixes #257. Fixes #590. Obsoletes #463.

Copy link
Member

@Geertiebear Geertiebear left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM except for one comment.

abis/mlibc/signal.h Show resolved Hide resolved
Comment on lines +61 to +80
#define si_pid __si_fields.__si_common.__first.__piduid.si_pid
#define si_uid __si_fields.__si_common.__first.__piduid.si_uid
#define si_status __si_fields.__si_common.__second.__sigchld.si_status
#define si_utime __si_fields.__si_common.__second.__sigchld.si_utime
#define si_stime __si_fields.__si_common.__second.__sigchld.si_stime
#define si_value __si_fields.__si_common.__second.si_value
#define si_addr __si_fields.__sigfault.si_addr
#define si_addr_lsb __si_fields.__sigfault.si_addr_lsb
#define si_lower __si_fields.__sigfault.__first.__addr_bnd.si_lower
#define si_upper __si_fields.__sigfault.__first.__addr_bnd.si_upper
#define si_pkey __si_fields.__sigfault.__first.si_pkey
#define si_band __si_fields.__sigpoll.si_band
#define si_fd __si_fields.__sigpoll.si_fd
#define si_timerid __si_fields.__si_common.__first.__timer.si_timerid
#define si_overrun __si_fields.__si_common.__first.__timer.si_overrun
#define si_ptr si_value.sival_ptr
#define si_int si_value.sival_int
#define si_call_addr __si_fields.__sigsys.si_call_addr
#define si_syscall __si_fields.__sigsys.si_syscall
#define si_arch __si_fields.__sigsys.si_arch
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These defines are incredibly ugly. Can't we use anonymous unions and structs to avoid this mess?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anonymous unions and structs are a C11 feature, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then I don't see a particularly better way than using these defines.

@64 64 force-pushed the signal-abi branch 2 times, most recently from 1aed8a1 to fda6cd5 Compare July 5, 2022 19:18
@avdgrinten
Copy link
Member

Overall, I'm not convinced that this is a good change. All OSes that currently use this ABI will need to be changed (which OSes are affected?). What do they gain by doing that? Also, what do they gain over simply using abis/linux instead?

@64
Copy link
Member Author

64 commented Jul 5, 2022

Well currently our siginfo_t is missing a lot of members that userspace expects. I don't think there's a way to avoid that without kernel changes.

@64
Copy link
Member Author

64 commented Jul 5, 2022

I wouldn't be opposed to deleting this in favour of abis/linux either, tbh.

@avdgrinten
Copy link
Member

Well, it's one issue to change Managarm, but this also affects Ironclad. If only Managarm wants to change, we can easily switch Managarm to abis/linux w/o forcing others to update.

@64
Copy link
Member Author

64 commented Jul 5, 2022

That's true: if they want to change to use abis/linux (and why wouldn't they, given an ABI break will happen anyway?) then we ought to ping all the sysdeps maintainers to OK this.

@64 64 closed this Jul 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants