Allow for manual opt-out of CAS features #120

jamesmunns · 2019-10-02T15:11:32Z

I've been working on a bare metal platform lately that does not have CAS (specifically ARMv4 bare metal), and this is not caught by the CPU detection logic in the build.rs. This PR adds a flag that allows for manual specification to avoid CAS features.

japaric · 2019-12-17T13:33:12Z

Thanks for the PR and sorry it took me so long to get to it.

Ideally, I would like to use the output of rustc --print cfg (see the rustc-cfg crate) here:

$ rustc --print cfg thumbv6m-none-eabi
debug_assertions
target_arch="arm"
target_endian="little"
target_env=""
target_feature="mclass"
target_feature="v5te"
target_feature="v6"
target_has_atomic_load_store="16"
target_has_atomic_load_store="32"
target_has_atomic_load_store="8"
target_has_atomic_load_store="ptr"
target_os="none"
target_pointer_width="32"
target_vendor=""

$ rustc --print cfg --target thumbv7m-none-eabi
debug_assertions
target_arch="arm"
target_endian="little"
target_env=""
target_feature="mclass"
target_feature="v5te"
target_feature="v6"
target_feature="v6k"
target_feature="v6t2"
target_feature="v7"
target_has_atomic="16"
target_has_atomic="32"
target_has_atomic="8"
target_has_atomic="ptr"
target_has_atomic_load_store="16"
target_has_atomic_load_store="32"
target_has_atomic_load_store="8"
target_has_atomic_load_store="ptr"
target_os="none"
target_pointer_width="32"
target_vendor=""

target_has_atomic indicates the CAS support; target_has_atomic_load_store indicates support only for atomic loads and stores. However, that output seems to be nightly-only for now (I believe there are no immediate plans to stabilize that part of the output):

$ cargo +beta --print cfg --target thumbv7m-none-eabi
debug_assertions
target_arch="arm"
target_endian="little"
target_env=""
target_os="none"
target_pointer_width="32"
target_vendor=""

As that can't be used, using Cargo features is the next best option. However, Cargo features must be additive (*); meaning that instead of an opt-in no_cas feature we should have an opt-out cas feature that makes the SPSC API available. The cas feature needs to be enabled by default to release it under a new patch version and not break existing users.

(*) To see what can go wrong with no_cas imagine a crate A uses heapless::spsc::Queue then in your crate you enable the no_cas feature of heapless and add A as a dependency; cargo build-ining your crate will fail to build the A dependency.

jamesmunns · 2019-12-17T14:04:24Z

I've inverted the logic here, and tried to add some helpful diagnostics:

cd heapless
cargo build --target thumbv7em-none-eabihf # works
cargo build --no-default-features --target thumbv6m-none-eabi # works
cargo build --target thumbv6m-none-eabi # doesn't work
cargo build --no-default-features --target thumbv6m-none-eabi --features cas # doesn't work

jamesmunns · 2019-12-17T14:47:56Z

src/pool/mod.rs

@@ -154,7 +154,6 @@
 //!
 //! [1]: https://static.docs.arm.com/ddi0403/eb/DDI0403E_B_armv7m_arm.pdf

-#[cfg(not(armv6m))]


Note: This is okay to remove, as the entire pool module is removed when the cas feature is not set, and it is not possible to set the cas feature with armv6m targets.

jamesmunns · 2019-12-17T14:50:21Z

In this PR, by inverting the features here, I believe this is now a breaking change, as any users who were intentionally or unintentionally setting no-default-features would lose access to mpmc and pool.

With the no_cas feature, this would not be a breaking change in my opinion.

japaric · 2019-12-17T15:04:05Z

I believe this is now a breaking change, as any users who were intentionally or unintentionally setting no-default-features would lose access to mpmc and pool.

You are correct. This is in principle a breaking change. However, the crate currently has zero default features so the number of people using no-default-features should also be zero as that configuration is a no-op.

Thanks!

bors r+

120: Allow for manual opt-out of CAS features r=japaric a=jamesmunns I've been working on a bare metal platform lately that does not have CAS (specifically ARMv4 bare metal), and this is not caught by the CPU detection logic in the build.rs. This PR adds a flag that allows for manual specification to avoid CAS features. Co-authored-by: James Munns <[email protected]>

bors · 2019-12-17T15:07:48Z

Build failed

continuous-integration/travis-ci/push

japaric · 2019-12-17T15:29:00Z

src/lib.rs

 pub mod mpmc;
-#[cfg(not(armv6m))]


Right, so we actually need to keep this not as: cfg(all(not(armv6m), feature = "cas")) to keep cargo build --target thumbv6m-none-eabi working.

Is that the interface you would like (implicitly masking the features), rather than forcing the user to opt-out with no-default-features?

I mentioned this in this comment.

To get this PR into a new patch version we need to keep cargo build --target thumbv6m-none-eabi (note: "cas" is implicitly enabled) working so the logic needs to make the "cas" feature a no-op on that target.

Happy to make the change, though I might suggest that we change the "implicit no-op" behavior on the next breaking (minor) release.

And probably make "cas" opt-in as well; I don't like having a crate that by default doesn't build for all targets. Could you make an issue to make sure we don't forget? I'll put on the v0.6.0 milestone.

japaric · 2019-12-17T15:29:21Z

src/lib.rs

 pub mod pool;
 pub mod spsc;

+#[cfg(all(armv6m, feature = "cas"))]
+core::compile_error!("The 'cas' feature may not be used with `armv6m` targets.");


And we have to drop this

Basically, the state of the "cas" feature should have no effect on ARMv6-M.

jamesmunns · 2019-12-17T16:05:24Z

@japaric back to you.

japaric · 2019-12-17T16:09:08Z

Thanks!

bors r+

120: Allow for manual opt-out of CAS features r=japaric a=jamesmunns I've been working on a bare metal platform lately that does not have CAS (specifically ARMv4 bare metal), and this is not caught by the CPU detection logic in the build.rs. This PR adds a flag that allows for manual specification to avoid CAS features. Co-authored-by: James Munns <[email protected]>

bors · 2019-12-17T16:11:51Z

Build failed

continuous-integration/travis-ci/push

jamesmunns · 2019-12-17T17:08:41Z

@japaric I missed you added a feature at the end of the file in my merge. This should work now.

japaric · 2019-12-18T10:51:58Z

bors r+

120: Allow for manual opt-out of CAS features r=japaric a=jamesmunns I've been working on a bare metal platform lately that does not have CAS (specifically ARMv4 bare metal), and this is not caught by the CPU detection logic in the build.rs. This PR adds a flag that allows for manual specification to avoid CAS features. Co-authored-by: James Munns <[email protected]>

bors · 2019-12-18T10:57:18Z

Build succeeded

continuous-integration/travis-ci/push

Allow for manual opt-out of CAS features

620ee9a

Invert feature flag logic

410bc9c

jamesmunns commented Dec 17, 2019

View reviewed changes

jamesmunns mentioned this pull request Dec 17, 2019

heapless does not build for riscv32imc-unknown-none-elf target #123

Closed

japaric suggested changes Dec 17, 2019

View reviewed changes

James Munns added 2 commits December 17, 2019 17:01

Change feature gating

cbc1c5d

Merge remote-tracking branch 'origin/master' into manual-cas-disable

d5789b7

japaric mentioned this pull request Dec 17, 2019

remove spsc / pool / mpmc modules on targets w/o atomic / CAS support #135

Merged

Fix bad merge

434b7b5

bors bot merged commit 434b7b5 into rust-embedded:master Dec 18, 2019

jamesmunns deleted the manual-cas-disable branch March 25, 2020 16:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow for manual opt-out of CAS features #120

Allow for manual opt-out of CAS features #120

jamesmunns commented Oct 2, 2019

japaric commented Dec 17, 2019

jamesmunns commented Dec 17, 2019

jamesmunns Dec 17, 2019

jamesmunns commented Dec 17, 2019

japaric commented Dec 17, 2019

bors bot commented Dec 17, 2019

japaric Dec 17, 2019

jamesmunns Dec 17, 2019

japaric Dec 17, 2019

jamesmunns Dec 17, 2019

japaric Dec 17, 2019

japaric Dec 17, 2019

japaric Dec 17, 2019

jamesmunns commented Dec 17, 2019

japaric commented Dec 17, 2019

bors bot commented Dec 17, 2019

jamesmunns commented Dec 17, 2019

japaric commented Dec 18, 2019

bors bot commented Dec 18, 2019

Allow for manual opt-out of CAS features #120

Allow for manual opt-out of CAS features #120

Conversation

jamesmunns commented Oct 2, 2019

japaric commented Dec 17, 2019

jamesmunns commented Dec 17, 2019

Choose a reason for hiding this comment

jamesmunns commented Dec 17, 2019

japaric commented Dec 17, 2019

bors bot commented Dec 17, 2019

Build failed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesmunns commented Dec 17, 2019

japaric commented Dec 17, 2019

bors bot commented Dec 17, 2019

Build failed

jamesmunns commented Dec 17, 2019

japaric commented Dec 18, 2019

bors bot commented Dec 18, 2019

Build succeeded