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

Switch to manual trait impls for sigevent #1394

Merged
merged 1 commit into from
Jun 11, 2019

Conversation

Susurrus
Copy link
Contributor

@Susurrus Susurrus commented Jun 9, 2019

sigevent on most platforms have padding or unused fields. Rather
than display those in the Debug impl by deriving it, manually implement
all extra_traits instead ignoring those fields.

I do worry that my PartialEq implementations for this for some platforms (like Linux) is not correct due to ignoring bytes that shouldn't be ignored because these structs don't have a proper union set up.

cc @asomers
Part of nix-rust/nix#1035

@rust-highfive
Copy link

r? @gnzlbg

(rust_highfive has picked a reviewer for you, use r? to override)

sigevent structs on most platforms have padding or unused fields. Rather
than display those in the Debug impl by deriving it, manually implement
all extra_traits instead ignoring those fields.
@gnzlbg
Copy link
Contributor

gnzlbg commented Jun 10, 2019

FWIW it might be that sigevent is broken anyways (there are few types that we skip testing everywhere), but sigevent, sig_t/sighandler_t, etc. are usually amongst them.

Looking at the type, does the function pointer field need to always be a non-null pointer ? If one can set that field to NULL in C, then this needs to be an Option in Rust, etc.

Copy link
Contributor

@asomers asomers left a comment

Choose a reason for hiding this comment

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

Apart from the NULL function pointer issue that @gnzlbg raised, this LGTM.

@Susurrus
Copy link
Contributor Author

@gnzlbg Are you referring to the sigev_notify_function field that's only exposed on Fuchsia? The function pointer fields for all other platforms are private members, so can't be called even if they're initialized properly. I can change all of these fields to be Option's if that's what you want, however then I think we need to use NonNull if we want to preserve the layout guarantees of these structs.

@gnzlbg
Copy link
Contributor

gnzlbg commented Jun 11, 2019

@bors: r+

@bors
Copy link
Contributor

bors commented Jun 11, 2019

📌 Commit 7c26591 has been approved by gnzlbg

@bors
Copy link
Contributor

bors commented Jun 11, 2019

⌛ Testing commit 7c26591 with merge 11c762c...

bors added a commit that referenced this pull request Jun 11, 2019
Switch to manual trait impls for sigevent

`sigevent` on most platforms have padding or unused fields. Rather
than display those in the `Debug` impl by deriving it, manually implement
all `extra_traits` instead ignoring those fields.

I do worry that my `PartialEq` implementations for this for some platforms (like Linux) is not correct due to ignoring bytes that shouldn't be ignored because these structs don't have a proper union set up.

cc @asomers
Part of nix-rust/nix#1035
@gnzlbg
Copy link
Contributor

gnzlbg commented Jun 11, 2019

Are you referring to the sigev_notify_function field that's only exposed on Fuchsia?

Not only, in other platforms there are fields with type *mut c_void that are actually function pointers.

I can change all of these fields to be Option's if that's what you want, however then I think we need to use NonNull if we want to preserve the layout guarantees of these structs.

Function pointers (fn()->()) in Rust are always NonNull. That is, initializing one to zero invokes undefined behavior instantaneously. An Option<fn()->()> has the same layout of a pointer, and Option::None represents the null pointer.


Another issue I am seeing is that these function pointers are not extern "C". So they should actually probably be of type Option<extern "C" unsafe fn(...)->...>.

@bors
Copy link
Contributor

bors commented Jun 11, 2019

☀️ Test successful - checks-cirrus-freebsd-11, checks-cirrus-freebsd-12, checks-travis, status-appveyor
Approved by: gnzlbg
Pushing 11c762c to master...

@bors bors merged commit 7c26591 into rust-lang:master Jun 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants