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

enable Atomic*.{load,store} for ARMv6-M / MSP430 #51953

Merged
merged 3 commits into from
Jul 6, 2018

Conversation

japaric
Copy link
Member

@japaric japaric commented Jun 30, 2018

closes #45085

as proposed in #45085 (comment)

this commit adds an atomic_cas target option and extends the #[cfg(target_has_atomic)]
attribute to enable a subset of the Atomic* API on architectures that don't support atomic CAS
natively, like MSP430 and ARMv6-M.

r? @alexcrichton

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 30, 2018
@@ -479,6 +479,9 @@ declare_features! (

// Allows async and await syntax
(active, async_await, "1.28.0", Some(50547), None),

// Allows async and await syntax
(active, cfg_target_has_atomic_cas, "1.28.0", Some(0), None),
Copy link
Member Author

Choose a reason for hiding this comment

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

after this gets a thumbs up I'll create a tracking issue and fill it the number here

@japaric
Copy link
Member Author

japaric commented Jun 30, 2018

cc @bradjc @pftbest

@pftbest
Copy link
Contributor

pftbest commented Jun 30, 2018

Looks good to me. Unfortunately I think there is a bug in llvm that needs to be fixed to use atomics on msp430. There is a chance that libcore will fail to compile on msp with this change, need to check before committing.

@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-3.9 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:start:tidy
tidy check
[00:04:58] * 554 error codes
[00:04:58] * highest error code: E0709
[00:04:59] Expected a gate test for the feature 'cfg_target_has_atomic_cas'.
[00:04:59] Hint: create a failing test file named 'feature-gate-cfg_target_has_atomic_cas.rs'
[00:04:59]       in the 'ui' test suite, with its failures due to
[00:04:59]       missing usage of #![feature(cfg_target_has_atomic_cas)].
[00:04:59] Hint: If you already have such a test and don't want to rename it,
[00:04:59]       you can also add a // gate-test-cfg_target_has_atomic_cas line to the test file.
[00:04:59] tidy error: Found 1 features without a gate test.
[00:05:00] some tidy checks failed
[00:05:00] 
[00:05:00] 
[00:05:00] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:05:00] 
[00:05:00] 
[00:05:00] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:05:00] Build completed unsuccessfully in 0:01:44
[00:05:00] Build completed unsuccessfully in 0:01:44
[00:05:00] Makefile:79: recipe for target 'tidy' failed
[00:05:00] make: *** [tidy] Error 1

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:1bf598a7
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
---
travis_time:end:09529b6e:start=1530389450177286860,finish=1530389450184758329,duration=7471469
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:13d7d22e
$ head -30 ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
head: cannot open ‘./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers’ for reading: No such file or directory
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:00f89848
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks!

@@ -163,7 +164,8 @@ mod boxed {
#[cfg(test)]
mod boxed_test;
pub mod collections;
#[cfg(target_has_atomic = "ptr")]
#[cfg_attr(stage0, cfg(target_has_atomic = "ptr"))]
#[cfg_attr(not(stage0), cfg(all(target_has_atomic = "ptr", target_has_atomic_cas)))]
Copy link
Member

Choose a reason for hiding this comment

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

Instead of this cfg_attr, perhaps this could be:

#[cfg(any(
    all(stage0, target_has_atomic = "ptr"),
    all(not(stage0), target_has_atomic_cas, target_has_atomic = "ptr")
))]

That may be a bit easier to clean up after the next stage0

Copy link
Member

Choose a reason for hiding this comment

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

Also, in terms of implementation, it may be easiest to piggy back on the existing target_has_atomic perhaps and do something like target_has_atomic ="cas" maybe?

@@ -853,6 +862,7 @@ impl<T> AtomicPtr<T> {
/// ```
#[inline]
#[stable(feature = "extended_compare_and_swap", since = "1.10.0")]
#[cfg_attr(not(stage0), cfg(target_has_atomic_cas))]
Copy link
Member

Choose a reason for hiding this comment

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

This I think can be #[cfg(any(stage0, target_has_atomic_cas))]

closes rust-lang#45085

this commit adds an `atomic_cas` target option and an unstable `#[cfg(target_has_atomic_cas)]`
attribute to enable a subset of the `Atomic*` API on architectures that don't support atomic CAS
natively, like MSP430 and ARMv6-M.
@japaric
Copy link
Member Author

japaric commented Jul 5, 2018

@alexcrichton latest commit reuses the existing #[cfg(target_has_atomic)] attribute and applies the suggested #[cfg] changes.

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Jul 5, 2018

📌 Commit 0ed3231 has been approved by alexcrichton

@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 5, 2018
@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-3.9 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
Check compiletest suite=run-make-fulldeps mode=run-make (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[01:26:54] 
[01:26:54] running 187 tests
[01:27:39] ....................i...............................................................................
[01:28:30] .........................................................................F............test [run-make] run-make-fulldeps/long-linker-command-lines has been running for over 60 seconds
[01:29:37] thread 'main' panicked at 'Some tests failed', tools/compiletest/src/main.rs:498:22
[01:29:37] failures:
[01:29:37] 
[01:29:37] ---- [run-make] run-make-fulldeps/target-without-atomics stdout ----
[01:29:37] ---- [run-make] run-make-fulldeps/target-without-atomics stdout ----
[01:29:37] 
[01:29:37] error: make failed
[01:29:37] status: exit code: 2
[01:29:37] command: "make"
[01:29:37] stdout:
[01:29:37] ------------------------------------------
[01:29:37] make[1]: Entering directory '/checkout/src/test/run-make-fulldeps/target-without-atomics'
[01:29:37] LD_LIBRARY_PATH="/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-make-fulldeps/target-without-atomics/target-without-atomics:/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib:/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools/x86_64-unknown-linux-gnu/release/deps:/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-sysroot/lib/rustlib/x86_64-unknown-linux-gnu/lib:" '/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc' --out-dir /checkout/obj/build/x86_64-unknown-linux-gnu/test/run-make-fulldeps/target-without-atomics/target-without-atomics -L /checkout/obj/build/x86_64-unknown-linux-gnu/test/run-make-fulldeps/target-without-atomics/target-without-atomics  --print cfg --target thumbv6m-none-eabi | "/checkout/src/etc/cat-and-grep.sh" -v target_has_atomic
[01:29:37] [[[ begin stdout ]]]
[01:29:37] debug_assertions
[01:29:37] target_arch="arm"
[01:29:37] target_endian="little"
[01:29:37] target_env=""
[01:29:37] target_has_atomic="16"
[01:29:37] target_has_atomic="32"
[01:29:37] target_has_atomic="8"
[01:29:37] target_has_atomic="ptr"
[01:29:37] target_os="none"
[01:29:37] target_pointer_width="32"
[01:29:37] target_vendor=""
[01:29:37] 
[01:29:37] [[[ end stdout ]]]
[01:29:37] Error: should not match: target_has_atomic
[01:29:37] Makefile:5: recipe for target 'all' failed
[01:29:37] make[1]: Leaving directory '/checkout/src/test/run-make-fulldeps/target-without-atomics'
[01:29:37] ------------------------------------------
[01:29:37] stderr:
[01:29:37] ------------------------------------------
[01:29:37] ------------------------------------------
[01:29:37] make[1]: *** [all] Error 1
[01:29:37] ------------------------------------------
[01:29:37] 
[01:29:37] 
[01:29:37] thread '[run-make] run-make-fulldeps/target-without-atomics' panicked at 'explicit panic', tools/compiletest/src/runtest.rs:3140:9
[01:29:37] 
[01:29:37] 
[01:29:37] failures:
[01:29:37]     [run-make] run-make-fulldeps/target-without-atomics
[01:29:37]     [run-make] run-make-fulldeps/target-without-atomics
[01:29:37] 
[01:29:37] test result: FAILED. 185 passed; 1 failed; 1 ignored; 0 measured; 0 filtered out
[01:29:37] 
[01:29:37] 
[01:29:37] 
[01:29:37] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/compiletest" "--compile-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib" "--run-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib" "--rustc-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "--rustdoc-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustdoc" "--src-base" "/checkout/src/test/run-make-fulldeps" "--build-base" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-make-fulldeps" "--stage-id" "stage2-x86_64-unknown-linux-gnu" "--mode" "run-make" "--target" "x86_64-unknown-linux-gnu" "--host" "x86_64-unknown-linux-gnu" "--llvm-filecheck" "/usr/lib/llvm-3.9/bin/FileCheck" "--host-rustcflags" "-Crpath -O -Zunstable-options " "--target-rustcflags" "-Crpath -O -Zunstable-options  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--docck-python" "/usr/bin/python2.7" "--lldb-python" "/usr/bin/python2.7" "--gdb" "/usr/bin/gdb" "--quiet" "--llvm-version" "3.9.1\n" "--system-llvm" "--cc" "cc" "--cxx" "c++" "--cflags" "-ffunction-sections -fdata-sections -fPIC -m64" "--llvm-components" "aarch64 aarch64asmparser aarch64asmprinter aarch64codegen aarch64desc aarch64disassembler aarch64info aarch64utils all all-targets amdgpu amdgpuasmparser amdgpuasmprinter amdgpucodegen amdgpudesc amdgpudisassembler amdgpuinfo amdgpuutils analysis arm armasmparser armasmprinter armcodegen armdesc armdisassembler arminfo asmparser asmprinter bitreader bitwriter bpf bpfasmprinter bpfcodegen bpfdesc bpfinfo codegen core coverage debuginfocodeview debuginfodwarf debuginfopdb engine executionengine globalisel hexagon hexagonasmparser hexagoncodegen hexagondesc hexagondisassembler hexagoninfo instcombine instrumentation interpreter ipo irreader libdriver lineeditor linker lto mc mcdisassembler mcjit mcparser mips mipsasmparser mipsasmprinter mipscodegen mipsdesc mipsdisassembler mipsinfo mirparser msp430 msp430asmprinter msp430codegen msp430desc msp430info native nativecodegen nvptx nvptxasmprinter nvptxcodegen nvptxdesc nvptxinfo objcarcopts object objectyaml option orcjit passes powerpc powerpcasmparser powerpcasmprinter powerpccodegen powerpcdesc powerpcdisassembler powerpcinfo profiledata runtimedyld scalaropts selectiondag sparc sparcasmparser sparcasmprinter sparccodegen sparcdesc sparcdisassembler sparcinfo support symbolize systemz systemzasmparser systemzasmprinter systemzcodegen systemzdesc systemzdisassembler systemzinfo tablegen target transformutils vectorize x86 x86asmparser x86asmprinter x86codegen x86desc x86disassembler x86info x86utils xcore xcoreasmprinter xcorecodegen xcoredesc xcoredisassembler xcoreinfo" "--llvm-cxxflags" "-I/usr/lib/llvm-3.9/include -std=c++0x -gsplit-dwarf -Wl,-fuse-ld=gold -fPIC -fvisibility-inlines-hidden -Wall -W -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -pedantic -Wno-long-long -Wno-maybe-uninitialized -Wdelete-non-virtual-dtor -Wno-comment -Werror=date-time -std=c++11 -ffunction-sections -fdata-sections -O2 -g -DNDEBUG  -fno-exceptions -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS" "--ar" "ar" "--adb-path" "adb" "--adb-test-dir" "/data/tmp/work" "--android-cross-path" "" "--color" "always"
[01:29:37] 
[01:29:37] 
[01:29:37] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[01:29:37] Build completed unsuccessfully in 0:46:56
[01:29:37] Build completed unsuccessfully in 0:46:56
[01:29:37] Makefile:58: recipe for target 'check' failed
[01:29:37] make: *** [check] Error 1
sckowoo9/s-f2msl497wd-46px1w-2tbvj9xpkkcf2
122008 ./obj/build/x86_64-unknown-linux-gnu/stage1-std/x86_64-unknown-linux-gnu/release
115724 ./obj/build/x86_64-unknown-linux-gnu/test/mir-opt
104364 ./obj/build/x86_64-unknown-linux-gnu/stage0-tools/x86_64-unknown-linux-gnu
104360 ./obj/build/x86_64-unknown-linux-gnu/stage0-tools/x86_64-unknown-linux-gnu/release

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@kennytm
Copy link
Member

kennytm commented Jul 6, 2018

@bors r-

There is a failed CI test.

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 6, 2018
@bradjc
Copy link
Contributor

bradjc commented Jul 6, 2018

Very excited about this change!

@japaric
Copy link
Member Author

japaric commented Jul 6, 2018

@bors r=alexcrichton

@bors
Copy link
Contributor

bors commented Jul 6, 2018

📌 Commit f9145d0 has been approved by alexcrichton

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 6, 2018
@bors
Copy link
Contributor

bors commented Jul 6, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 0072c95 to master...

@bors bors merged commit f9145d0 into rust-lang:master Jul 6, 2018
Nemo157 added a commit to Nemo157/futures-rs that referenced this pull request Jul 14, 2018
As of rust-lang/rust#51953
`cfg(target_has_atomic)` has been updated to separately enable
compare-and-swap (CAS) operations from the different supported sizes.
Because `AtomicWaker` uses these CAS operations we need to also gate its
inclusion on `cfg(target_has_atomic = "cas")`. `thumv6m` is one of the
architectures that supports atomic read-write operations on its pointer
sized integers, but doesn't support CAS operations.
sdroege pushed a commit to sdroege/futures-rs that referenced this pull request Jul 19, 2018
As of rust-lang/rust#51953
`cfg(target_has_atomic)` has been updated to separately enable
compare-and-swap (CAS) operations from the different supported sizes.
Because `AtomicWaker` uses these CAS operations we need to also gate its
inclusion on `cfg(target_has_atomic = "cas")`. `thumv6m` is one of the
architectures that supports atomic read-write operations on its pointer
sized integers, but doesn't support CAS operations.
pietroalbini added a commit to pietroalbini/rust that referenced this pull request Sep 22, 2018
…richton

remove (more) CAS API from Atomic* types where not natively supported

closes rust-lang#54276

In PR rust-lang#51953 I made the Atomic* types available on targets like thumbv6m and
msp430 with the intention of *only* exposing the load and store API on those
types -- the rest of the API doesn't work on those targets because the are no
native instructions to implement CAS loops.

Unfortunately, it seems I didn't properly cfg away all the CAS API on those
targets, as evidenced in rust-lang#54276. This PR amends the issue by removing the rest
of the CAS API.

This is technically a breaking change because *libraries* that were using this
API and were being compiled for e.g. thumbv6m-none-eabi will stop compiling.
However, using those libraries (before this change) in programs (binaries) would
lead to linking errors when compiled for e.g. thumbv6m so this change
effectively shifts a linker error in binaries to a compiler error in libraries.

On a side note: extending the Atomic API is a bit error prone because of these
non-cas targets. Unless the author of the change is aware of these targets and
properly uses `#[cfg(atomic = "cas")]` they could end up exposing new CAS API on
these targets. I can't think of a test to check that an API is not present on
some target, but we could extend the `tidy` tool to check that *all* newly added
atomic API has the `#[cfg(atomic = "cas")]` attribute unless it's whitelisted in
`tidy` then the author of the change would have to verify if the API can be used
on non-cas targets.

In any case, I'd like to plug this hole ASAP. We can revisit testing in a
follow-up issue / PR.

r? @alexcrichton
cc @mvirkkunen
japaric added a commit to japaric/rust that referenced this pull request Oct 28, 2018
PR rust-lang#51953 enabled the Atomic*.{load,store} API on MSP430. Unfortunately, the
LLVM backend doesn't currently support those atomic operations, so this commit
removes the API and leaves instructions on how and when to enable it in the
future.
kennytm added a commit to kennytm/rust that referenced this pull request Oct 30, 2018
msp430: remove the whole Atomic* API

PR rust-lang#51953 enabled the Atomic*.{load,store} API on MSP430. Unfortunately,
the LLVM backend doesn't currently support those atomic operations, so this
commit removes the API and leaves instructions on how and when to enable it
in the future.

the second fixes compiling liballoc for msp430

closes rust-lang#54511
r? @alexcrichton
cc @chernomor @awygle @cr1901 @pftbest
bradjc added a commit to tock/tock that referenced this pull request Nov 26, 2018
Since rust-lang/rust#51953 was merged, there is
now support for atomic load and store with cortex-m0 in rust by default.
bradjc added a commit to tock/tock that referenced this pull request Nov 30, 2018
Since rust-lang/rust#51953 was merged, there is
now support for atomic load and store with cortex-m0 in rust by default.
taiki-e added a commit to taiki-e/crossbeam that referenced this pull request May 1, 2019
taiki-e added a commit to taiki-e/crossbeam that referenced this pull request May 11, 2019
alistair23 added a commit to alistair23/tock that referenced this pull request Jan 23, 2020
This reverts commit 7f20daa.

There are RISC-V platforms that don't support atomics (such as
OpenTitan) so let's revert this commit until RV32 has atomic load and
store support when the Atomic (A) extension isn't supported. That is
wait until something like this for RISC-V is merged:
rust-lang/rust#51953

Signed-off-by: Alistair Francis <[email protected]>
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 <[email protected]>
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider adding load/store atomics for targets that don't support compare and swap
7 participants