Support loom in rtic-common and rtic-sync. Fix rtic-sync bug.

.cargo/config.toml

@@ -4,3 +4,5 @@ pxtask = "run --package xtask --features rayon --"
 
 # Don't define the RUSTFLAGS link.x thing here: it messes
 # up compilation of the usage examples.
+[build]
+rustflags = [ "--cfg", "loom" ]

rtic-common/CHANGELOG.md

@@ -8,6 +8,7 @@ For each category, *Added*, *Changed*, *Fixed* add new entries at the top!
 ## [Unreleased]
 
 ### Added
+- Support `loom`.
 
 ### Changed
 

rtic-common/Cargo.toml

@@ -18,9 +18,18 @@ repository = "https://github.com/rtic-rs/rtic"
 # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
 
 [dependencies]
-critical-section = "1"
 portable-atomic = { version = "1", default-features = false }
 
 [features]
 default = []
-testing = ["critical-section/std"]
+testing = [ "critical-section/std" ]
+
+[target.'cfg(not(loom))'.dependencies]
+critical-section = "1"
+
+[target.'cfg(loom)'.dependencies]
+loom = { version = "0.7.2" }
+critical-section = { version = "1", features = [ "restore-state-bool" ] }
+
+[lints.rust]
+unexpected_cfgs = { level = "allow", check-cfg = ['cfg(loom)'] }

rtic-common/src/lib.rs

@@ -1,6 +1,6 @@
 //! Utility structs that can be useful to other subcrates.
 
-#![no_std]
+#![cfg_attr(not(loom), no_std)]
 #![deny(missing_docs)]
 
 #[cfg(test)]
@@ -10,3 +10,9 @@ extern crate std;
 pub mod dropper;
 pub mod wait_queue;
 pub mod waker_registration;
+
+#[cfg(loom)]
+pub mod loom_cs;
+
+#[cfg(not(loom))]
+pub mod unsafecell;

rtic-common/src/loom_cs.rs

@@ -0,0 +1,69 @@
+//! A loom-based implementation of CriticalSection, effectively copied from the critical_section::std module.
+
+use core::cell::RefCell;
+use core::mem::MaybeUninit;
+
+use loom::cell::Cell;
+use loom::sync::{Mutex, MutexGuard};
+
+loom::lazy_static! {
+    static ref GLOBAL_MUTEX: Mutex<()> = Mutex::new(());
+    // This is initialized if a thread has acquired the CS, uninitialized otherwise.
+    static ref GLOBAL_GUARD: RefCell<MaybeUninit<MutexGuard<'static, ()>>> = RefCell::new(MaybeUninit::uninit());
+}
+
+loom::thread_local!(static IS_LOCKED: Cell<bool> = Cell::new(false));
+
+struct StdCriticalSection;
+critical_section::set_impl!(StdCriticalSection);
+
+unsafe impl critical_section::Impl for StdCriticalSection {
+    unsafe fn acquire() -> bool {
+        // Allow reentrancy by checking thread local state
+        IS_LOCKED.with(|l| {
+            if l.get() {
+                // CS already acquired in the current thread.
+                return true;
+            }
+
+            // Note: it is fine to set this flag *before* acquiring the mutex because it's thread local.
+            // No other thread can see its value, there's no potential for races.
+            // This way, we hold the mutex for slightly less time.
+            l.set(true);
+
+            // Not acquired in the current thread, acquire it.
+            let guard = match GLOBAL_MUTEX.lock() {
+                Ok(guard) => guard,
+                Err(err) => {
+                    // Ignore poison on the global mutex in case a panic occurred
+                    // while the mutex was held.
+                    err.into_inner()
+                }
+            };
+            GLOBAL_GUARD.borrow_mut().write(guard);
+
+            false
+        })
+    }
+
+    unsafe fn release(nested_cs: bool) {
+        if !nested_cs {
+            // SAFETY: As per the acquire/release safety contract, release can only be called
+            // if the critical section is acquired in the current thread,
+            // in which case we know the GLOBAL_GUARD is initialized.
+            //
+            // We have to `assume_init_read` then drop instead of `assume_init_drop` because:
+            // - drop requires exclusive access (&mut) to the contents
+            // - mutex guard drop first unlocks the mutex, then returns. In between those, there's a brief
+            //   moment where the mutex is unlocked but a `&mut` to the contents exists.
+            // - During this moment, another thread can go and use GLOBAL_GUARD, causing `&mut` aliasing.
+            #[allow(let_underscore_lock)]
+            let _ = GLOBAL_GUARD.borrow_mut().assume_init_read();
+
+            // Note: it is fine to clear this flag *after* releasing the mutex because it's thread local.
+            // No other thread can see its value, there's no potential for races.
+            // This way, we hold the mutex for slightly less time.
+            IS_LOCKED.with(|l| l.set(false));
+        }
+    }
+}

rtic-common/src/unsafecell.rs

@@ -0,0 +1,28 @@
+//! Loom-compatible [`core::cell::UnsafeCell`].
+
+/// An [`core::cell::UnsafeCell`] wrapper that provides compatibility with
+/// loom's UnsafeCell.
+#[derive(Debug)]
+pub struct UnsafeCell<T>(core::cell::UnsafeCell<T>);
+
+impl<T> UnsafeCell<T> {
+    /// Create a new `UnsafeCell`.
+    pub const fn new(data: T) -> UnsafeCell<T> {
+        UnsafeCell(core::cell::UnsafeCell::new(data))
+    }
+
+    /// Access the contents of the `UnsafeCell` through a const pointer.
+    pub fn with<R>(&self, f: impl FnOnce(*const T) -> R) -> R {
+        f(self.0.get())
+    }
+
+    /// Access the contents of the `UnsafeCell` through a mut pointer.
+    pub fn with_mut<R>(&self, f: impl FnOnce(*mut T) -> R) -> R {
+        f(self.0.get())
+    }
+
+    /// Consume the UnsafeCell.
+    pub fn into_inner(self) -> T {
+        self.0.into_inner()
+    }
+}

rtic-common/src/wait_queue.rs

@@ -5,7 +5,14 @@ use core::pin::Pin;
 use core::ptr::null_mut;
 use core::task::Waker;
 use critical_section as cs;
-use portable_atomic::{AtomicBool, AtomicPtr, Ordering};
+
+use portable_atomic::Ordering;
+
+#[cfg(not(loom))]
+use portable_atomic::{AtomicBool, AtomicPtr};
+
+#[cfg(loom)]
+use loom::sync::atomic::{AtomicBool, AtomicPtr};
 
 /// A helper definition of a wait queue.
 pub type WaitQueue = DoublyLinkedList<Waker>;
@@ -21,12 +28,22 @@ pub struct DoublyLinkedList<T> {
 
 impl<T> DoublyLinkedList<T> {
     /// Create a new linked list.
+    #[cfg(not(loom))]
     pub const fn new() -> Self {
         Self {
             head: AtomicPtr::new(null_mut()),
             tail: AtomicPtr::new(null_mut()),
         }
     }
+
+    /// Create a new linked list.
+    #[cfg(loom)]
+    pub fn new() -> Self {
+        Self {
+            head: AtomicPtr::new(null_mut()),
+            tail: AtomicPtr::new(null_mut()),
+        }
+    }
 }
 
 impl<T> Default for DoublyLinkedList<T> {
@@ -123,6 +140,7 @@ impl<T: Clone> Link<T> {
     const R: Ordering = Ordering::Relaxed;
 
     /// Create a new link.
+    #[cfg(not(loom))]
     pub const fn new(val: T) -> Self {
         Self {
             val,
@@ -133,6 +151,18 @@ impl<T: Clone> Link<T> {
         }
     }
 
+    /// Create a new link.
+    #[cfg(loom)]
+    pub fn new(val: T) -> Self {
+        Self {
+            val,
+            next: AtomicPtr::new(null_mut()),
+            prev: AtomicPtr::new(null_mut()),
+            is_popped: AtomicBool::new(false),
+            _up: PhantomPinned,
+        }
+    }
+
     /// Return true if this link has been poped from the list.
     pub fn is_popped(&self) -> bool {
         self.is_popped.load(Self::R)

rtic-common/src/waker_registration.rs

@@ -1,8 +1,13 @@
 //! Waker registration utility.
 
-use core::cell::UnsafeCell;
 use core::task::Waker;
 
+#[cfg(not(loom))]
+use crate::unsafecell::UnsafeCell;
+
+#[cfg(loom)]
+use loom::cell::UnsafeCell;
+
 /// A critical section based waker handler.
 pub struct CriticalSectionWakerRegistration {
     waker: UnsafeCell<Option<Waker>>,
@@ -13,54 +18,68 @@ unsafe impl Sync for CriticalSectionWakerRegistration {}
 
 impl CriticalSectionWakerRegistration {
     /// Create a new waker registration.
+    #[cfg(not(loom))]
     pub const fn new() -> Self {
         Self {
             waker: UnsafeCell::new(None),
         }
     }
 
+    /// Create a new waker registration.
+    #[cfg(loom)]
+    pub fn new() -> Self {
+        Self {
+            waker: UnsafeCell::new(None),
+        }
+    }
+
     /// Register a waker.
     /// This will overwrite the previous waker if there was one.
     pub fn register(&self, new_waker: &Waker) {
         critical_section::with(|_| {
-            // SAFETY: This access is protected by the critical section.
-            let self_waker = unsafe { &mut *self.waker.get() };
+            self.waker.with_mut(|self_waker| {
+                // SAFETY: This access is protected by the critical section.
+                let self_waker = unsafe { &mut *self_waker };
 
-            // From embassy
-            // https://github.com/embassy-rs/embassy/blob/b99533607ceed225dd12ae73aaa9a0d969a7365e/embassy-sync/src/waitqueue/waker.rs#L59-L61
-            match self_waker {
-                // Optimization: If both the old and new Wakers wake the same task, we can simply
-                // keep the old waker, skipping the clone. (In most executor implementations,
-                // cloning a waker is somewhat expensive, comparable to cloning an Arc).
-                Some(ref w2) if (w2.will_wake(new_waker)) => {}
-                _ => {
-                    // clone the new waker and store it
-                    if let Some(old_waker) = core::mem::replace(self_waker, Some(new_waker.clone()))
-                    {
-                        // We had a waker registered for another task. Wake it, so the other task can
-                        // reregister itself if it's still interested.
-                        //
-                        // If two tasks are waiting on the same thing concurrently, this will cause them
-                        // to wake each other in a loop fighting over this WakerRegistration. This wastes
-                        // CPU but things will still work.
-                        //
-                        // If the user wants to have two tasks waiting on the same thing they should use
-                        // a more appropriate primitive that can store multiple wakers.
-                        old_waker.wake()
+                // From embassy
+                // https://github.com/embassy-rs/embassy/blob/b99533607ceed225dd12ae73aaa9a0d969a7365e/embassy-sync/src/waitqueue/waker.rs#L59-L61
+                match self_waker {
+                    // Optimization: If both the old and new Wakers wake the same task, we can simply
+                    // keep the old waker, skipping the clone. (In most executor implementations,
+                    // cloning a waker is somewhat expensive, comparable to cloning an Arc).
+                    Some(ref w2) if (w2.will_wake(new_waker)) => {}
+                    _ => {
+                        // clone the new waker and store it
+                        if let Some(old_waker) =
+                            core::mem::replace(self_waker, Some(new_waker.clone()))
+                        {
+                            // We had a waker registered for another task. Wake it, so the other task can
+                            // reregister itself if it's still interested.
+                            //
+                            // If two tasks are waiting on the same thing concurrently, this will cause them
+                            // to wake each other in a loop fighting over this WakerRegistration. This wastes
+                            // CPU but things will still work.
+                            //
+                            // If the user wants to have two tasks waiting on the same thing they should use
+                            // a more appropriate primitive that can store multiple wakers.
+                            old_waker.wake()
+                        }
                     }
                 }
-            }
+            });
         });
     }
 
     /// Wake the waker.
     pub fn wake(&self) {
         critical_section::with(|_| {
-            // SAFETY: This access is protected by the critical section.
-            let self_waker = unsafe { &mut *self.waker.get() };
-            if let Some(waker) = self_waker.take() {
-                waker.wake()
-            }
+            self.waker.with_mut(|self_waker| {
+                // SAFETY: This access is protected by the critical section.
+                let self_waker = unsafe { &mut *self_waker };
+                if let Some(waker) = self_waker.take() {
+                    waker.wake()
+                }
+            });
         });
     }
 }

rtic-sync/CHANGELOG.md

@@ -27,6 +27,11 @@ For each category, _Added_, _Changed_, _Fixed_ add new entries at the top!
 - `defmt v0.3` derives added and forwarded to `embedded-hal(-x)` crates.
 - signal structure
 
+### Fixed
+- Fix [#780].
+
+[#780]: https://github.com/rtic-rs/rtic/issues/780
+
 ## v1.2.0 - 2024-01-10
 
 ### Changed

rtic-sync/Cargo.toml

@@ -25,14 +25,22 @@ portable-atomic = { version = "1", default-features = false }
 embedded-hal = { version = "1.0.0" }
 embedded-hal-async = { version = "1.0.0" }
 embedded-hal-bus = { version = "0.2.0", features = ["async"] }
-
 defmt-03 = { package = "defmt", version = "0.3", optional = true }
 
 [dev-dependencies]
 static_cell = "2.1.0"
-tokio = { version = "1", features = ["rt", "macros", "time"] }
+
+[target.'cfg(not(loom))'.dev-dependencies]
+tokio = { version = "1", features = ["rt", "macros", "time"], default-features = false }
 
 [features]
 default = []
-testing = ["critical-section/std", "rtic-common/testing"]
+testing = ["rtic-common/testing"]
 defmt-03 = ["dep:defmt-03", "embedded-hal/defmt-03", "embedded-hal-async/defmt-03", "embedded-hal-bus/defmt-03"]
+
+[lints.rust]
+unexpected_cfgs = { level = "allow", check-cfg = ['cfg(loom)'] }
+
+[target.'cfg(loom)'.dependencies]
+loom = { version = "0.7.2", features = [ "futures" ] }
+cassette = "0.3.0"

rtic-sync/checkpoint.json

@@ -0,0 +1 @@
+{"preemption_bound":null,"pos":0,"branches":{"entries":[{"Schedule":{"preemptions":0,"initial_active":0,"threads":["Active","Disabled","Disabled","Disabled","Disabled"],"prev":null,"exploring":true}},{"Schedule":{"preemptions":0,"initial_active":0,"threads":["Active","Disabled","Disabled","Disabled","Disabled"],"prev":{"index":0,"_p":null},"exploring":true}},{"Load":{"values":[0,0,0,0,0,0,0],"pos":0,"len":1,"exploring":true}},{"Schedule":{"preemptions":0,"initial_active":0,"threads":["Active","Disabled","Disabled","Disabled","Disabled"],"prev":{"index":1,"_p":null},"exploring":true}},{"Schedule":{"preemptions":0,"initial_active":0,"threads":["Active","Disabled","Disabled","Disabled","Disabled"],"prev":{"index":3,"_p":null},"exploring":true}},{"Schedule":{"preemptions":0,"initial_active":0,"threads":["Active","Disabled","Disabled","Disabled","Disabled"],"prev":{"index":4,"_p":null},"exploring":true}},{"Schedule":{"preemptions":0,"initial_active":0,"threads":["Active","Disabled","Disabled","Disabled","Disabled"],"prev":{"index":5,"_p":null},"exploring":true}},{"Schedule":{"preemptions":0,"initial_active":0,"threads":["Active","Pending","Skip","Disabled","Disabled"],"prev":{"index":6,"_p":null},"exploring":true}},{"Schedule":{"preemptions":0,"initial_active":0,"threads":["Visited","Active","Pending","Disabled","Disabled"],"prev":{"index":7,"_p":null},"exploring":true}},{"Schedule":{"preemptions":1,"initial_active":1,"threads":["Pending","Active","Skip","Disabled","Disabled"],"prev":{"index":8,"_p":null},"exploring":true}},{"Schedule":{"preemptions":1,"initial_active":1,"threads":["Disabled","Active","Skip","Disabled","Disabled"],"prev":{"index":9,"_p":null},"exploring":true}},{"Load":{"values":[0,0,0,0,0,0,0],"pos":0,"len":1,"exploring":true}},{"Schedule":{"preemptions":1,"initial_active":1,"threads":["Disabled","Active","Skip","Disabled","Disabled"],"prev":{"index":10,"_p":null},"exploring":true}},{"Load":{"values":[0,0,0,0,0,0,0],"pos":0,"len":1,"exploring":true}},{"Schedule":{"preemptions":1,"initial_active":1,"threads":["Disabled","Active","Skip","Disabled","Disabled"],"prev":{"index":12,"_p":null},"exploring":true}},{"Schedule":{"preemptions":1,"initial_active":1,"threads":["Disabled","Active","Pending","Disabled","Disabled"],"prev":{"index":14,"_p":null},"exploring":true}},{"Schedule":{"preemptions":1,"initial_active":1,"threads":["Active","Visited","Pending","Disabled","Disabled"],"prev":{"index":15,"_p":null},"exploring":true}},{"Schedule":{"preemptions":2,"initial_active":0,"threads":["Active","Disabled","Skip","Disabled","Disabled"],"prev":{"index":16,"_p":null},"exploring":true}},{"Load":{"values":[1,0,0,0,0,0,0],"pos":0,"len":1,"exploring":true}},{"Schedule":{"preemptions":2,"initial_active":0,"threads":["Active","Disabled","Skip","Disabled","Disabled"],"prev":{"index":17,"_p":null},"exploring":true}},{"Load":{"values":[0,0,0,0,0,0,0],"pos":0,"len":1,"exploring":true}},{"Schedule":{"preemptions":2,"initial_active":0,"threads":["Active","Disabled","Pending","Disabled","Disabled"],"prev":{"index":19,"_p":null},"exploring":true}},{"Schedule":{"preemptions":2,"initial_active":0,"threads":["Active","Disabled","Skip","Disabled","Disabled"],"prev":{"index":21,"_p":null},"exploring":true}},{"Load":{"values":[1,0,0,0,0,0,0],"pos":0,"len":1,"exploring":true}},{"Schedule":{"preemptions":2,"initial_active":0,"threads":["Active","Disabled","Skip","Disabled","Disabled"],"prev":{"index":22,"_p":null},"exploring":true}},{"Schedule":{"preemptions":2,"initial_active":0,"threads":["Active","Disabled","Skip","Disabled","Disabled"],"prev":{"index":24,"_p":null},"exploring":true}},{"Load":{"values":[0,0,0,0,0,0,0],"pos":0,"len":1,"exploring":true}},{"Schedule":{"preemptions":2,"initial_active":0,"threads":["Active","Disabled","Skip","Disabled","Disabled"],"prev":{"index":25,"_p":null},"exploring":true}},{"Schedule":{"preemptions":2,"initial_active":0,"threads":["Active","Disabled","Skip","Disabled","Disabled"],"prev":{"index":27,"_p":null},"exploring":true}},{"Schedule":{"preemptions":2,"initial_active":0,"threads":["Active","Disabled","Skip","Disabled","Disabled"],"prev":{"index":28,"_p":null},"exploring":true}},{"Schedule":{"preemptions":2,"initial_active":0,"threads":["Active","Pending","Skip","Disabled","Disabled"],"prev":{"index":29,"_p":null},"exploring":true}},{"Schedule":{"preemptions":2,"initial_active":0,"threads":["Active","Pending","Pending","Disabled","Disabled"],"prev":{"index":30,"_p":null},"exploring":true}},{"Schedule":{"preemptions":2,"initial_active":0,"threads":["Visited","Visited","Active","Disabled","Disabled"],"prev":{"index":31,"_p":null},"exploring":true}},{"Schedule":{"preemptions":3,"initial_active":2,"threads":["Skip","Skip","Active","Disabled","Disabled"],"prev":{"index":32,"_p":null},"exploring":true}},{"Load":{"values":[0,1,2,0,0,0,0],"pos":0,"len":3,"exploring":true}},{"Schedule":{"preemptions":3,"initial_active":2,"threads":["Pending","Pending","Active","Disabled","Disabled"],"prev":{"index":33,"_p":null},"exploring":true}},{"Schedule":{"preemptions":3,"initial_active":2,"threads":["Pending","Pending","Active","Disabled","Disabled"],"prev":{"index":35,"_p":null},"exploring":true}},{"Schedule":{"preemptions":3,"initial_active":2,"threads":["Pending","Pending","Active","Disabled","Disabled"],"prev":{"index":36,"_p":null},"exploring":true}},{"Schedule":{"preemptions":3,"initial_active":2,"threads":["Pending","Pending","Active","Disabled","Disabled"],"prev":{"index":37,"_p":null},"exploring":true}},{"Schedule":{"preemptions":3,"initial_active":2,"threads":["Pending","Pending","Active","Disabled","Disabled"],"prev":{"index":38,"_p":null},"exploring":true}},{"Schedule":{"preemptions":3,"initial_active":2,"threads":["Pending","Skip","Active","Disabled","Disabled"],"prev":{"index":39,"_p":null},"exploring":true}},{"Schedule":{"preemptions":3,"initial_active":null,"threads":["Visited","Active","Disabled","Disabled","Disabled"],"prev":{"index":40,"_p":null},"exploring":true}}]},"exploring":true,"skipping":false,"exploring_on_start":true}

rtic-sync/src/arbiter.rs

@@ -60,6 +60,7 @@ unsafe impl<T> Sync for Arbiter<T> {}
 
 impl<T> Arbiter<T> {
     /// Create a new arbiter.
+    #[cfg(not(loom))]
     pub const fn new(inner: T) -> Self {
         Self {
             wait_queue: WaitQueue::new(),
@@ -382,6 +383,7 @@ pub mod i2c {
 }
 
 #[cfg(test)]
+#[cfg(not(loom))]
 mod tests {
     use super::*;