Replace static mut for CORE1_STACK

[jannic]

Both in our docs and in some examples, we suggest using a static mut for allocating memory for core1's stack:

  static mut CORE1_STACK: Stack<SIZE: 4096> = Stack::new();
[...]
    core1.spawn(unsafe { &mut CORE1_STACK.mem }, [...] )

With rust 1.83, this will cause a warning (at least it does with current beta), and with edition 2024 the lint will become deny-by-default. (https://doc.rust-lang.org/nightly/edition-guide/rust-2024/static-mut-references.html)

Therefore, we should suggest something else. Maybe we should also change the signature of multicore::Core::spawn to take a pointer instead of a &'static mut for the stack?

[thejpster]

Yeah we shouldn't use a reference here - a pointer is fine. But not a *const u8 because it might not be sufficiently aligned.

For the global a UnsafeCell with a MaybeUninit makes sense because we don't need to init the stack to anything.

[9names]

Yeah we shouldn't use a reference here - a pointer is fine. But not a *const u8 because it might not be sufficiently aligned.

The Stack struct itself ensures the address is sufficiently aligned, is that not enough?

[thejpster]

If the argument is a pointer to a u8, what forces them to use the Stack struct?

[jannic]

If the argument is a pointer to a u8, what forces them to use the Stack struct?

Well if the spawn function takes a pointer it doesn't really matter if it's a *mut u8 or a *mut Stack<SIZE>: Casting pointers is safe, so the function signature doesn't guarantee that the pointer points to anything sensible.

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=74374413e46fd1078eb93c58c381e2ae

So that would be an argument to find a different solution, where spawn gets passed some reference or struct that actually guarantees alignment. It's not like the current function signature is wrong. It's just that the most obvious way to get a reference with a static lifetime, using a static mut, is dangerous and deprecated.

[9names]

We could make multicore::spawn() fallible (and return Err if not aligned), or keep it infallible and panic instead?

[jannic]

As I understand it, as soon as the function takes a pointer, it must be unsafe, as a pointer could be anything, and there's no feasible runtime check to ensure it's valid. (Alignment is not the only issue.)

And once we make it unsafe, we are technically free to just assume it's properly aligned, given we document the function accordingly. Sure, panicking if we can detect an invalid pointer would be a nice safeguard.

I would prefer a safe API where the type system guarantees a valid argument, like we currently have.

[thejpster]

So, something like:

use std::sync::atomic::Ordering;
use std::sync::atomic::AtomicBool;
use std::mem::MaybeUninit;
use std::cell::UnsafeCell;

#[repr(C, align(65536))]
struct Stack<const N: usize> {
   inner: UnsafeCell<MaybeUninit<[u8; N]>>,
   taken: AtomicBool
}

unsafe impl<const N: usize> Sync for Stack<N> {}

impl<const N: usize> Stack<N> {
    const fn new() -> Stack<N> {
        Stack {
            inner: UnsafeCell::new(unsafe { MaybeUninit::zeroed().assume_init() }),
            taken: AtomicBool::new(false),
        }
    }
    
    fn get(&self) -> *const u8 {
        // I guess if you have CAS, you could do that
        if self.taken.load(Ordering::Relaxed) {
            panic!("Stack taken");
        }
        self.taken.store(true, Ordering::Relaxed);
        (&raw const self.inner).cast()
    }
}

fn spawn<const N: usize>(stack: &Stack<N>) {
    println!("Buffer starts at {:p}", stack.get())
}

static STACK: Stack<8192> = Stack::new();

fn main() {
    spawn(&STACK);
}

[jannic]

Yes, for example.
I implemented it very similar in a proof-of-concept I wrote:

use std::cell::Cell;
use std::cell::UnsafeCell;

// Mock implementation so this snippet is executable
mod critical_section {
    pub fn with<T>(f: impl FnOnce(()) -> T) ->T {
        f(())
    }
}

#[repr(C, align(32))]
pub struct Stack<const SIZE: usize> {
    /// Memory to be used for the stack
    mem: UnsafeCell<[usize; SIZE]>,
    taken: Cell<bool>,
}

unsafe impl<const SIZE: usize> Sync for Stack<SIZE> {}

impl<const SIZE: usize> Stack<SIZE> {
    /// Construct a stack of length SIZE, initialized to 0
    pub const fn new() -> Stack<SIZE> {
        Stack {
            mem: UnsafeCell::new([0; SIZE]),
            taken: Cell::new(false),
        }
    }

    /// Get a mutable reference to the contained memory.
    ///
    /// Only succeeds once. Subsequent calls will return an empty option.
    pub fn take(&self) -> Option<&mut [usize; SIZE]> {
        let taken = critical_section::with(|_| self.taken.replace(true));
        if taken {
            None
        } else {
            // Safety: Above code ensures that this can only be executed once.
            Some(unsafe { &mut *self.mem.get() })
        }
    }
}

static CORE1_STACK: Stack<4096> = Stack::new();

fn spawn(stack: &'static mut [usize]) {
    println!("Buffer starts at {:p}", stack)
}

fn main() {
    spawn(CORE1_STACK.take().unwrap());
}

Advantage:

• No need to change the signature of spawn.
• Doesn't require AtomicBool

Disadvantage:

• Only guarantees 4-byte alignment in the function signature. (8-byte alignment for the stack poitner and 32 byte alignment for stack guard is already implemented inside spawn, possibly wasting some bytes.

BTW, passing a &mut into spawn and then writing into that memory using pointer writes is probably UB. This is not a regression but an existing bug we should care about anyway.

[jannic]

BTW, passing a &mut into spawn and then writing into that memory using pointer writes is probably UB. This is not a regression but an existing bug we should care about anyway.

Also note that if this assessment is correct, we need to change the signature of spawn anyway to make it sound.

[thejpster]

I'm not sure I'm comfortable passing around an &mut [u8], given the second core will be writing to the same memory in the background. I think it's better to pass around a &Stack and get the pointer out of it right when you hand the pointer to Core 1 to use as the stack pointer.

[jannic]

Yes, that's exactly what I meant with "Also note that if this assessment is correct, we need to change the signature of spawn anyway to make it sound."

While it would be nice to not break the current API, I don't think this can't be sound. So we have to change it.

Still thinking about the best alternative. But yes, it probably needs to be some struct that's passed by shared reference.

[jannic]

How about such an interface?

pub struct Stack<const SIZE: usize> {
    [private fields]
}

unsafe impl<const SIZE: usize> Sync for Stack<SIZE> {}

impl<const SIZE: usize> Stack<SIZE> {
    /// Construct a stack of length SIZE, initialized to 0
    pub const fn new() -> Stack<SIZE> {
        [...]
    }

    /// Take the StackAllocation out of this Stack.
    ///
    /// This returns None if the stack is already taken.
    pub fn take(&self) -> Option<StackAllocation> {
        [...]
    }
}

/// This object represents a memory area which can be used for a stack.
///
/// It is essentially a range of pointers with these additional guarantees:
/// The begin / end pointers must define a stack
/// with proper alignment (at least 8 bytes, preferably 32 bytes)
/// and sufficient size (TBD bytes). The underlying memory must
/// have a static lifetime. No references to that memory must exist.
/// Therefore, a function that gets passed such an object is free to write
/// to arbitrary memory locations in the range, and may continue to do
/// so after dropping this object.
pub struct StackAllocation {
    mem: Range<*mut usize>,  // private field, so this object can't be created directly
}

impl StackAllocation {
    fn get(&self) -> Range<*mut usize> {
        self.mem.clone()
    }

    /// Unsafely construct a stack allocation
    ///
    /// This is mainly useful to construct a stack allocation in some memory region
    /// defined in a linker script, for example to place the stack in the SRAM4/5 regions.
    ///
    /// # Safety
    ///
    /// The caller must ensure that the guarantees that a StackAllocation provides
    /// are upheld.
    pub unsafe fn from_raw_parts(start: *mut usize, end: *mut usize) -> Self {
        StackAllocation { mem: start..end }
    }
}

[...]

    pub fn spawn<F>(&mut self, stack: StackAllocation, entry: F) -> Result<(), Error>
    where
        F: FnOnce() + Send + 'static,

So spawn gets an owned StackAllocation, guaranteeing exclusive access to the memory range. Such an allocation can be easily created without unsafe user code:

static CORE1_STACK: Stack<4096> = Stack::new();
[...]
    core1.spawn(CORE1_STACK.take().unwrap(), move || { [...] })

With this approach, it's also possible to place the stack in RAM4 by defining appropriate linker symbols and calling unsafe { StackAllocation::from_raw_parts(sram4_start, sram4_end) }. This obviously requires some unsafe code, but it provides flexibility if one wants to start with memory not managed by rust for some reason.

Design considerations:

• No static mut (of course, as that was the main goal).
• No unsafe user code required for the standard case.
• There's never a reference to the underlying memory, avoiding UB by violating aliasing rules. (The shared pointer to Stack fine, as implicitly used when calling CORE1_STACK.take(), is fine due to the way UnsafeCell works).
• Once spawn is called, the stack can't be reused (without unsafe code).
• When creating a StackAllocation manually (and unsafely), there's no overhead for the taken flag. So eg. a 4k stack fits into RAM4. (#[link_section(.ram4)] static CORE1_STACK: Stack<4096> = Stack::new(); wouldn't work as that's slightly bigger than 4k)
• Minimal changes to existing code:

-static mut CORE1_STACK: Stack<4096> = Stack::new();
+static CORE1_STACK: Stack<4096> = Stack::new();

-        core1.spawn(unsafe { &mut CORE1_STACK.mem }, move || {
+        core1.spawn(CORE1_STACK.take().unwrap(), move || {

• If one is uncomfortable with the possible panic due to unwrap, it's possible to write alternative error handling. In simple cases where spawn happens early in main, the unwrap is most likely fine.

[thejpster]

I love it.