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

Support Android ndk versions r23-beta3 and up #85806

Merged
merged 3 commits into from Jun 4, 2021
Merged

Support Android ndk versions r23-beta3 and up #85806

merged 3 commits into from Jun 4, 2021

Conversation

ghost
Copy link

@ghost ghost commented May 29, 2021

Since android ndk version r23-beta3, libgcc has been replaced with libunwind. This moves the linking of libgcc/libunwind into the unwind crate where we check if the system compiler can find libunwind and fall back to libgcc if needed.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Mark-Simulacrum (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 29, 2021
@Mark-Simulacrum
Copy link
Member

@petrochenkov -- do you have a read on this PR perhaps?

It seems like this could break downstream users if they're relying on libgcc linked in (due to std asking for it), even though they sort of shouldn't be doing so. Ultimately seems ok, though, especially if upstream is going to drop it...

@petrochenkov
Copy link
Contributor

If this passes CI (which uses an old NDK?) then I'm happy with this.

We are linking compiler-builtins anyway so Rust code is covered, and if libgcc is required by libc then we'll know it from CI, I guess. That's certainly a possibility because a random old NDK that I have at hand (android-ndk-r17c-aarch64-21) does this

-lgcc -lc -ldl -lgcc

@Mark-Simulacrum
Copy link
Member

@bors r+ rollup=iffy

@bors
Copy link
Contributor

bors commented May 29, 2021

📌 Commit f077233ab84a97c71300cc37e94d1d1c7d584349 has been approved by Mark-Simulacrum

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 29, 2021
@bors
Copy link
Contributor

bors commented May 30, 2021

⌛ Testing commit f077233ab84a97c71300cc37e94d1d1c7d584349 with merge 144163b601605bc1e85ec5580115659ca3faf399...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented May 30, 2021

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 30, 2021
@ghost
Copy link
Author

ghost commented May 30, 2021

There seem to be two main issues:

1. __sync_* missing

These are normally defined by compiler-builtins but they are only exposed on target_os = "linux" and not android:
https://github.com/rust-lang/compiler-builtins/blob/master/src/lib.rs#L52-L53

This should be changed to include android as it also uses them.

Implemented here: rust-lang/compiler-builtins#423

2. Missing unwinder

libgcc included an unwinder which we are now missing. ndk r23 beta 3 now includes libunwind as a replacement. So we could check in the unwind build script if the C compiler can link with libunwind and fall back to libgcc otherwise.

Any other ideas are appreciated.

Tilmann Meyer added 2 commits June 1, 2021 21:32
Since android ndk version `r23-beta3`, `libgcc` has been replaced with
`libunwind`. This moves the linking of `libgcc`/`libunwind` into the
`unwind` crate where we check if the system compiler can find
`libunwind` and fall back to `libgcc` if needed.
@ghost ghost changed the title Do not link with libgcc on Android Support Android ndk versions r23-beta3 and up Jun 1, 2021
@ghost
Copy link
Author

ghost commented Jun 2, 2021

The mentioned issues should now be resolved as compiler-builtins now exposes the missing functions. And we now try to find the newer libunwind using the C compiler and fall back to libgcc if it cannot be found.

@petrochenkov
Copy link
Contributor

@ATiltedTree
To clarify, you are going to use this for custom builds of the standard library, right?
The NDK on CI is still old, and will always result in linking to libgcc in released Rust toolchains.

@bors r+

@bors
Copy link
Contributor

bors commented Jun 4, 2021

📌 Commit 965997b has been approved by petrochenkov

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 4, 2021
@ghost
Copy link
Author

ghost commented Jun 4, 2021

To clarify, you are going to use this for custom builds of the standard library, right?

Yes, I am going to use this for custom std builds.

The NDK on CI is still old, and will always result in linking to libgcc in released Rust toolchains.

And that is fine as long as I can do custom builds with newer toolchains.

@bors
Copy link
Contributor

bors commented Jun 4, 2021

⌛ Testing commit 965997b with merge 96acff98ad88f528eac1e23f2c1f6ce47f4b0f2b...

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Nov 7, 2022
…Simulacrum

Upgrade cc for working is_flag_supported on cross-compiles

rust-lang#85806 fixed unwind v.s gcc support on later Android ndks using `is_flag_supported`. However, due to rust-lang/cc-rs#675, this didn't work properly on cross-compiles. rust-lang/cc-rs@3eeb50b fixes this, and was released in cc 1.0.74, hence the upgrade
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Nov 7, 2022
…Simulacrum

Upgrade cc for working is_flag_supported on cross-compiles

rust-lang#85806 fixed unwind v.s gcc support on later Android ndks using `is_flag_supported`. However, due to rust-lang/cc-rs#675, this didn't work properly on cross-compiles. rust-lang/cc-rs@3eeb50b fixes this, and was released in cc 1.0.74, hence the upgrade
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Nov 8, 2022
…Simulacrum

Upgrade cc for working is_flag_supported on cross-compiles

rust-lang#85806 fixed unwind v.s gcc support on later Android ndks using `is_flag_supported`. However, due to rust-lang/cc-rs#675, this didn't work properly on cross-compiles. rust-lang/cc-rs@3eeb50b fixes this, and was released in cc 1.0.74, hence the upgrade
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Nov 8, 2022
…Simulacrum

Upgrade cc for working is_flag_supported on cross-compiles

rust-lang#85806 fixed unwind v.s gcc support on later Android ndks using `is_flag_supported`. However, due to rust-lang/cc-rs#675, this didn't work properly on cross-compiles. rust-lang/cc-rs@3eeb50b fixes this, and was released in cc 1.0.74, hence the upgrade
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Nov 8, 2022
…Simulacrum

Upgrade cc for working is_flag_supported on cross-compiles

rust-lang#85806 fixed unwind v.s gcc support on later Android ndks using `is_flag_supported`. However, due to rust-lang/cc-rs#675, this didn't work properly on cross-compiles. rust-lang/cc-rs@3eeb50b fixes this, and was released in cc 1.0.74, hence the upgrade
bczhc added a commit to bczhc/some-tools that referenced this pull request Nov 10, 2022
change Rust toolchain version to the latest nightly,
because the "-lunwind" problem has gone after upgrading
the latest Android NDK

The Rust team has made some changes to support new versions
of Android NDK, so if I still use an old NDK, "-lunwind"
problem appears.

- rust-lang/rust#85806
Manishearth added a commit to Manishearth/rust that referenced this pull request Nov 14, 2022
…Simulacrum

Upgrade cc for working is_flag_supported on cross-compiles

rust-lang#85806 fixed unwind v.s gcc support on later Android ndks using `is_flag_supported`. However, due to rust-lang/cc-rs#675, this didn't work properly on cross-compiles. rust-lang/cc-rs@3eeb50b fixes this, and was released in cc 1.0.74, hence the upgrade
@phatblat
Copy link

Confirmed that @ssrlive's dummy libgcc.a workaround #85806 (comment) works with both NDK versions 23.1.7779620 and 25.1.8937393 with rust version 1.66.1 android targets.

Here's a one-liner script to drop these libgcc.a files next to every libunwind.a.

find "${ndk_dir}" -name "libunwind.a" \
    -execdir bash -c 'echo "INPUT(-lunwind)" > libgcc.a' \;

ndk_dir can be a specific NDK version dir (e.g. ~/Library/Android/sdk/ndk/25.1.8937393) or just ~ to get them all.

@cjdelisle
Copy link

In case you are using busybox find (e.g. alpine linux), this should work:

find "${ndk_dir}" -name 'libunwind.a' | \
  sed 's@libunwind.a$@libgcc.a@' | \
  while read x; do
    echo "INPUT(-lunwind)" > $x
  done

KarimHamidou added a commit to KarimHamidou/godot-rust that referenced this pull request Feb 6, 2023
920: Update Android NDK (21 -> 25), workaround Rust bug r=Bromeon a=Bromeon

Newer NDKs don't work out of the box due to a Rust bug (only fixed in nightly).

See rust-lang/rust#85806 and workaround c4dt/lightarti-rest#104.

Co-authored-by: Jan Haller <bromeon@gmail.com>
GuilhermeOrceziae added a commit to GuilhermeOrceziae/godot-rust that referenced this pull request Feb 9, 2023
920: Update Android NDK (21 -> 25), workaround Rust bug r=Bromeon a=Bromeon

Newer NDKs don't work out of the box due to a Rust bug (only fixed in nightly).

See rust-lang/rust#85806 and workaround c4dt/lightarti-rest#104.

Co-authored-by: Jan Haller <bromeon@gmail.com>
thomcc pushed a commit to tcdi/postgrestd that referenced this pull request Feb 10, 2023
Upgrade cc for working is_flag_supported on cross-compiles

rust-lang/rust#85806 fixed unwind v.s gcc support on later Android ndks using `is_flag_supported`. However, due to rust-lang/cc-rs#675, this didn't work properly on cross-compiles. rust-lang/cc-rs@3eeb50b fixes this, and was released in cc 1.0.74, hence the upgrade
hesuteia added a commit to hesuteia/godot-rust that referenced this pull request Feb 11, 2023
920: Update Android NDK (21 -> 25), workaround Rust bug r=Bromeon a=Bromeon

Newer NDKs don't work out of the box due to a Rust bug (only fixed in nightly).

See rust-lang/rust#85806 and workaround c4dt/lightarti-rest#104.

Co-authored-by: Jan Haller <bromeon@gmail.com>
mberry added a commit to Argyle-Software/kyber that referenced this pull request Mar 16, 2023
Failing due to not locating libunwind
This is related to: cross-rs/cross#1128
There are workarounds here: rust-lang/rust#85806 (comment)
Someone might want to try implement it later
For now I'm pulling the android compiles
mberry added a commit to Argyle-Software/kyber that referenced this pull request Mar 16, 2023
* v0.5.0
Update: changelog
Update: wasm binary and files
Bump: deps
Add: release on version tag github action
Add: release checklist

* Remove: Android CI target
Failing due to not finding libunwind
This is related to: cross-rs/cross#1128
There is a workaround: rust-lang/rust#85806 (comment)
Someone might want to try implement it later
For now I'm pulling out the android compiles from CI
ecobiubiu added a commit to ecobiubiu/open-rust that referenced this pull request Mar 30, 2023
920: Update Android NDK (21 -> 25), workaround Rust bug r=Bromeon a=Bromeon

Newer NDKs don't work out of the box due to a Rust bug (only fixed in nightly).

See rust-lang/rust#85806 and workaround c4dt/lightarti-rest#104.

Co-authored-by: Jan Haller <bromeon@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.