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

std: Fix linking against signal on Android #32415

Merged
merged 1 commit into from
Apr 5, 2016

Conversation

alexcrichton
Copy link
Member

Currently the minimum supported Android version of the standard library is
API level 18 (android-18). Back in those days 1 the signal function was
just an inline wrapper around bsd_signal, but starting in API level
android-20 the signal symbols was introduced 2. Finally, in android-21
the API bsd_signal was removed 3.

Basically this means that if we want to be binary compatible with multiple
Android releases (oldest being 18 and newest being 21) then we need to check
for both symbols and not actually link against either.

This was first discovered in rust-lang/libc#236 with a fix proposed in
rust-lang/libc#237. I suspect that we'll want to accept rust-lang/libc#237 so
Rust crates at large continue to be compatible with newer releases of Android
and crates, like the standard library, that want to opt into older support can
continue to do so via similar means.

Closes rust-lang/libc#236

@rust-highfive
Copy link
Collaborator

r? @brson

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

@bors
Copy link
Contributor

bors commented Mar 23, 2016

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

@aturon
Copy link
Member

aturon commented Apr 1, 2016

@bors: r+

@bors
Copy link
Contributor

bors commented Apr 1, 2016

📌 Commit c5b9b24 has been approved by aturon

@bors
Copy link
Contributor

bors commented Apr 2, 2016

⌛ Testing commit c5b9b24 with merge b60f122...

@bors
Copy link
Contributor

bors commented Apr 2, 2016

💔 Test failed - auto-linux-64-x-android-t

@alexcrichton
Copy link
Member Author

Looks like weak! isn't working on Android, both symbols are returning None and then we're panicking on startup.

Gee I wonder why weak! isn't working? Could it have something to do with dlopen(NULL, RTLD_LAZY)? Could it have perhaps been previously reported even?!

Ok, so that strategy doesn't work on Android, but apparently RTLD_DEFAULT does as a direct argument to dlsym, so just gotta add it to libc

Currently the minimum supported Android version of the standard library is
API level 18 (android-18). Back in those days [1] the `signal` function was
just an inline wrapper around `bsd_signal`, but starting in API level
android-20 the `signal` symbols was introduced [2]. Finally, in android-21
the API `bsd_signal` was removed [3].

Basically this means that if we want to be binary compatible with multiple
Android releases (oldest being 18 and newest being 21) then we need to check
for both symbols and not actually link against either.

This was first discovered in rust-lang/libc#236 with a fix proposed in
rust-lang/libc#237. I suspect that we'll want to accept rust-lang/libc#237 so
Rust crates at large continue to be compatible with newer releases of Android
and crates, like the standard library, that want to opt into older support can
continue to do so via similar means.

Closes rust-lang/libc#236

[1]: https://chromium.googlesource.com/android_tools/+/20ee6d20/ndk/platforms/android-18/arch-arm/usr/include/signal.h
[2]: https://chromium.googlesource.com/android_tools/+/fbd420/ndk_experimental/platforms/android-20/arch-arm/usr/include/signal.h
[3]: https://chromium.googlesource.com/android_tools/+/20ee6d/ndk/platforms/android-21/arch-arm/usr/include/signal.h
@alexcrichton
Copy link
Member Author

@bors: r+ 9c462b8

@bors
Copy link
Contributor

bors commented Apr 5, 2016

⌛ Testing commit 9c462b8 with merge 69d3a19...

@bors
Copy link
Contributor

bors commented Apr 5, 2016

💔 Test failed - auto-linux-64-x-netbsd

@alexcrichton
Copy link
Member Author

Hm this may end up being difficult to land and have ramifications on the rest of the Rust ecosystem. This updates the libc submodule to include changes in rust-lang/rfcs#1529 which is starting to link a new library by default. I'm wary of this causing breakage on bots beyond ours, so we should keep our eyes peeled and be ready to revert if need be.

@alexcrichton
Copy link
Member Author

@bors: retry

On Tue, Apr 5, 2016 at 3:27 AM, bors [email protected] wrote:

[image: 💔] Test failed - auto-linux-64-x-netbsd
http://buildbot.rust-lang.org/builders/auto-linux-64-x-netbsd/builds/170


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#32415 (comment)

@bors
Copy link
Contributor

bors commented Apr 5, 2016

⌛ Testing commit 9c462b8 with merge 241a9d0...

bors added a commit that referenced this pull request Apr 5, 2016
std: Fix linking against `signal` on Android

Currently the minimum supported Android version of the standard library is
API level 18 (android-18). Back in those days [1] the `signal` function was
just an inline wrapper around `bsd_signal`, but starting in API level
android-20 the `signal` symbols was introduced [2]. Finally, in android-21
the API `bsd_signal` was removed [3].

Basically this means that if we want to be binary compatible with multiple
Android releases (oldest being 18 and newest being 21) then we need to check
for both symbols and not actually link against either.

This was first discovered in rust-lang/libc#236 with a fix proposed in
rust-lang/libc#237. I suspect that we'll want to accept rust-lang/libc#237 so
Rust crates at large continue to be compatible with newer releases of Android
and crates, like the standard library, that want to opt into older support can
continue to do so via similar means.

Closes rust-lang/libc#236

[1]: https://chromium.googlesource.com/android_tools/+/20ee6d20/ndk/platforms/android-18/arch-arm/usr/include/signal.h
[2]: https://chromium.googlesource.com/android_tools/+/fbd420/ndk_experimental/platforms/android-20/arch-arm/usr/include/signal.h
[3]: https://chromium.googlesource.com/android_tools/+/20ee6d/ndk/platforms/android-21/arch-arm/usr/include/signal.h
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.

Android linking fails with NDK r11b: undefined reference to 'bsd_signal'
5 participants