Merge #447

447: Add implementation for critical-section 1.0 r=adamgreig a=Dirbaio

Picking up #433 since it seems stalled. Changes from #433 are:
- Update to `critical-section 1.0.0-alpha.2`
- Use `bool` restore token
- Name Cargo feature `critical-section-single-core`.

TODO before merging:

- [x] Wait for `critical-section 1.0` release rust-embedded/critical-section#19

Co-Authored-By: Markus Reiter `@reitermarkus` 

Co-authored-by: Dario Nieuwenhuis <dirbaio@dirbaio.net>

.github/workflows/ci.yml

@@ -29,6 +29,6 @@ jobs:
           toolchain: ${{ matrix.rust }}
           override: true
       - name: Run tests
-        run: cargo test --all --exclude cortex-m-rt --exclude testsuite
+        run: cargo test --all --exclude cortex-m-rt --exclude testsuite --features cortex-m/critical-section-single-core
 
 # FIXME: test on macOS and Windows

.github/workflows/clippy.yml

@@ -23,4 +23,4 @@ jobs:
       - uses: actions-rs/clippy-check@v1
         with:
           token: ${{ secrets.GITHUB_TOKEN }}
-          args: --all
+          args: --all --features cortex-m/critical-section-single-core

.github/workflows/on-target.yml

@@ -22,7 +22,7 @@ jobs:
       - name: Build testsuite
         env:
           RUSTFLAGS: -C link-arg=-Tlink.x -D warnings
-        run: cargo build -p testsuite --target thumbv7m-none-eabi --features testsuite/semihosting
+        run: cargo build -p testsuite --target thumbv7m-none-eabi --features semihosting,cortex-m/critical-section-single-core
       - name: Install QEMU
         run: sudo apt-get update && sudo apt-get install qemu qemu-system-arm
       - name: Run testsuite
@@ -51,7 +51,7 @@ jobs:
       - name: Build testsuite
         env:
           RUSTFLAGS: -C link-arg=-Tlink.x -D warnings
-        run: cargo build -p testsuite --target thumbv6m-none-eabi --features testsuite/rtt
+        run: cargo build -p testsuite --target thumbv6m-none-eabi --features rtt,cortex-m/critical-section-single-core
       - name: Upload testsuite binaries
         uses: actions/upload-artifact@v3
         with:

.github/workflows/rt-ci.yml

@@ -69,18 +69,18 @@ jobs:
       - name: Install all Rust targets
         run: rustup target install thumbv6m-none-eabi thumbv7m-none-eabi thumbv7em-none-eabi thumbv7em-none-eabihf thumbv8m.base-none-eabi thumbv8m.main-none-eabi thumbv8m.main-none-eabihf
       - name: Build examples for thumbv6m-none-eabi
-        run: cargo build --target=thumbv6m-none-eabi --examples
+        run: cargo build --target=thumbv6m-none-eabi --features cortex-m/critical-section-single-core --examples
       - name: Build examples for thumbv7m-none-eabi
-        run: cargo build --target=thumbv7m-none-eabi --examples
+        run: cargo build --target=thumbv7m-none-eabi --features cortex-m/critical-section-single-core --examples
       - name: Build examples for thumbv7em-none-eabi
-        run: cargo build --target=thumbv7em-none-eabi --examples
+        run: cargo build --target=thumbv7em-none-eabi --features cortex-m/critical-section-single-core --examples
       - name: Build examples for thumbv7em-none-eabihf
-        run: cargo build --target=thumbv7em-none-eabihf --examples
+        run: cargo build --target=thumbv7em-none-eabihf --features cortex-m/critical-section-single-core --examples
       - name: Build examples for thumbv8m.base-none-eabi
-        run: cargo build --target=thumbv8m.base-none-eabi --examples
+        run: cargo build --target=thumbv8m.base-none-eabi --features cortex-m/critical-section-single-core --examples
       - name: Build examples for thumbv8m.main-none-eabi
-        run: cargo build --target=thumbv8m.main-none-eabi --examples
+        run: cargo build --target=thumbv8m.main-none-eabi --features cortex-m/critical-section-single-core --examples
       - name: Build examples for thumbv8m.main-none-eabihf
-        run: cargo build --target=thumbv8m.main-none-eabihf --examples
+        run: cargo build --target=thumbv8m.main-none-eabihf --features cortex-m/critical-section-single-core --examples
       - name: Build crate for host OS
         run: cargo build

CHANGELOG.md

@@ -17,9 +17,11 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 - TPIU: add `swo_supports` for checking what SWO configurations the target supports. (#381)
 - Add `std` and `serde` crate features for improved host-side ITM decode functionality when working with the downstream `itm`, `cargo-rtic-scope` crates (#363, #366).
 - Added the ability to name the statics generated by `singleton!()` for better debuggability (#364, #380).
+- Added `critical-section-single-core` feature which provides an implementation for the `critical_section` crate for single-core systems, based on disabling all interrupts. (#447)
 
 ### Fixed
 - Fixed `singleton!()` statics sometimes ending up in `.data` instead of `.bss` (#364, #380).
+- `interrupt::free` no longer hands out a `CriticalSection` token because it is unsound on multi-core. Use `critical_section::with` instead. (#447)
 
 ### Changed
 - Inline assembly is now always used, requiring Rust 1.59.

Cargo.toml

@@ -17,7 +17,7 @@ rust-version = "1.59"
 links = "cortex-m"  # prevent multiple versions of this crate to be linked together
 
 [dependencies]
-bare-metal = "1"
+critical-section = "1.0.0"
 volatile-register = "0.2.0"
 bitfield = "0.13.2"
 embedded-hal = "0.2.4"
@@ -32,6 +32,7 @@ cm7 = []
 cm7-r0p1 = ["cm7"]
 linker-plugin-lto = []
 std = []
+critical-section-single-core = ["critical-section/restore-state-bool"]
 
 [workspace]
 members = [

cortex-m-rt/ci/script.sh

@@ -7,10 +7,13 @@ main() {
 
     cargo check --target "$TARGET" --features device
 
+    # A `critical_section` implementation is always needed.
+    needed_features=cortex-m/critical-section-single-core
+
     if [ "$TARGET" = x86_64-unknown-linux-gnu ] && [ "$TRAVIS_RUST_VERSION" = stable ]; then
         ( cd macros && cargo check && cargo test )
 
-        cargo test --features device --test compiletest
+        cargo test --features "device,${needed_features}" --test compiletest
     fi
 
     local examples=(
@@ -43,35 +46,35 @@ main() {
     if [ "$TARGET" != x86_64-unknown-linux-gnu ]; then
         # Only test on stable and nightly, not MSRV.
         if [ "$TRAVIS_RUST_VERSION" = stable ] || [ "$TRAVIS_RUST_VERSION" = nightly ]; then
-            RUSTDOCFLAGS="-Cpanic=abort" cargo test --doc
+            RUSTDOCFLAGS="-Cpanic=abort" cargo test --features "${needed_features}" --doc
         fi
 
         for linker in "${linkers[@]}"; do
             for ex in "${examples[@]}"; do
-                cargo rustc --target "$TARGET" --example "$ex" -- $linker
-                cargo rustc --target "$TARGET" --example "$ex" --release -- $linker
+                cargo rustc --target "$TARGET" --example "$ex" --features "${needed_features}" -- $linker
+                cargo rustc --target "$TARGET" --example "$ex" --features "${needed_features}" --release -- $linker
             done
             for ex in "${fail_examples[@]}"; do
-                ! cargo rustc --target "$TARGET" --example "$ex" -- $linker
-                ! cargo rustc --target "$TARGET" --example "$ex" --release -- $linker
+                ! cargo rustc --target "$TARGET" --example "$ex" --features "${needed_features}" -- $linker
+                ! cargo rustc --target "$TARGET" --example "$ex" --features "${needed_features}" --release -- $linker
             done
-            cargo rustc --target "$TARGET" --example device --features device -- $linker
-            cargo rustc --target "$TARGET" --example device --features device --release -- $linker
+            cargo rustc --target "$TARGET" --example device --features "device,${needed_features}" -- $linker
+            cargo rustc --target "$TARGET" --example device --features "device,${needed_features}" --release -- $linker
 
-            cargo rustc --target "$TARGET" --example minimal --features set-sp -- $linker
-            cargo rustc --target "$TARGET" --example minimal --features set-sp --release -- $linker
-            cargo rustc --target "$TARGET" --example minimal --features set-vtor -- $linker
-            cargo rustc --target "$TARGET" --example minimal --features set-vtor --release -- $linker
+            cargo rustc --target "$TARGET" --example minimal --features "set-sp,${needed_features}" -- $linker
+            cargo rustc --target "$TARGET" --example minimal --features "set-sp,${needed_features}" --release -- $linker
+            cargo rustc --target "$TARGET" --example minimal --features "set-vtor,${needed_features}" -- $linker
+            cargo rustc --target "$TARGET" --example minimal --features "set-vtor,${needed_features}" --release -- $linker
         done
     fi
 
     case $TARGET in
         thumbv6m-none-eabi|thumbv7m-none-eabi)
             for linker in "${linkers[@]}"; do
                 env RUSTFLAGS="$linker -C link-arg=-Tlink.x" cargo run \
-                    --target "$TARGET" --example qemu | grep "x = 42"
+                    --target "$TARGET" --features "${needed_features}" --example qemu | grep "x = 42"
                 env RUSTFLAGS="$linker -C link-arg=-Tlink.x" cargo run \
-                    --target "$TARGET" --example qemu --release | grep "x = 42"
+                    --target "$TARGET" --features "${needed_features}" --example qemu --release | grep "x = 42"
             done
 
             ;;

cortex-m-semihosting/Cargo.toml

@@ -21,3 +21,4 @@ no-semihosting = []
 
 [dependencies]
 cortex-m = { path = "..", version = ">= 0.5.8, < 0.8" }
+critical-section = "1.0.0"

cortex-m-semihosting/src/export.rs

@@ -2,14 +2,12 @@
 
 use core::fmt::{self, Write};
 
-use cortex_m::interrupt;
-
 use crate::hio::{self, HostStream};
 
 static mut HSTDOUT: Option<HostStream> = None;
 
 pub fn hstdout_str(s: &str) {
-    let _result = interrupt::free(|_| unsafe {
+    let _result = critical_section::with(|_| unsafe {
         if HSTDOUT.is_none() {
             HSTDOUT = Some(hio::hstdout()?);
         }
@@ -19,7 +17,7 @@ pub fn hstdout_str(s: &str) {
 }
 
 pub fn hstdout_fmt(args: fmt::Arguments) {
-    let _result = interrupt::free(|_| unsafe {
+    let _result = critical_section::with(|_| unsafe {
         if HSTDOUT.is_none() {
             HSTDOUT = Some(hio::hstdout()?);
         }
@@ -31,7 +29,7 @@ pub fn hstdout_fmt(args: fmt::Arguments) {
 static mut HSTDERR: Option<HostStream> = None;
 
 pub fn hstderr_str(s: &str) {
-    let _result = interrupt::free(|_| unsafe {
+    let _result = critical_section::with(|_| unsafe {
         if HSTDERR.is_none() {
             HSTDERR = Some(hio::hstderr()?);
         }
@@ -41,7 +39,7 @@ pub fn hstderr_str(s: &str) {
 }
 
 pub fn hstderr_fmt(args: fmt::Arguments) {
-    let _result = interrupt::free(|_| unsafe {
+    let _result = critical_section::with(|_| unsafe {
         if HSTDERR.is_none() {
             HSTDERR = Some(hio::hstderr()?);
         }

src/critical_section.rs

@@ -0,0 +1,27 @@
+#[cfg(all(cortex_m, feature = "critical-section-single-core"))]
+mod single_core_critical_section {
+    use critical_section::{set_impl, Impl, RawRestoreState};
+
+    use crate::interrupt;
+    use crate::register::primask;
+
+    struct SingleCoreCriticalSection;
+    set_impl!(SingleCoreCriticalSection);
+
+    unsafe impl Impl for SingleCoreCriticalSection {
+        unsafe fn acquire() -> RawRestoreState {
+            let was_active = primask::read().is_active();
+            interrupt::disable();
+            was_active
+        }
+
+        unsafe fn release(was_active: RawRestoreState) {
+            // Only re-enable interrupts if they were enabled before the critical section.
+            if was_active {
+                interrupt::enable()
+            }
+        }
+    }
+}
+
+pub use critical_section::with;

src/interrupt.rs

@@ -1,6 +1,5 @@
 //! Interrupts
 
-pub use bare_metal::{CriticalSection, Mutex};
 #[cfg(cortex_m)]
 use core::arch::asm;
 #[cfg(cortex_m)]
@@ -27,7 +26,7 @@ pub unsafe trait InterruptNumber: Copy {
     fn number(self) -> u16;
 }
 
-/// Disables all interrupts
+/// Disables all interrupts in the current core.
 #[cfg(cortex_m)]
 #[inline]
 pub fn disable() {
@@ -39,11 +38,11 @@ pub fn disable() {
     compiler_fence(Ordering::SeqCst);
 }
 
-/// Enables all the interrupts
+/// Enables all the interrupts in the current core.
 ///
 /// # Safety
 ///
-/// - Do not call this function inside an `interrupt::free` critical section
+/// - Do not call this function inside a critical section.
 #[cfg(cortex_m)]
 #[inline]
 pub unsafe fn enable() {
@@ -53,21 +52,26 @@ pub unsafe fn enable() {
     asm!("cpsie i", options(nomem, nostack, preserves_flags));
 }
 
-/// Execute closure `f` in an interrupt-free context.
+/// Execute closure `f` with interrupts disabled in the current core.
 ///
-/// This as also known as a "critical section".
+/// This method does not synchronise multiple cores and may disable required
+/// interrupts on some platforms; see the `critical-section` crate for a cross-platform
+/// way to enter a critical section which provides a `CriticalSection` token.
+///
+/// This crate provides an implementation for `critical-section` suitable for single-core systems,
+/// based on disabling all interrupts. It can be enabled with the `critical-section-single-core` feature.
 #[cfg(cortex_m)]
 #[inline]
 pub fn free<F, R>(f: F) -> R
 where
-    F: FnOnce(&CriticalSection) -> R,
+    F: FnOnce() -> R,
 {
     let primask = crate::register::primask::read();
 
     // disable interrupts
     disable();
 
-    let r = f(unsafe { &CriticalSection::new() });
+    let r = f();
 
     // If the interrupts were active before our `disable` call, then re-enable
     // them. Otherwise, keep them disabled
@@ -85,7 +89,7 @@ where
 #[inline]
 pub fn free<F, R>(_: F) -> R
 where
-    F: FnOnce(&CriticalSection) -> R,
+    F: FnOnce() -> R,
 {
     panic!("cortex_m::interrupt::free() is only functional on cortex-m platforms");
 }

src/lib.rs

@@ -43,15 +43,16 @@
 // Don't warn about feature(asm) being stable on Rust >= 1.59.0
 #![allow(stable_features)]
 
-extern crate bare_metal;
-extern crate volatile_register;
-
 #[macro_use]
 mod macros;
 
 pub mod asm;
 #[cfg(armv8m)]
 pub mod cmse;
+// This is only public so the `singleton` macro does not require depending on
+// the `critical-section` crate separately.
+#[doc(hidden)]
+pub mod critical_section;
 pub mod delay;
 pub mod interrupt;
 #[cfg(all(not(armv6m), not(armv8m_base)))]

src/macros.rs

@@ -62,7 +62,7 @@ macro_rules! iprintln {
 #[macro_export]
 macro_rules! singleton {
     ($name:ident: $ty:ty = $expr:expr) => {
-        $crate::interrupt::free(|_| {
+        $crate::critical_section::with(|_| {
             // this is a tuple of a MaybeUninit and a bool because using an Option here is
             // problematic:  Due to niche-optimization, an Option could end up producing a non-zero
             // initializer value which would move the entire static from `.bss` into `.data`...

src/peripheral/mod.rs

@@ -57,7 +57,6 @@
 //!
 //! - ARMv7-M Architecture Reference Manual (Issue E.b) - Chapter B3
 
-use crate::interrupt;
 use core::marker::PhantomData;
 use core::ops;
 
@@ -164,7 +163,7 @@ impl Peripherals {
     /// Returns all the core peripherals *once*
     #[inline]
     pub fn take() -> Option<Self> {
-        interrupt::free(|_| {
+        critical_section::with(|_| {
             if unsafe { TAKEN } {
                 None
             } else {

src/peripheral/sau.rs

@@ -7,7 +7,6 @@
 //!
 //! For reference please check the section B8.3 of the Armv8-M Architecture Reference Manual.
 
-use crate::interrupt;
 use crate::peripheral::SAU;
 use bitfield::bitfield;
 use volatile_register::{RO, RW};
@@ -162,7 +161,7 @@ impl SAU {
     /// This function is executed under a critical section to prevent having inconsistent results.
     #[inline]
     pub fn set_region(&mut self, region_number: u8, region: SauRegion) -> Result<(), SauError> {
-        interrupt::free(|_| {
+        critical_section::with(|_| {
             let base_address = region.base_address;
             let limit_address = region.limit_address;
             let attribute = region.attribute;
@@ -215,7 +214,7 @@ impl SAU {
     /// This function is executed under a critical section to prevent having inconsistent results.
     #[inline]
     pub fn get_region(&mut self, region_number: u8) -> Result<SauRegion, SauError> {
-        interrupt::free(|_| {
+        critical_section::with(|_| {
             if region_number >= self.region_numbers() {
                 Err(SauError::RegionNumberTooBig)
             } else {

testsuite/Cargo.toml

@@ -13,6 +13,7 @@ semihosting = ["cortex-m-semihosting", "minitest/semihosting"]
 cortex-m-rt.path = "../cortex-m-rt"
 cortex-m.path = ".."
 minitest.path = "minitest"
+critical-section = "1.0.0"
 
 [dependencies.rtt-target]
 version = "0.3.1"

testsuite/minitest/macros/src/lib.rs

@@ -215,8 +215,8 @@ fn tests_impl(args: TokenStream, input: TokenStream) -> parse::Result<TokenStrea
         unsafe {
             ::rtt_target::set_print_channel_cs(
                 channels.up.0,
-                &((|arg, f| cortex_m::interrupt::free(|_| f(arg)))
-                    as rtt_target::CriticalSectionFunc),
+                &((|arg, f| ::critical_section::with(|_| f(arg)))
+                    as ::rtt_target::CriticalSectionFunc),
             );
         }
     });