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

Restrict aarch64 outline atomics to glibc for now. #90044

Merged

Conversation

hkratz
Copy link
Contributor

@hkratz hkratz commented Oct 19, 2021

The introduced dependency on getauxval causes linking problems with musl, making compiling any binaries for aarch64-unknown-linux-musl impossible without workarounds such as using lld or adding liblibc.rlib again to the linker invocation, see #89626.

This is a workaround until libc>0.2.108 is merged.

@rust-highfive
Copy link
Collaborator

r? @oli-obk

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 19, 2021
@oli-obk
Copy link
Contributor

oli-obk commented Oct 19, 2021

r? @workingjubilee

@jbg
Copy link

jbg commented Oct 19, 2021

I may not understand how the features argument is interpreted, but shouldn't this be before the -C target-feature processing on line 417? It should be defaulting outline-atomics to on, but right now it seems to be forcing it to on, so -C target-feature=-outline-atomics doesn't do anything.

For example, right now you can't get a successful build on aarch64-unknown-linux-musl by setting RUSTFLAGS="-C target-feature=-outline-atomics". Presumably even after this patch, a glibc user can't disable the outline-atomics target feature if they want to.

@hkratz
Copy link
Contributor Author

hkratz commented Oct 19, 2021

Allowing -C target-feature=-outline-atomics to override this setting would definitely be an improvement, but it would not work because the std library would still be compiled with it enabled.

But I can move the outline atomics block before -C target-feature processing code as well (later feature specs override earlier ones).

@apiraino apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 28, 2021
@hkratz
Copy link
Contributor Author

hkratz commented Nov 4, 2021

If an updated libc with rust-lang/libc#2272 fixes it, then this is no longer needed. Let's wait for that.

@rustbot label -S-waiting-on-review +S-waiting-on-author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 4, 2021
@jbg
Copy link

jbg commented Nov 4, 2021

This issue does not only occur with a libc dependency in the tree. Even a no-deps Hello World program triggers it.

@hkratz
Copy link
Contributor Author

hkratz commented Nov 4, 2021

This issue does not only occur with a libc dependency in the tree. Even a no-deps Hello World program triggers it.

Yes, but the std library uses rust-lang/libc. Having an unbundled libc.a at the right place in link order will resolve this problem as well.

@workingjubilee
Copy link
Member

😩 Sorry for missing this. Let me see...

@workingjubilee
Copy link
Member

So it looks like we accidentally broke musl on aarch64 because we aren't compiling musl in a way that is convenient for musl (already a known factor, really, but I didn't expect it to cause an issue here), and:

  • we may find out that a recent libc crate update fixes it when it lands
  • but we should try to get this resolved before it hits stable
  • and another PR against rust-lang/rust offers a known permanent fix Eventually

so this one should wait a little for that but not a long time, correct? Alright, I will keep this on my radar.

@workingjubilee
Copy link
Member

Speaking of radar, it looks like we bumped LLVM to min 12. Accordingly, #90623 was opened, which remedies the FIXME and also will pose a merge conflict with this PR.

I would stop the rollup, but currently we seem to be inclined to let things ride for a few nights and hope a new libc fixes it, so might as well let it ride. It also addresses the second commit in this set.

@bors
Copy link
Contributor

bors commented Nov 6, 2021

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

@hkratz
Copy link
Contributor Author

hkratz commented Nov 6, 2021

If the libc update does not fix it, I will update this PR to just remove the +outline-atomics feature from the aarch64_unknown_linux_musl target spec.

@workingjubilee workingjubilee reopened this Nov 8, 2021
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Nov 8, 2021
Changelog:
https://github.com/rust-lang/libc/releases/tag/0.2.107
Primarily intended to pull in fd331f65f214ea75b6210b415b5fd8650be15c73
This should help with rust-lang#90044
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 9, 2021
The introduced dependency on `getauxval`causes linking
problems with musl, see rust-lang#89626.
@workingjubilee
Copy link
Member

I find this state of affairs baffling but I am going to approve this commit with a note here that it can't actually be backported in as-is form, so that this issue doesn't persist into the next beta cut.
@bors r+ rollup=always

@bors
Copy link
Contributor

bors commented Nov 24, 2021

📌 Commit bd287fa has been approved by workingjubilee

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 24, 2021
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Nov 24, 2021
…mics_for_musl, r=workingjubilee

Restrict aarch64 outline atomics to glibc for now.

The introduced dependency on `getauxval` causes linking problems with musl, making compiling any binaries for `aarch64-unknown-linux-musl` impossible without workarounds such as using lld or adding liblibc.rlib again to the linker invocation, see rust-lang#89626.

This is a workaround until libc>0.2.108 is merged.
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Nov 24, 2021
…mics_for_musl, r=workingjubilee

Restrict aarch64 outline atomics to glibc for now.

The introduced dependency on `getauxval` causes linking problems with musl, making compiling any binaries for `aarch64-unknown-linux-musl` impossible without workarounds such as using lld or adding liblibc.rlib again to the linker invocation, see rust-lang#89626.

This is a workaround until libc>0.2.108 is merged.
This was referenced Nov 24, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 24, 2021
…laumeGomez

Rollup of 7 pull requests

Successful merges:

 - rust-lang#89542 (Partially stabilize `duration_consts_2`)
 - rust-lang#90044 (Restrict aarch64 outline atomics to glibc for now.)
 - rust-lang#90420 (Create rustdoc_internals feature gate)
 - rust-lang#91075 (Reduce prominence of item-infos)
 - rust-lang#91151 (Fix test in std::process on android)
 - rust-lang#91179 (Fix more <a> color)
 - rust-lang#91199 (rustdoc: Add test for mixing doc comments and attrs)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit cbe563a into rust-lang:master Nov 25, 2021
@rustbot rustbot added this to the 1.58.0 milestone Nov 25, 2021
hkratz added a commit to rusticstuff/rust that referenced this pull request Nov 25, 2021
This is a functionally equivalent, minimally invasive backport of rust-lang#90044.

backport-of: nothing
@hkratz
Copy link
Contributor Author

hkratz commented Nov 25, 2021

Though, speaking of host tools guarantees: How did the original regression land if this issue blocks even linking a simple "Hello, world!"?

The host tools are linked against shared musl in CI, which works fine. This is only an issue with +crt-static, which is the default for the aarch64-unknown-linux-musl target.

@hkratz hkratz deleted the disable_arm_outline_atomics_for_musl branch November 25, 2021 19:54
@workingjubilee
Copy link
Member

Oh, then the severity of this is somewhat reduced, as musl-based distros patch rustc's musl targets to default to -crt-static (a patch we will eventually take up, and to some degree this hammers home why we should).

workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Nov 28, 2021
Mark-Simulacrum pushed a commit to Mark-Simulacrum/rust that referenced this pull request Nov 29, 2021
This is a functionally equivalent, minimally invasive backport of rust-lang#90044, which
fixes the problem that compiling any binary with the target
aarch64-unknown-linux-musl fails unless lld is used for linking (rust-lang#89626).

I have tested this backport by building aarch64-unknown-linux-gnu, installing
the std libraries for the -musl and -gnu variants in
rustc-beta-aarch64-unknown-linux-gnu/rustc/lib/rustlib and running helloworld
successfully for both targets on arm64 hardware.
@Mark-Simulacrum Mark-Simulacrum modified the milestones: 1.58.0, 1.57.0 Nov 29, 2021
@Mark-Simulacrum Mark-Simulacrum added beta-accepted Accepted for backporting to the compiler in the beta channel. and removed beta-nominated Nominated for backporting to the compiler in the beta channel. labels Nov 29, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 29, 2021
…imulacrum

[stable] 1.57.0 artifacts

This is the standard beta->stable promotion, and includes a last-minute backports of:

* rust-lang#90044 via inclusion of rust-lang#91220.
*  [beta] Don't treat unnormalized function arguments as well-formed rust-lang#91242

r? `@Mark-Simulacrum`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants