Shared references of mutable static warnings

[valexandru]

Shared references of mutable static warnings

[bradjc]

I opened #3842 to help track this.

I think we need to finish up #3824 first to take care of those. In general all of the statics in boards/ we probably want to keep.

For the others it might be worth evaluating whether we actually want them, and documenting them if so.

[bradjc]

I rebased #3842 after merging #3824, and, uh, we are no where close to being able to update the nightly.

[kupiakos]

Don't use as_ref when the raw pointer cannot be null - there is no point. Use a pointer dereference.

diff --git a/kernel/src/deferred_call.rs b/kernel/src/deferred_call.rs
index cdac8330d..e21137ab8 100644
--- a/kernel/src/deferred_call.rs
+++ b/kernel/src/deferred_call.rs
@@ -142,7 +142,7 @@ impl DeferredCall {
     pub fn new() -> Self {
         // SAFETY: No accesses to CTR are via an &mut, and the Tock kernel is
         // single-threaded so all accesses will occur from this thread.
-        let ctr = unsafe { &CTR };
+        let ctr = unsafe { &*core::ptr::addr_of!(CTR) };
         let idx = ctr.get() + 1;
         ctr.set(idx);
         DeferredCall { idx }

I think the best balance here would be a newtype implementing Sync that is zero-cost on chip but can perform basic soundness checks on host like ensuring that you only access from a single thread.

[bradjc]

We definitely need to do something about these (otherwise we cannot update nightly). We would like to keep track of where these warnings are happening and see if we can fix the underlying issue.

I wonder if we should create our own macro which wraps addr_of!() so we can mark static mut uses that we would like to investigate further.

[bradjc]

One workaround are diffs that look like:

diff --git a/kernel/src/deferred_call.rs b/kernel/src/deferred_call.rs
index cdac8330d..e21137ab8 100644
--- a/kernel/src/deferred_call.rs
+++ b/kernel/src/deferred_call.rs
@@ -142,7 +142,7 @@ impl DeferredCall {
     pub fn new() -> Self {
         // SAFETY: No accesses to CTR are via an &mut, and the Tock kernel is
         // single-threaded so all accesses will occur from this thread.
-        let ctr = unsafe { &CTR };
+        let ctr = unsafe { core::ptr::addr_of!(CTR).as_ref().unwrap() };
         let idx = ctr.get() + 1;
         ctr.set(idx);
         DeferredCall { idx }

which seems, undesirable.

[lschuermann]

@bradjc I do, unfortunately, think that your posted diff is more or less the way to go here. We're not going to be able to rid ourselves of all usages of mutable statics, in particular in code paths like DeferredCall or the Cortex-M fault handler state variables.

We can think about introducing a special type of Cell for this purpose, not unlike LocalCell proposed here: https://github.com/KizzyCode/embedded-threadsafe-rust/blob/master/embedded-threadsafe/src/safecells/local.rs (albeit without dynamic checks for thread ID or interrupt contexts)

I'm thinking of an interface that has a safe constructor, but unsafe accessors (getters and setters). This because, to store this LocalCell in a non-mutable static, it needs to be Sync: obviously we don't want to do any actual synchronization (expensive + many of our architectures don't have atomics), but we can put the burden of proof that it's only ever accessed from one thread at a time on the callers through unsafe. It'd effectively be a very thin wrapper around UnsafeCell, without safe abstractions as commonly provided by other Cell types.

This should have identical semantics to what we're doing now, but is avoiding unwieldy syntax, and prevents accidentally "leaking" a Rust reference to a mutable static for longer than we'd want to have it in scope, and thus would be potentially in danger of violating Rust's aliasing constraints.

@kupiakos Interested to hear your thoughts on this as well.

[bradjc]

Plan is to look into some sort of *Cell approach, or something principled. However, if there is not a solution in place by April 1, 2024, we'll move forward with the core::ptr::addr_of!(CTR).as_ref().unwrap()-esque approach so we can compile on new nightly again.

[lschuermann]

I think the best balance here would be a newtype implementing Sync that is zero-cost on chip but can perform basic soundness checks on host like ensuring that you only access from a single thread.

Great, that's exactly what I've been prototyping. Can share on a future call, will unfortunately miss the next two. Might hand off to @alevy to share instead.