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

1.0.1 does not compile for arm-thumb-v6m targets (Cortex-M0) #461

Open
henrikssn opened this issue Jan 13, 2021 · 17 comments · May be fixed by #467
Open

1.0.1 does not compile for arm-thumb-v6m targets (Cortex-M0) #461

henrikssn opened this issue Jan 13, 2021 · 17 comments · May be fixed by #467
Assignees

Comments

@henrikssn
Copy link

You can reproduce by cloning cortex-m-quickstart and setting target accordingly in .cargo/config

$ rustc --version
rustc 1.48.0 (7eac88abb 2020-11-16)
$ cargo --version
cargo 1.48.0 (65cbdd2dc 2020-10-14)
error: could not compile `bytes`

To learn more, run the command again with --verbose.
warning: build failed, waiting for other jobs to finish...
error: build failed
error[E0599]: no method named `fetch_add` found for struct `AtomicUsize` in the current scope
   --> /home/erik/.cargo/registry/src/github.com-1ecc6299db9ec823/bytes-1.0.1/src/bytes.rs:970:38
    |
970 |     let old_size = (*shared).ref_cnt.fetch_add(1, Ordering::Relaxed);
    |                                      ^^^^^^^^^ method not found in `AtomicUsize`

error[E0599]: no method named `compare_exchange` found for reference `&AtomicPtr<()>` in the current scope
    --> /home/erik/.cargo/registry/src/github.com-1ecc6299db9ec823/bytes-1.0.1/src/bytes.rs:1030:16
     |
1030 |     match atom.compare_exchange(ptr as _, shared as _, Ordering::AcqRel, Ordering::Acquire) {
     |                ^^^^^^^^^^^^^^^^ method not found in `&AtomicPtr<()>`

error[E0599]: no method named `fetch_sub` found for struct `AtomicUsize` in the current scope
    --> /home/erik/.cargo/registry/src/github.com-1ecc6299db9ec823/bytes-1.0.1/src/bytes.rs:1058:23
     |
1058 |     if (*ptr).ref_cnt.fetch_sub(1, Ordering::Release) != 1 {
     |                       ^^^^^^^^^ method not found in `AtomicUsize`

error[E0599]: no method named `fetch_add` found for struct `AtomicUsize` in the current scope
    --> /home/erik/.cargo/registry/src/github.com-1ecc6299db9ec823/bytes-1.0.1/src/bytes_mut.rs:1201:37
     |
1201 |     let old_size = (*ptr).ref_count.fetch_add(1, Ordering::Relaxed);
     |                                     ^^^^^^^^^ method not found in `AtomicUsize`

error[E0599]: no method named `fetch_sub` found for struct `AtomicUsize` in the current scope
    --> /home/erik/.cargo/registry/src/github.com-1ecc6299db9ec823/bytes-1.0.1/src/bytes_mut.rs:1210:25
     |
1210 |     if (*ptr).ref_count.fetch_sub(1, Ordering::Release) != 1 {
     |                         ^^^^^^^^^ method not found in `AtomicUsize`

error: aborting due to 5 previous errors

For more information about this error, try `rustc --explain E0599`.
       Error Failed to run cargo build: exit code = Some(101)

@taiki-e
Copy link
Member

taiki-e commented Jan 13, 2021

This is because those targets do not support atomic CAS. See also rust-lang/rust#51953.

The feature to handle this correctly on the library side is unstable, and some crates support it via the unstable feature flag (cfg-target-has-atomic feature in futures-rs, nightly feature in crossbeam, etc.).

Technically bytes can do the same, but it may not be very helpful as Bytes, BytesMut, etc. are not available on those platforms. (If crates that depend on bytes are using it, the compile will fail anyway.)
Whether it can fix the problem you are encountering depends on how bytes are actually used in your project's dependencies.

@taiki-e
Copy link
Member

taiki-e commented Jan 13, 2021

@henrikssn I have created a patch to support those targets (https://github.com/taiki-e/bytes/tree/cfg_target_has_atomic). Could you try if it solves the problem in your project? (in the way described below)

  1. Add the following block to your project's root Cargo.toml:
[patch.crates-io]
bytes = { git = "https://github.com/taiki-e/bytes", branch = "cfg_target_has_atomic" }
  1. And run cargo with RUSTFLAGS="--cfg=bytes_unstable", like:
RUSTFLAGS="--cfg=bytes_unstable" cargo build

@henrikssn
Copy link
Author

Works like a charm!

The packages I depend only use Buf / BufMut from this package and we are always passing byte slices to them.

@dunmatt
Copy link

dunmatt commented Jan 17, 2021

Is it possible to do a similar magic to allow bytes::Bytes to be used? I'm trying to get prost to build for thumbv6m-none-eabi and it's complaining...

@niclashoyer
Copy link

while the patch allows compilation it now requires nightly, doesn't it? Would it instead be possible to just add a feature flag for Bytes, so one can choose this by using Cargo features? Or maybe put the traits in another crate?

@taiki-e
Copy link
Member

taiki-e commented Jan 20, 2021

@dunmatt

Is it possible to do a similar magic to allow bytes::Bytes to be used?

atomic CAS (compare and swap) cannot be used in thumbv6m-none-eabi and Bytes uses it. It may be possible to rewrite the implementation to use only atomic load/store, but that probably not be easy.


@niclashoyer

while the patch allows compilation it now requires nightly, doesn't it? Would it instead be possible to just add a feature flag for Bytes, so one can choose this by using Cargo features?

Adding an enabled by default feature that enables Bytes/BytesMut is a breaking change, and adding a disabled by default feature that disables Bytes/BytesMut violates the rules of cargo features that "features should be additive".

There is a way to avoid using unstable features, but there are some problems.

Or maybe put the traits in another crate?

I don't think it's possible without breaking changes. Buf uses Bytes in public API.

@carllerche
Copy link
Member

If it never compiled, I don't think it would be a breaking change to support Bytes as a !Send and !Sync type on some platforms. That said, I'm not sure if we want to do that :) It would probably be better to support compilation of bytes on those platforms w/o the type entirely and someone could implement a single-threaded Bytes type in a separate crate.

Thoughts?

@dunmatt
Copy link

dunmatt commented Feb 13, 2021 via email

@taiki-e
Copy link
Member

taiki-e commented Feb 13, 2021

If it never compiled, I don't think it would be a breaking change to support Bytes as a !Send and !Sync type on some platforms.

I don't think this is preferable. This can make crates that currently manually implement Send on types containing Bytes/BytesMut unsound on some platforms.

@taiki-e
Copy link
Member

taiki-e commented May 23, 2021

while the patch allows compilation it now requires nightly, doesn't it?

Updated #467 to support stable Rust.

@marius-meissner
Copy link

Maybe I'm missing something, then I apologize upfront.

Could using the following polyfill help in any way?
https://crates.io/crates/atomic-polyfill

@taiki-e
Copy link
Member

taiki-e commented Mar 17, 2022

Could using the following polyfill help in any way?
https://crates.io/crates/atomic-polyfill

That is unsound on multi-core systems.

@marius-meissner
Copy link

That is unsound on multi-core systems.

Right, but what about adding it as an optional feature? In this case, single thread applications (including interrupts) could make safe use of Bytes/BytesMut, which would be a huge benefit for systems like Cortex M0.

@taiki-e
Copy link
Member

taiki-e commented Mar 18, 2022

Supporting it as an optional dependency is a bad idea if enabling it could cause unsoundness.

One of the approachs to handle such cases soundly is to use the optional cfg as my portable-atomic crate does.

This is intentionally not an optional feature. (If this is an optional feature, dependencies can implicitly enable the feature, resulting in the use of unsound code without the end-user being aware of it.)

This prevents them from accidentally being compiled against a multi-core target unless the end-user explicitly guarantees that the target is single core.

@marius-meissner
Copy link

One of the approachs to handle such cases soundly is to use the optional cfg

Thanks, makes absolute sense.

as my portable-atomic crate does.

Wow, what an awesome crate!
Are there reasons against integrating this in bytes?

My naive draft would be the following extension of your PR #467

#[cfg(not(all(test, loom)))]
pub(crate) mod sync {
    pub(crate) mod atomic {
        #[cfg(not(bytes_no_atomic_cas))]
        pub(crate) use core::sync::atomic::{fence, AtomicPtr, AtomicUsize, Ordering};

        #[cfg(bytes_no_atomic_cas)]
        pub(crate) use core::sync::atomic::{fence, Ordering};
        #[cfg(bytes_no_atomic_cas)]
        pub(crate) use portable_atomic::{AtomicUsize, AtomicPtr};

@taiki-e
Copy link
Member

taiki-e commented Aug 12, 2022

@marius-meissner
Sorry for the late response.

as my portable-atomic crate does.

Wow, what an awesome crate!
Are there reasons against integrating this in bytes?

I was hesitant to use this with bytes because of tokio's strict dependency policy and the fact that there were few users at the time, but now that crates like metrics and spin are using portable-atomic, I think we can progress this. (Also, the increase of dependencies can be avoided in most cases by using portable-atomic as an optional dependency, as spin did.)

@carllerche @Darksonn: Any objection to adding an optional dependency (portable-atomic) to support these targets? I'm the (only) maintainer of that crate and can add others from the tokio core team as backup maintainers if needed. Also, since it is an optional dependency for no-std users, it is unlikely that it will actually appear in tokio's dependency tree.
EDIT: Also, portable-atomic is a pre-1.0 crate, but it will not be used in the public API and I expect it will be treated like tokio's parking_lot feature.

@carllerche
Copy link
Member

I think it would be fine to add it as an optional dependency. Maybe call it something like extra-platforms or something.

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 a pull request may close this issue.

6 participants