close
The Wayback Machine - https://web.archive.org/web/20211125083512/https://github.com/rust-lang/rust/pull/90044
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

@rust-highfive rust-highfive commented Oct 19, 2021

r? @oli-obk

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

Loading

@oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Oct 19, 2021

Loading

@jbg
Copy link

@jbg 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.

Loading

@hkratz
Copy link
Contributor Author

@hkratz 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).

Loading

@hkratz
Copy link
Contributor Author

@hkratz 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

Loading

@jbg
Copy link

@jbg 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.

Loading

@hkratz
Copy link
Contributor Author

@hkratz 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.

Loading

@workingjubilee
Copy link
Contributor

@workingjubilee workingjubilee commented Nov 5, 2021

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

Loading

@workingjubilee
Copy link
Contributor

@workingjubilee workingjubilee commented Nov 5, 2021

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.

Loading

@workingjubilee
Copy link
Contributor

@workingjubilee workingjubilee commented Nov 5, 2021

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.

Loading

@bors
Copy link
Contributor

@bors bors commented Nov 6, 2021

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

Loading

@hkratz
Copy link
Contributor Author

@hkratz 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.

Loading

@bors bors closed this in cd5de2f Nov 7, 2021
@workingjubilee workingjubilee reopened this Nov 8, 2021
workingjubilee added a commit to workingjubilee/rustc that referenced this issue Nov 8, 2021
workingjubilee added a commit to workingjubilee/rustc that referenced this issue 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 issue Nov 9, 2021
bors added a commit to rust-lang-ci/rust that referenced this issue Nov 10, 2021
The introduced dependency on `getauxval`causes linking
problems with musl, see rust-lang#89626.
@hkratz hkratz force-pushed the disable_arm_outline_atomics_for_musl branch from 9c7500e to bd287fa Nov 10, 2021
workingjubilee added a commit to workingjubilee/rustc that referenced this issue Nov 24, 2021
Changelog:
https://github.com/rust-lang/libc/releases/tag/0.2.107
https://github.com/rust-lang/libc/releases/tag/0.2.108
Primarily intended to pull in fd331f65f214ea75b6210b415b5fd8650be15c73
This should help with rust-lang#90044
@hkratz
Copy link
Contributor Author

@hkratz hkratz commented Nov 24, 2021

Damn, this can't be backported to beta as #90623 (on which this builds now) was after the beta fork.

Loading

@workingjubilee
Copy link
Contributor

@workingjubilee workingjubilee commented Nov 24, 2021

Whoops, didn't notice that this got updated, sorry. Unfortunately didn't find time to push through the libc cascade all the way, so it looks like this will be it for now. We can figure out if #90681 will fix it on the next train, when we have the luxury of time.
This looks correct.

Loading

@workingjubilee
Copy link
Contributor

@workingjubilee workingjubilee commented Nov 24, 2021

Ah damn.

Loading

@workingjubilee
Copy link
Contributor

@workingjubilee workingjubilee commented Nov 24, 2021

@pnkfelix It seems this PR can't land on beta, but I agree with Hans' remarks on Zulip regarding the severity of the regression. I have no idea how we should proceed in terms of mitigation.

Brainstorming alternatives in full-on "throw spaghetti at wall" mode:

  • Technically #90681 might fix it if merged and then backported but that seems risky as it pulls in even more diffs, which would be fine on nightly but to push into beta would involve even more risks of regressions. That would also require #90527
  • hack in one of the known mitigations for now, like defaulting to using lld for that target for a cycle??? seems also risky.
  • cry and allow the target to regress :( strictly speaking, this is (probably?) permitted by our aarch64-linux-musl platform support policy, as it is tier 2 and thus we do not have the solidity of the tier 1 "yes, it always works" policies, but it seems a bit strained since it always is supposed to build.

Loading

@hkratz
Copy link
Contributor Author

@hkratz hkratz commented Nov 24, 2021

I can open a minimal, functionally equivalent PR against beta (just restricting aarch64 outline atomics to glibc) and test that thoroughly. But I don't know if policy allows (non-backport) PRs that land only in beta.

Loading

@workingjubilee
Copy link
Contributor

@workingjubilee workingjubilee commented Nov 24, 2021

Sometimes we do point releases, so I can't imagine it's completely banned? I am not familiar with that nuance, however.

Loading

@workingjubilee
Copy link
Contributor

@workingjubilee workingjubilee commented Nov 24, 2021

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

Loading

@workingjubilee
Copy link
Contributor

@workingjubilee workingjubilee commented Nov 24, 2021

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

Loading

@bors
Copy link
Contributor

@bors bors commented Nov 24, 2021

📌 Commit bd287fa has been approved by workingjubilee

Loading

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue 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 issue 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.
bors added a commit to rust-lang-ci/rust that referenced this issue 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
10 checks passed
Loading
@rustbot rustbot added this to the 1.58.0 milestone Nov 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

9 participants