Make task-hiffy less iffy

task/hiffy/Cargo.toml

@@ -4,7 +4,6 @@ version = "0.1.0"
 edition = "2024"
 
 [dependencies]
-armv6m-atomic-hack = { path = "../../lib/armv6m-atomic-hack" }
 drv-hf-api = { path = "../../drv/hf-api", optional = true }
 drv-hash-api = { path = "../../drv/hash-api", optional = true }
 drv-i2c-api = { path = "../../drv/i2c-api" }

task/hiffy/src/generic.rs

@@ -17,8 +17,7 @@ pub enum Functions {
 }
 
 #[unsafe(no_mangle)]
-#[used(compiler)]
-static HIFFY_FUNCTIONS: Option<&Functions> = None;
+pub static HIFFY_FUNCTIONS: Option<&Functions> = None;
 
 pub(crate) static HIFFY_FUNCS: &[Function] = &[
     crate::common::sleep,

task/hiffy/src/lpc55.rs

@@ -382,8 +382,8 @@ pub(crate) static HIFFY_FUNCS: &[Function] = &[
 // This definition forces the compiler to emit the DWARF needed for debuggers
 // to be able to know function indices, arguments and return values.
 //
-#[used(compiler)]
-static HIFFY_FUNCTIONS: Option<&Functions> = None;
+#[unsafe(no_mangle)]
+pub static HIFFY_FUNCTIONS: Option<&Functions> = None;
 
 pub(crate) fn trace_execute(offset: usize, op: hif::Op) {
     ringbuf_entry!(Trace::Execute((offset, op)));

task/hiffy/src/main.rs

@@ -14,21 +14,7 @@
 
 #![no_std]
 #![no_main]
-//
-// TODO: Hiffy is using unsafe and static mut in ways that are not obviously
-// sound. This became a warning in early 2024. In the interest of preventing
-// regressions in everything _else_ I'm suppressing the warning here so we can
-// turn Clippy back on. If you're reading this, this file is potentially unsound
-// and needs attention!
-//
-#![allow(static_mut_refs)]
-// This is necessary in order to use the `#[used(compiler)]` attribute on Hiffy
-// statics which are written to by Humility, and must not be optimized out.
-#![feature(used_with_arg)]
-
-// This trait may not be needed, if compiling for a non-armv6m target.
-#[allow(unused_imports)]
-use armv6m_atomic_hack::AtomicU32Ext;
+
 use core::sync::atomic::{AtomicU32, Ordering};
 use hif::*;
 use static_cell::*;
@@ -107,7 +93,8 @@ cfg_if::cfg_if! {
 // - [`HIFFY_TEXT`]       => Program text for HIF operations
 // - [`HIFFY_DATA`]       => Binary data from the caller
 // - [`HIFFY_RSTACK`]     => HIF return stack
-// - [`HIFFY_SCRATCH`]    => Scratch space for hiffy functions
+// - [`HIFFY_SCRATCH`]    => Scratch space for hiffy functions; debugger reads
+//                           its size but does not modify it
 // - [`HIFFY_REQUESTS`]   => Count of succesful requests
 // - [`HIFFY_ERRORS`]     => Count of HIF execution failures
 // - [`HIFFY_FAILURE`]    => Most recent HIF failure, if any
@@ -116,35 +103,87 @@ cfg_if::cfg_if! {
 // - [`HIFFY_READY`]      => Variable that will be non-zero iff the HIF
 //                           execution engine is waiting to be kicked
 //
-static mut HIFFY_TEXT: [u8; HIFFY_TEXT_SIZE] = [0; HIFFY_TEXT_SIZE];
-static mut HIFFY_DATA: [u8; HIFFY_DATA_SIZE] = [0; HIFFY_DATA_SIZE];
-static mut HIFFY_RSTACK: [u8; HIFFY_RSTACK_SIZE] = [0; HIFFY_RSTACK_SIZE];
+// We are making the following items "no mangle" and "pub" to hint to the
+// compiler that they are "exposed", and may be written (spookily) outside the
+// scope of Rust itself. The aim is to prevent the optimizer from *assuming* the
+// contents of these buffers will remain unchanged between accesses, as they
+// will be written directly by the debugger.
+//
+// Below, we use atomic ordering (e.g. Acquire and Release) to inhibit
+// compile- and run-time re-ordering around the explicit sequencing performed
+// by the HIFFY_READY, HIFFY_KICK, HIFFY_REQUESTS, and HIFFY_ERRORS that are
+// used to arbitrate shared access between the debugger and this software task.
+//
+// We assume that Hubris and Humility are cooperating, using the following state
+// machines to avoid conflicting accesses:
+// ┌─────────────────────────────────────────────────────────────────────────────────┐
+// │                                                                                 │
+// │                                        KICK == 0                                │
+// │                    ┌────────────────────────────────────────────────┐           │
+// │                    │                                                │           │
+// │                    │                                       ┌─────────────────┐  │
+// │  ┌─────────┐       ▽  ┌─────────────────┐     ┌───────┐    │ Write READY = 0 │  │
+// │  │ Startup │──┬────┴─▷│ Write READY = 1 │────▷│ Sleep │───▷│ Read KICK       │  │
+// │  └─────────┘  │       └─────────────────┘     └───────┘    │                 │  │
+// │               │                                            └─────────────────┘  │
+// │               │                                                     │           │
+// │               │  ┌───────────────────────────────┐                  ▽           │
+// │               │  │ Read REQUESTS                 │ Success ┌─────────────────┐  │
+// │               ├──│ Write REQUESTS = REQUESTS + 1 │◁────┐   │ Write KICK = 0  │  │
+// │               │  └───────────────────────────────┘     ├───│ Execute script  │  │
+// │               │  ┌───────────────────────────────┐     │   │                 │  │
+// │               │  │ Read ERRORS                   │     │   └─────────────────┘  │
+// │               └──│ Write ERRORS = ERRORS + 1     │◁────┘                        │
+// │                  └───────────────────────────────┘ Failure                      │
+// │ ┌────────────┐                                                                  │
+// └─┤ Hiffy Task ├──────────────────────────────────────────────────────────────────┘
+//   └────────────┘
+// ┌─────────────────────────────────────────────────────────────────────────────────┐
+// │                                                                                 │
+// │                                ┌────────────────┐                ┌────────┐     │
+// │          ┌────────┐ READY == 1 │ Read REQUEST   │   REQUEST or   │ Read   │     │
+// │        ┌▷│  Idle  │───────────▷│ Read ERRORS    │───────────────▷│ RESULT │     │
+// │        │ └────────┘            │ Write KICK = 1 │ ERRORS changed │        │     │
+// │        │                       └────────────────┘                └────────┘     │
+// │        │                                                              │         │
+// │        └──────────────────────────────────────────────────────────────┘         │
+// │ ┌──────────┐                                                                    │
+// └─┤ Humility ├────────────────────────────────────────────────────────────────────┘
+//   └──────────┘
+#[unsafe(no_mangle)]
+pub static mut HIFFY_TEXT: [u8; HIFFY_TEXT_SIZE] = [0; HIFFY_TEXT_SIZE];
+#[unsafe(no_mangle)]
+pub static mut HIFFY_DATA: [u8; HIFFY_DATA_SIZE] = [0; HIFFY_DATA_SIZE];
+#[unsafe(no_mangle)]
+pub static mut HIFFY_RSTACK: [u8; HIFFY_RSTACK_SIZE] = [0; HIFFY_RSTACK_SIZE];
 
-static HIFFY_SCRATCH: StaticCell<[u8; HIFFY_SCRATCH_SIZE]> =
+pub static HIFFY_SCRATCH: StaticCell<[u8; HIFFY_SCRATCH_SIZE]> =
     StaticCell::new([0; HIFFY_SCRATCH_SIZE]);
 
-#[used]
-static HIFFY_REQUESTS: AtomicU32 = AtomicU32::new(0);
-#[used]
-static HIFFY_ERRORS: AtomicU32 = AtomicU32::new(0);
-#[used]
-static HIFFY_KICK: AtomicU32 = AtomicU32::new(0);
-#[used]
-static HIFFY_READY: AtomicU32 = AtomicU32::new(0);
+#[unsafe(no_mangle)]
+pub static HIFFY_REQUESTS: AtomicU32 = AtomicU32::new(0);
+#[unsafe(no_mangle)]
+pub static HIFFY_ERRORS: AtomicU32 = AtomicU32::new(0);
+#[unsafe(no_mangle)]
+pub static HIFFY_KICK: AtomicU32 = AtomicU32::new(0);
+#[unsafe(no_mangle)]
+pub static HIFFY_READY: AtomicU32 = AtomicU32::new(0);
 
-#[used]
-static mut HIFFY_FAILURE: Option<Failure> = None;
+#[unsafe(no_mangle)]
+pub static mut HIFFY_FAILURE: Option<Failure> = None;
 
-///
-/// We deliberately export the HIF version numbers to allow Humility to
-/// fail cleanly if its HIF version does not match our own.
-///
+// We deliberately export the HIF version numbers to allow Humility to
+// fail cleanly if its HIF version does not match our own.
+//
+// Note that `#[unsafe(no_mangle)]` does not preserve these values through the
+// linker, so we used `#[used]` instead.  They are not used by any code, so
+// there's no safety concerns.
 #[used]
-static HIFFY_VERSION_MAJOR: AtomicU32 = AtomicU32::new(HIF_VERSION_MAJOR);
+pub static HIFFY_VERSION_MAJOR: AtomicU32 = AtomicU32::new(HIF_VERSION_MAJOR);
 #[used]
-static HIFFY_VERSION_MINOR: AtomicU32 = AtomicU32::new(HIF_VERSION_MINOR);
+pub static HIFFY_VERSION_MINOR: AtomicU32 = AtomicU32::new(HIF_VERSION_MINOR);
 #[used]
-static HIFFY_VERSION_PATCH: AtomicU32 = AtomicU32::new(HIF_VERSION_PATCH);
+pub static HIFFY_VERSION_PATCH: AtomicU32 = AtomicU32::new(HIF_VERSION_PATCH);
 
 #[unsafe(export_name = "main")]
 fn main() -> ! {
@@ -153,20 +192,13 @@ fn main() -> ! {
     let mut stack = [None; 32];
     const NLABELS: usize = 4;
 
-    //
-    // Sadly, there seems to be no other way to force these variables to
-    // not be eliminated...
-    //
-    HIFFY_VERSION_MAJOR.fetch_add(0, Ordering::SeqCst);
-    HIFFY_VERSION_MINOR.fetch_add(0, Ordering::SeqCst);
-    HIFFY_VERSION_PATCH.fetch_add(0, Ordering::SeqCst);
-
     loop {
-        HIFFY_READY.fetch_add(1, Ordering::SeqCst);
+        HIFFY_READY.store(1, Ordering::Relaxed);
         hl::sleep_for(sleep_ms);
-        HIFFY_READY.fetch_sub(1, Ordering::SeqCst);
+        HIFFY_READY.store(0, Ordering::Relaxed);
 
-        if HIFFY_KICK.load(Ordering::SeqCst) == 0 {
+        // Humility writes `1` to `HIFFY_KICK`
+        if HIFFY_KICK.load(Ordering::Acquire) == 0 {
             sleeps += 1;
 
             // Exponentially backoff our sleep value, but no more than 250ms
@@ -182,45 +214,92 @@ fn main() -> ! {
         // Whenever we have been kicked, we adjust our timeout down to 1ms,
         // from which we will exponentially backoff
         //
-        HIFFY_KICK.fetch_sub(1, Ordering::SeqCst);
+        HIFFY_KICK.store(0, Ordering::Release);
         sleep_ms = 1;
         sleeps = 0;
 
-        // TODO without a safety comment explaining why these are safe, it is
-        // not clear if this is sound, do _not_ "fix" this by slapping on an
-        // addr_of_mut! without further analysis!
-        let text = unsafe { &HIFFY_TEXT };
-        let data = unsafe { &HIFFY_DATA };
-        let rstack = unsafe { &mut HIFFY_RSTACK[0..] };
-
         let check = |offset: usize, op: &Op| -> Result<(), Failure> {
             trace_execute(offset, *op);
             Ok(())
         };
 
-        let rv = execute::<_, NLABELS>(
-            text,
-            HIFFY_FUNCS,
-            data,
-            &mut stack,
-            rstack,
-            &mut *HIFFY_SCRATCH.borrow_mut(),
-            check,
-        );
+        let rv = {
+            // Dummy object to bind references to a non-static lifetime
+            let lifetime = ();
+
+            // SAFETY: We construct references from our pointers with a limited
+            // (non-static) lifetime, so they can't escape this block.  We are
+            // in single-threaded code, so no one else can read or write to
+            // static memory.  While the HIF program is running, the debugger is
+            // only reading from `HIFFY_REQUESTS` and `HIFFY_ERRORS`; it is not
+            // writing to any locations in memory.  See the diagram above for
+            // Hubris / Humility coordination.
+            let (text, data, rstack) = unsafe {
+                (
+                    bind_lifetime_ref(&lifetime, &raw const HIFFY_TEXT),
+                    bind_lifetime_ref(&lifetime, &raw const HIFFY_DATA),
+                    bind_lifetime_mut(&lifetime, &raw mut HIFFY_RSTACK),
+                )
+            };
+            execute::<_, NLABELS>(
+                text,
+                HIFFY_FUNCS,
+                data,
+                &mut stack,
+                rstack,
+                &mut *HIFFY_SCRATCH.borrow_mut(),
+                check,
+            )
+        };
 
         match rv {
             Ok(_) => {
-                HIFFY_REQUESTS.fetch_add(1, Ordering::SeqCst);
+                let prev = HIFFY_REQUESTS.load(Ordering::Relaxed);
+                HIFFY_REQUESTS.store(prev.wrapping_add(1), Ordering::Release);
                 trace_success();
             }
             Err(failure) => {
-                HIFFY_ERRORS.fetch_add(1, Ordering::SeqCst);
+                // SAFETY: We are in single-threaded code and the debugger will
+                // not be reading HIFFY_FAILURE until HIFFY_ERRORS is
+                // incremented below.  See the diagram above for Hubris /
+                // Humility coordination.
                 unsafe {
                     HIFFY_FAILURE = Some(failure);
                 }
-
+                let prev = HIFFY_ERRORS.load(Ordering::Relaxed);
+                HIFFY_ERRORS.store(prev.wrapping_add(1), Ordering::Release);
                 trace_failure(failure);
             }
         }
     }
 }
+
+/// Converts an array pointer to a shared reference with a particular lifetime
+///
+/// # Safety
+/// `ptr` must point to a valid, aligned, initialized `[u8; N]`.
+/// The referent must not be mutated while the returned reference is live.
+#[expect(clippy::needless_lifetimes)] // gotta make it obvious
+unsafe fn bind_lifetime_ref<'a, const N: usize>(
+    _: &'a (),
+    array: *const [u8; N],
+) -> &'a [u8; N] {
+    // SAFETY: converting from pointer to reference is safe given the function's
+    // safety conditions (listed in docstring)
+    unsafe { array.as_ref().unwrap_lite() }
+}
+
+/// Converts an array pointer to a mutable reference with a particular lifetime
+///
+/// # Safety
+/// `ptr` must point to a valid, aligned, initialized `[u8; N]`.
+/// The referent must not be mutated while the returned reference is live.
+#[expect(clippy::needless_lifetimes, clippy::mut_from_ref)]
+unsafe fn bind_lifetime_mut<'a, const N: usize>(
+    _: &'a (),
+    array: *mut [u8; N],
+) -> &'a mut [u8; N] {
+    // SAFETY: converting from pointer to reference is safe given the function's
+    // safety conditions (listed in docstring)
+    unsafe { array.as_mut().unwrap_lite() }
+}

task/hiffy/src/stm32g0.rs

@@ -460,8 +460,7 @@ pub(crate) static HIFFY_FUNCS: &[Function] = &[
 // to be able to know function indices, arguments and return values.
 //
 #[unsafe(no_mangle)]
-#[used(compiler)]
-static HIFFY_FUNCTIONS: Option<&Functions> = None;
+pub static HIFFY_FUNCTIONS: Option<&Functions> = None;
 
 pub(crate) fn trace_execute(_offset: usize, _op: hif::Op) {}
 

task/hiffy/src/stm32h7.rs

@@ -583,8 +583,7 @@ pub(crate) static HIFFY_FUNCS: &[Function] = &[
 // to be able to know function indices, arguments and return values.
 //
 #[unsafe(no_mangle)]
-#[used(compiler)]
-static HIFFY_FUNCTIONS: Option<&Functions> = None;
+pub static HIFFY_FUNCTIONS: Option<&Functions> = None;
 
 pub(crate) fn trace_execute(offset: usize, op: hif::Op) {
     ringbuf_entry!(Trace::Execute((offset, op)));

task/hiffy/src/tests.rs

@@ -101,8 +101,7 @@ pub(crate) static HIFFY_FUNCS: &[Function] = &[run_a_test];
 // to be able to know function indices, arguments and return values.
 //
 #[unsafe(no_mangle)]
-#[used]
-static HIFFY_FUNCTIONS: Option<&Functions> = None;
+pub static HIFFY_FUNCTIONS: Option<&Functions> = None;
 
 pub(crate) fn trace_execute(offset: usize, op: hif::Op) {
     ringbuf_entry!(Trace::Execute((offset, op)));