Skip to content

Add signalfd and related to android #671

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
Merged
merged 1 commit into from
Jul 20, 2017
Merged

Add signalfd and related to android #671

merged 1 commit into from
Jul 20, 2017

Conversation

Susurrus
Copy link
Contributor

No description provided.

@Susurrus Susurrus mentioned this pull request Jul 17, 2017
Merged
Susurrus added a commit to Susurrus/nix that referenced this pull request Jul 17, 2017
@Susurrus
Remove signalfd feature in favor of conditional compilation
8ac69fc 
Note that this is now only available for Linux as support is missing in libc
for Android (see rust-lang/libc#671).
@alexcrichton
Copy link
Member

Looks like CI is failing?

@Susurrus
Copy link
Contributor Author

@alexcrichton Yes, 64-bit android builds are failing because the 64-bit fields within signalfd_siginfo are the wrong size I guess. This doesn't jive with what I'm seeing in the source headers anywhere, however, so I'm trying to figure out what's going on here. I'll come back to this in the coming days and see if I can sort it out.

Susurrus added a commit to Susurrus/nix that referenced this pull request Jul 17, 2017
@Susurrus
Remove signalfd feature in favor of conditional compilation
b3b221e 
Note that this is now only available for Linux as support is missing in libc
for Android (see rust-lang/libc#671).
Susurrus added a commit to Susurrus/nix that referenced this pull request Jul 18, 2017
@Susurrus
Remove signalfd feature in favor of conditional compilation
e8ef17f 
Note that this is now only available for Linux as support is missing in libc
for Android (see rust-lang/libc#671).

As part of this work the SIGUSR2 signal mutex was altered to be a general
signal mutex. This is because all signal handling is shared across all threads
in the Rust test harness, so if you alter one signal, depending on whether it's
additive or may overwrite the mask for other signals, it could break the other
ones. Instead of putting this on the user, just broaden the scope of the mutex
so that any altering of signal handling needs to use it.
Susurrus added a commit to Susurrus/nix that referenced this pull request Jul 18, 2017
@Susurrus
Remove signalfd feature in favor of conditional compilation
74ea3c7 
Note that this is now only available for Linux as support is missing in libc
for Android (see rust-lang/libc#671).

As part of this work the SIGUSR2 signal mutex was altered to be a general
signal mutex. This is because all signal handling is shared across all threads
in the Rust test harness, so if you alter one signal, depending on whether it's
additive or may overwrite the mask for other signals, it could break the other
ones. Instead of putting this on the user, just broaden the scope of the mutex
so that any altering of signal handling needs to use it.
@Susurrus
Copy link
Contributor Author

@ndusart You updated the Android NDK version recently, so I'm wondering if you have any familiarity with Android APIs here. As far as I can see when searching the same headers I find that signalfd_siginfo field is defined thesame for all architectures. I even checked the __u64 is the same on all platforms, as it is. Is there something I'm missing about 64-bit versus 32-bit Android platforms?

@ndusart
Copy link
Contributor

ndusart commented Jul 20, 2017 •
edited
Loading

The problem here is that __u64 is the same type on each platforms but uint64_t is not the same type on 32 or 64bits.

__u64 is a typedef to unsigned long long (defined in asm-generic/int-ll64.h, included from asm-generic/types.h)

and uint64_t is only unsigned long long on 32-bits architectures, not 64-bits where it is unsigned long (see stdint.h)

you should use ::c_ulonglong here

@Susurrus
Copy link
Contributor Author

So uint64_t will be 64-bits on all platforms while __u64 will vary? Ugh that's some confusing naming. I figured that had to be the case but couldn't believe it given their naming.

@alexcrichton This brings up a good point though with typedefs, what should I be using for datatypes here? __u64 and family is defined on Linux, but not android. Should I be trying to use the exact same types with all the typedef facade (define __u64 and friends and use them), should I use equivalent types (c_ulonglong instead of __u64), or should I use explicit types (u32 and u64) depending on the platform?

@Susurrus
Copy link
Contributor Author

BTW @ndusart Thanks for looking at this!

@ndusart
Copy link
Contributor

ndusart commented Jul 20, 2017 •
edited
Loading

Explicit types will not work either as ctest translate them to the stdint types, so you'll have the same behavior as using uint64_t.

I didn't look if __u64 was defined in this crate, It's probably better to define __u64 in android as well then.

So uint64_t will be 64-bits on all platforms while __u64 will vary?

No, __u64 should be 64-bits on all platforms. It's just that one decided to use unsigned long long on all platforms while the other chose to use unsigned long on 64 bits. These are different C types.

@alexcrichton
Copy link
Member

@Susurrus I'd only require a type with the same size/align/sign, other than that I don't think it's necessary to strive to have the same exact type as C, there's no way that we're "correct" in all other structs.

@Susurrus
Copy link
Contributor Author

No, __u64 should be 64-bits on all platforms. It's just that one decided to use unsigned long long on all platforms while the other chose to use unsigned long on 64 bits. These are different C types.

Ahh, I think I got it! While the bit-width is the same the types are different and the compiler complains. Makes sense.

@Susurrus I'd only require a type with the same size/align/sign, other than that I don't think it's necessary to strive to have the same exact type as C, there's no way that we're "correct" in all other structs.

Alright, I did it the minimally-invasive way, using c_ulonglong directly instead of defining the __u64 type. Looks ugly, but it should be correct.

@alexcrichton
Copy link
Member

Looks like CI is still red?

@Susurrus
Copy link
Contributor Author

@alexcrichton Fixed.

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Jul 20, 2017

📌 Commit 83ab9a3 has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Jul 20, 2017

⌛ Testing commit 83ab9a3 with merge 516df7f...

bors added a commit that referenced this pull request Jul 20, 2017
@bors
Auto merge of #671 - Susurrus:signalfd_android, r=alexcrichton
516df7f 
Add signalfd and related to android
@bors
Copy link
Contributor

bors commented Jul 20, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 516df7f to master...

@bors bors merged commit 83ab9a3 into rust-lang:master Jul 20, 2017
@Susurrus Susurrus deleted the signalfd_android branch July 20, 2017 19:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Susurrus @alexcrichton @ndusart @bors