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

Mark atomics as unsupported on thumbv6m #99595

Merged
merged 1 commit into from
Jul 24, 2022
Merged

Conversation

nikic
Copy link
Contributor

@nikic nikic commented Jul 22, 2022

The thumbv6m target does not support atomics. Historically, LLVM
had a bug where atomic load/stores for this target were emitted
as plain load/stores rather than as libatomic calls. This was
fixed in https://reviews.llvm.org/D120026, which will be part of
LLVM 15. As we require that "atomic support" does not use libatomic,
we need to indicate that this target does not have native atomics.

The thumbv6m target does not support atomics. Historically, LLVM
had a bug where atomic load/stores for this target were emitted
as plain load/stores rather than as libatomic calls. This was
fixed in https://reviews.llvm.org/D120026, which will be part of
LLVM 15. As we require that "atomic support" does not use libatomic,
we need to indicate that this target does not have native atomics.
@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jul 22, 2022
@rust-highfive
Copy link
Collaborator

r? @nagisa

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

@rust-highfive
Copy link
Collaborator

⚠️ Warning ⚠️

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 22, 2022
@nagisa
Copy link
Member

nagisa commented Jul 23, 2022

@bors r+

Recovering this support seems like it’d be fairly straightforward by adding to compiler-builtins the necessary __atomic functions that use the MSR to implement a barrier. But for now this PR reflects reality of us not having these definitions as far as I can tell.

cc @Amanieu

@bors
Copy link
Contributor

bors commented Jul 23, 2022

📌 Commit 7514610 has been approved by nagisa

It is now in the queue for this repository.

@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 Jul 23, 2022
@Amanieu
Copy link
Member

Amanieu commented Jul 23, 2022

As we require that "atomic support" does not use libatomic, we need to indicate that this target does not have native atomics.

To be precise, we require at atomics be lock-free. It is possible to have lock-free atomics backed by a library, such as when OS support is available (such as on armv5te-unknown-linux-gnueabi).

This change is likely to break a lot of existing code using atomic load/store on thumbv6m (cc @japaric). I think this change makes sense for LLVM where you want all atomic operations to be either provided by a library or implemented natively. However Rust doesn't expose libatomic operations so this concern doesn't apply.

I see 2 solutions here:

  • Since LLVM doesn't handle atomic load/store any more, automatically lower those to volatile load/store in rustc.
  • Do nothing, and tell people using atomic load/store to use volatile load/store instead.

@nikic
Copy link
Contributor Author

nikic commented Jul 23, 2022

As we require that "atomic support" does not use libatomic, we need to indicate that this target does not have native atomics.

To be precise, we require at atomics be lock-free. It is possible to have lock-free atomics backed by a library, such as when OS support is available (such as on armv5te-unknown-linux-gnueabi).

Right. Just to spell it out the relation explicitly: Lock-free atomic libcalls would use __sync style libcalls (from compiler-builtins), rather than __atomic style libcalls (from libatomic).

This change is likely to break a lot of existing code using atomic load/store on thumbv6m (cc @japaric). I think this change makes sense for LLVM where you want all atomic operations to be either provided by a library or implemented natively. However Rust doesn't expose libatomic operations so this concern doesn't apply.

I see 2 solutions here:

  • Since LLVM doesn't handle atomic load/store any more, automatically lower those to volatile load/store in rustc.
  • Do nothing, and tell people using atomic load/store to use volatile load/store instead.

If preserving non-CAS atomics for thumbv6m is important, we could probably add something similar to the existing singlethread target option, which would carry a requirement that code for this target must never be linked against libatomic. Lowering atomic load/store to a memory barrier + normal load/store should be fine, as long as the Rust code is never linked against C code using libatomic. I believe that's effectively the implicit assumption we have been making until now.

bors bot added a commit to taiki-e/portable-atomic that referenced this pull request Jul 23, 2022
18: Use asm-based atomic load/store on thumbv6m r=taiki-e a=taiki-e

Atomic types on thumbv6m will be removed from the standard library: rust-lang/rust#99595

This patch implements the equivalent of the atomic types that will be removed in rust-lang/rust#99595.
Note that this patch does *not* drop support for compilers older than 1.59.

Co-authored-by: Taiki Endo <te316e89@gmail.com>
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jul 23, 2022
Mark atomics as unsupported on thumbv6m

The thumbv6m target does not support atomics. Historically, LLVM
had a bug where atomic load/stores for this target were emitted
as plain load/stores rather than as libatomic calls. This was
fixed in https://reviews.llvm.org/D120026, which will be part of
LLVM 15. As we require that "atomic support" does not use libatomic,
we need to indicate that this target does not have native atomics.
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 23, 2022
…laumeGomez

Rollup of 6 pull requests

Successful merges:

 - rust-lang#99298 (Make `ui-fulldeps/gated-plugins` and `ui-fulldeps/multiple-plugins` tests stage 2 only)
 - rust-lang#99396 (Add some additional double-adjustment regression tests)
 - rust-lang#99449 (Do not resolve associated const when there is no provided value)
 - rust-lang#99595 (Mark atomics as unsupported on thumbv6m)
 - rust-lang#99627 (Lock stdout once when listing tests)
 - rust-lang#99638 (Remove Clean trait implementation for hir::Ty and middle::Ty)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 90c6cde into rust-lang:master Jul 24, 2022
@rustbot rustbot added this to the 1.64.0 milestone Jul 24, 2022
@thejpster
Copy link
Contributor

thejpster commented Jul 24, 2022

What are we supposed to do with all this existing code that uses AtomicUxx load/stores (e.g. on the Raspberry Pi RP2040)? Will it just no longer compile?

@taiki-e
Copy link
Member

taiki-e commented Jul 24, 2022

Yes, they will fail to compile. (Can be checked by using this target specification file)

@jannic
Copy link
Contributor

jannic commented Jul 24, 2022

This would revert #51953 which I think would be wrong. Afaik atomic load and store are available, but CAS is missing, which is covered by atomic_cas: false.

@jamesmunns
Copy link
Member

Just adding another voice to this: if this change lands in stable without a mechanism to still use atomic load/stores (even if the underlying rust impl changes), this will be a huge stable to stable regression and breaking change for thumbv6m targets.

@Amanieu
Copy link
Member

Amanieu commented Jul 24, 2022

I opened #99668 to track this.

@jamesmunns
Copy link
Member

Thank you @Amanieu !

taiki-e added a commit to taiki-e/portable-atomic that referenced this pull request Jul 26, 2022
This reverts commit a864da7.

rust-lang/rust#99595 has been reverted, so this
is no longer needed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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