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.

Sign up for GitHub

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

Jump to bottom

Use Rust union types #2056

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Use Rust union types #2056

wants to merge 4 commits into from

Conversation

kellda
Copy link
Contributor

@kellda kellda commented Feb 4, 2021

For #1020

Replace some types with unions now that rustc supports them.

I also found sigevent.sigev_notify_thread_id, siginfo_t.__data and sigaction.sa_sigaction that I think should be unions, but I wasn't sure.

@rust-highfive
Copy link

r? @Amanieu

(rust-highfive has picked a reviewer for you, use r? to override)

@kellda
Copy link
Contributor Author

kellda commented Feb 4, 2021

Also I had to ignore iffaddrs.ifa_ifu on Linux to pass CI

@Amanieu
Copy link
Member

Amanieu commented Feb 4, 2021

Unfortunately I believe this is a breaking change, which means we can't actually do this until we bump the crate version to 1.0.

cc @JohnTitor

@kellda
Copy link
Contributor Author

kellda commented Feb 4, 2021

I'm aware that it is a breaking change and that's not a problem for me to wait until a new major version for this to be merged.

@bors
Copy link
Contributor

bors commented Feb 13, 2021

☔ The latest upstream changes (presumably #1975) made this pull request unmergeable. Please resolve the merge conflicts.

@tgross35
Copy link
Contributor

tgross35 commented Aug 29, 2024 •
edited
Loading

@kellda main is now intended for libc 1.0. Are you able to rebase this?

@rustbot author

@tgross35 tgross35 added this to the 1.0 milestone Aug 29, 2024
@rustbot
Copy link
Collaborator

rustbot commented Aug 29, 2024

Some changes occurred in solarish module

cc @jclulow, @pfmooney

@kellda
Copy link
Contributor Author

kellda commented Aug 29, 2024

I'm unsure how to find out what's wrong with sigval on emscripten...

@kellda
Copy link
Contributor Author

kellda commented Aug 29, 2024

Btw, I noted (when I originally opened this PR) that the following fields may also be wroth considering turning into unions:

  • sigevent.sigev_notify_thread_id
  • siginfo_t.{_pad/__data}
  • sigaction.sa_sigaction

@tgross35
Copy link
Contributor

tgross35 commented Oct 1, 2024

I'm unsure how to find out what's wrong with sigval on emscripten...

I also don't know. I'll mark this as review as a reminder that I need to take a look at this.

@rustbot review

Btw, I noted (when I originally opened this PR) that the following fields may also be wroth considering turning into unions:

* sigevent.sigev_notify_thread_id

* siginfo_t.{_pad/__data}

* sigaction.sa_sigaction

Feel free to submit another PR!

@tgross35
Copy link
Contributor

This needs another rebase, feel free to just leave emscripten disabled in build.rs as a FIXME if needed.

Regarding traits: don't add Hash/PartialEq/Eq for unions, or leave them but make the bodies unimplemented!("traits") if some other structs depend on them. We are likely going to drop these in 1.0 so we aren't doing any unsafe union field access.

For Debug, make the unions fmt.debug_struct("type_name").finish_non_exhaustive().

@rustbot author

@rustbot rustbot added the O-unix label Nov 21, 2024
tgross35
tgross35 approved these changes Dec 7, 2024
View reviewed changes
src/unix/linux_like/linux/gnu/b64/s390x.rs
@@ -212,33 +212,17 @@ s! {
}

s_no_extra_traits! {
// FIXME: This is actually a union.
pub struct fpreg_t {
pub union fpreg_t {
Copy link
Contributor

Choose a reason for hiding this comment

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

https://github.com/bminor/glibc/blob/77c7c4417410f4bf15ea246dd52603dc7be8de45/sysdeps/unix/sysv/linux/s390/sys/ucontext.h#L59-L63

src/unix/linux_like/mod.rs
@@ -159,7 +159,7 @@ s! {
pub ifa_flags: ::c_uint,
pub ifa_addr: *mut ::sockaddr,
pub ifa_netmask: *mut ::sockaddr,
pub ifa_ifu: *mut ::sockaddr, // FIXME This should be a union
pub ifa_ifu: __c_anonymous_ifa_ifu,
Copy link
Contributor

Choose a reason for hiding this comment

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

https://github.com/bminor/glibc/blob/77c7c4417410f4bf15ea246dd52603dc7be8de45/inet/ifaddrs.h#L29 / https://github.com/kraj/musl/blob/eb4309b142bb7a8bdc839ef1faf18811b9ff31c8/include/ifaddrs.h#L12

src/unix/linux_like/mod.rs
pub data: epoll_data,
}

pub union epoll_data {
Copy link
Contributor

Choose a reason for hiding this comment

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

https://github.com/bminor/glibc/blob/77c7c4417410f4bf15ea246dd52603dc7be8de45/sysdeps/unix/sysv/linux/sys/epoll.h#L77-L89 / https://github.com/kraj/musl/blob/eb4309b142bb7a8bdc839ef1faf18811b9ff31c8/include/sys/epoll.h#L42-L56

src/unix/mod.rs
@@ -213,6 +208,23 @@ s! {
}
}

s_no_extra_traits! {
pub union sigval {
Copy link
Contributor

Choose a reason for hiding this comment

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

https://github.com/torvalds/linux/blob/b5f217084ab3ddd4bdd03cd437f8e3b7e2d1f5b6/include/uapi/asm-generic/siginfo.h#L8-L11

src/fuchsia/mod.rs
@@ -1046,6 +1048,16 @@ s_no_extra_traits! {
pub struct pthread_cond_t {
size: [u8; ::__SIZEOF_PTHREAD_COND_T],
}

pub union sigval {
Copy link
Contributor

Choose a reason for hiding this comment

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

https://cs.opensource.google/fuchsia/fuchsia/+/main:third_party/llvm-libc/src/include/llvm-libc-types/union_sigval.h;l=12;drc=b47f6213560e67982d8b9d01a78daeb6ec361a5b

src/fuchsia/mod.rs
@@ -470,7 +472,7 @@ s! {
pub ifa_flags: ::c_uint,
pub ifa_addr: *mut ::sockaddr,
pub ifa_netmask: *mut ::sockaddr,
pub ifa_ifu: *mut ::sockaddr, // FIXME This should be a union
pub ifa_ifu: __c_anonymous_ifa_ifu,
Copy link
Contributor

Choose a reason for hiding this comment

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

https://cs.opensource.google/fuchsia/fuchsia/+/main:zircon/third_party/ulib/musl/include/ifaddrs.h;l=12;drc=7c5e521391fddb98fd8f6970da7c410899ddf5cf

src/fuchsia/mod.rs
pub data: epoll_data,
}

pub union epoll_data {
Copy link
Contributor

Choose a reason for hiding this comment

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

https://cs.opensource.google/fuchsia/fuchsia/+/main:third_party/llvm-libc/src/include/llvm-libc-types/struct_epoll_data.h;l=12;drc=b47f6213560e67982d8b9d01a78daeb6ec361a5b, https://cs.opensource.google/fuchsia/fuchsia/+/main:third_party/android/platform/bionic/libc/include/bits/epoll_event.h;l=48;drc=dd6763a6c677687ad06727af037ed9a7b018c773

@tgross35
Copy link
Contributor

tgross35 commented Dec 7, 2024

These changes overall look good to me assuming the traits get figured out. I left comments with sources just to make sure I double checked everything here.

#4176 landed so you can remove all debug implementations. If anything else complains about missing traits, just implement the body unimplemented!("traits") so we fix this up later.

Comment @rustbot ready when this is ready for another look.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tgross35 tgross35 tgross35 approved these changes

Assignees

@Amanieu Amanieu

Labels
breakage-candidate O-dragonfly O-gnu O-linux O-linux-like O-macos O-solarish O-unix S-waiting-on-author
Projects
None yet
Milestone
1.0
Development

Successfully merging this pull request may close these issues.
7 participants
@kellda @rust-highfive @Amanieu @bors @tgross35 @rustbot @JohnTitor