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

Fix build error with targets that do not support atomic CAS operations #12

Merged
merged 2 commits into from
May 11, 2021

Conversation

taiki-e
Copy link
Member

@taiki-e taiki-e commented May 11, 2021

Some no-std targets (e.g., ARMv6-M) do not support atomic CAS operations and cannot use Arc, etc.

$ cargo build --target thumbv6m-none-eabi --no-default-features --features alloc -p valuable
error[E0433]: failed to resolve: could not find `sync` in `alloc`
   --> valuable/src/enumerable.rs:217:52
    |
217 | impl<E: ?Sized + Enumerable> Enumerable for alloc::sync::Arc<E> {
    |                                                    ^^^^ could not find `sync` in `alloc`

This patch detects those targets using the TARGET environment variables provided by cargo for the build script, and a list of targets that do not support atomic CAS operations.

This way is the same as the way we recently adopted in futures, and was originally inspired by the way heapless and defmt do, but this doesn't maintain the target list manually. (It's not really fully automated, but it's very easy to update.)

refs: rust-lang/rust#51953, rust-lang/futures-rs#2400
related: tokio-rs/bytes#461

Some no-std targets (e.g., ARMv6-M) do not support atomic CAS
operations and cannot use `Arc`, etc. This patch detects those targets
using the `TARGET` environment variables provided by cargo for the
build script, and a list of targets that do not support atomic CAS
operations.
Comment on lines 7 to 19
// The rustc-cfg listed below are considered public API, but it is *unstable*
// and outside of the normal semver guarantees:
//
// - `valuable_no_atomic_cas`
// Assume the target does *not* support atomic CAS operations.
// This is usually detected automatically by the build script, but you may
// need to enable it manually when building for custom targets or using
// non-cargo build systems that don't run the build script.
//
// With the exceptions mentioned above, the rustc-cfg strings below are
// *not* public API. Please let us know by opening a GitHub issue if your build
// environment requires some way to enable these cfgs other than by executing
// our build script.
Copy link
Member Author

@taiki-e taiki-e May 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In futures, there was a question about compatibility with custom targets, so we exposed a similar cfg as an unstable public API.

We can also treat it as a private API until there is an actual request, but this cfg is unstable and we can change/remove it at any time, so I think it is also ok as-is. (Whether this cfg is considered private or public depends on how the comments describe about it, so either way, it is a very simple change.)

Btw, one reason for the instability is that this cfg can eventually be replaced by cfg(accessible). So I believe this cfg will never be a stable API.

Copy link
Member Author

@taiki-e taiki-e May 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll need to implement similar detection logic for atomic integers later, so it's probably better to make this private for now. I'll update PR...

UPDATE: done in f7f1b95

Copy link
Member

@carllerche carllerche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. This strategy is great.

@carllerche carllerche merged commit fb40789 into master May 11, 2021
@taiki-e taiki-e deleted the taiki-e/no-atomic-cas branch May 12, 2021 01:27
carllerche pushed a commit that referenced this pull request May 17, 2021
This implements Valuable for the following types.

- AtomicBool
- AtomicI8
- AtomicI16
- AtomicI32
- AtomicI64
- AtomicIsize
- AtomicU8
- AtomicU16
- AtomicU32
- AtomicU64
- AtomicUsize

In some targets, atomic u64/i64 is not available or atomic is not available at all, so this detects those
targets by using the same way as #12.
bors bot added a commit to crossbeam-rs/crossbeam that referenced this pull request May 29, 2021
698: Remove uses of unstable feature(cfg_target_has_atomic) r=taiki-e a=taiki-e

Some no-std targets (e.g., ARMv6-M) do not support atomic CAS operations and cannot use Arc, etc.

Currently, we are using an unstable feature to detect them, but it has caused breakage in the past (#435).
Also, users of stable Rust are not able to compile crossbeam on those targets.

Instead of depending on unstable features of the compiler, this patch detects those targets using the TARGET environment variables provided by cargo for the build script, and a list of targets that do not support atomic CAS operations.

This way is the same as the way we recently adopted in [futures](rust-lang/futures-rs#2400) and [valuable](tokio-rs/valuable#12), and was originally inspired by the way [heapless](rust-embedded/heapless@44c66a7) and [defmt](https://github.com/knurling-rs/defmt/blob/963152f0fc530fca64ba4ff1492d9c4b7bf76062/build.rs#L42-L51) do, but this doesn't maintain the target list manually. (It's not really fully automated, but [it's very easy to update](https://github.com/crossbeam-rs/crossbeam/blob/a42dbed87a5739228b576f526b1e2fd80260a29b/.github/workflows/ci.yml#L89).)

Also, this completely removes the dependency on unstable features from crates other than crossbeam-epoch.

refs: rust-lang/rust#51953, rust-lang/futures-rs#2400, tokio-rs/valuable#12

704: Add AtomicCell::fetch_update r=taiki-e a=taiki-e

Equivalent of [`std::sync::atomic::AtomicN::fetch_update`](https://doc.rust-lang.org/nightly/core/sync/atomic/struct.AtomicUsize.html#method.fetch_update) that stabilized in Rust 1.45.



Co-authored-by: Taiki Endo <te316e89@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants