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

Always link to libgcc for musl targets. #986

Closed
wants to merge 1 commit into from

Conversation

Alexhuszagh
Copy link
Contributor

@Alexhuszagh Alexhuszagh commented Jul 29, 2022

Fixes issues with numerous missing symbols from libgcc routines that are
not currently implemented in Rust's compiler builtins. These include:

  • __trunctfsf2
  • __ctzdi2
  • __letf2
  • __addtf3

These missing symbols all seem to be soft-float and integer arithmetic routines. These missing symbols all seem to be soft-float and integer arithmetic routines. This is a temporary workaround until these are all implemented in compiler-builtins.

Closes #367.
Closes #985.

@Alexhuszagh Alexhuszagh added upstream A-musl Area: musl libc targets labels Jul 29, 2022
@Alexhuszagh Alexhuszagh requested a review from a team as a code owner July 29, 2022 19:49
@Alexhuszagh

This comment was marked as outdated.

bors bot added a commit that referenced this pull request Jul 29, 2022
@Alexhuszagh

This comment was marked as outdated.

bors bot added a commit that referenced this pull request Jul 29, 2022
@bors

This comment was marked as outdated.

@Alexhuszagh
Copy link
Contributor Author

bors try --target mips64-unknown-linux-muslabi64

bors bot added a commit that referenced this pull request Jul 29, 2022
@bors
Copy link
Contributor

bors bot commented Jul 29, 2022

try

Build succeeded:

@Alexhuszagh Alexhuszagh force-pushed the musl_linker branch 2 times, most recently from 21d663d to 792618c Compare July 29, 2022 20:36
@Alexhuszagh
Copy link
Contributor Author

Alexhuszagh commented Jul 29, 2022

It might be worth conditionally disabling linking to libgcc, since it might obscure missing builtins that need to be patched in compiler-builtins. I really don't like this solution even though I think it's probably necessary in some ways: we can't track every builtin that's a dependency, and it's likely certain targets are missing more obscure builtins even as more common ones are implemented: aarch64-unknown-linux-musl was mostly implemented in Rust 1.48, but we've still had issues with more obscure symbols in more recent versions.

Since we're somewhat of a canary in the coal mine due to our support of many tier 2 and some tier 3 targets, it's probably a good idea to allow people to opt-in to use libgcc for a musl target as a fallback (we always link to libgcc last, so it has the lowest priority).

Fixes issues with numerous missing symbols from libgcc routines that are
not currently implemented in Rust's compiler builtins. These include:
- __trunctfsf2
- __ctzdi2
- __letf2
- __addtf3

These missing symbols all seem to be soft-float and integer arithmetic routines. This is a temporary workaround until these are all implemented in compiler-builtins.
@Alexhuszagh Alexhuszagh marked this pull request as draft July 30, 2022 06:45
@NobodyXu
Copy link

@Alexhuszagh
Copy link
Contributor Author

Alexhuszagh commented Jul 30, 2022

@Alexhuszagh It still failed https://github.com/NobodyXu/cargo-binstall/runs/7589919126?check_suite_focus=true

Right because although you've configured the proper cross version, you haven't built the correct image and are using one from the latest main, so it's still using the wrong linker. I can provide a temporary link on Dockerhub for testing (not an official image, and it will likely be removed in a few days) that you can then provide via CROSS_TARGET_ARMV7_UNKNOWN_LINUX_MUSLEABIHF_IMAGE, which will then override the default image. Or, provide it in Cross.toml:

[target.armv7-unknown-linux-musleabihf]
image = "..."

Sorry, I might not have been clear that you have to build the image to test the changes.

@NobodyXu
Copy link

Sorry, I might not have been clear that you have to build the image to test the changes.

Oh that's right, I saw you change the Dockerfiles but since this PR is not merged it, it is not on the ghcr.

@Alexhuszagh Can you provide a temporary image on dockerhub? Thanks!

@Alexhuszagh
Copy link
Contributor Author

Alexhuszagh commented Jul 30, 2022

Actually, I think the better issue would be to document how to use build-std or provide -lgcc as a flag when it's used to RUSTFLAGS for any musl or GNU target when the build-std cfg option is provided. Numerous other targets require external C builtins for build-std (such as aarch64-unknown-linux-gnu), and porting every builtin is a monumental effort. Nor should we obscure potential errors in toolchains due to missing intrinsics for regular builds (IE, without using build-std).

So, CROSS_BUILD_STD and the TOML variant should likely set -lgcc if applicable, we should fix #367 with a GCC linker as well, and we should update the linkers provided in #367 and #974 to no longer link to libgcc when both those patches made to compiler-builtins reaches std.

I'll still provide the images, but I'm almost certain there's a better way.

@Alexhuszagh
Copy link
Contributor Author

Alexhuszagh commented Jul 30, 2022

@NobodyXu The images for the two targets are provided here. I'll likely update the documentation for how to use build-std when external C dependencies are required tomorrow.

Update: Sorry, link fixed.

@NobodyXu
Copy link

@Alexhuszagh That fixed the issue https://github.com/NobodyXu/cargo-binstall/actions/runs/2765292275 , thanks, though I cannot merge that PR.

Can using zig-cc image fixes this?
And what will be the final solution in cross 0.3.0?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-musl Area: musl libc targets upstream
Projects
None yet
2 participants