Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Shared references of mutable static warnings #3841

Open
4 tasks
valexandru opened this issue Feb 9, 2024 · 8 comments · May be fixed by #3945
Open
4 tasks

Shared references of mutable static warnings #3841

valexandru opened this issue Feb 9, 2024 · 8 comments · May be fixed by #3945
Labels

Comments

@valexandru
Copy link
Contributor

valexandru commented Feb 9, 2024

While compiling tock with the latest nightly version of rust, nightly-2024-02-08, I encounter 25 warnings regarding the usage of shared references of mutable static in folders such as arch/, kernel/, chips/ and boards/ as it can be seen bellow:

warning: shared reference of mutable static is discouraged
   --> kernel/src/deferred_call.rs:145:28
    |
145 |         let ctr = unsafe { &CTR };
    |                            ^^^^ shared reference of mutable static
    |
    = note: for more information, see issue #114447 <https://github.com/rust-lang/rust/issues/114447>
    = note: reference of mutable static is a hard error from 2024 edition
    = note: mutable statics can be written to by multiple threads: aliasing violations or data races will cause undefined behavior
    = note: `#[warn(static_mut_ref)]` on by default
help: shared references are dangerous since if there's any kind of mutation of that static while the reference lives, that's UB; use `addr_of!` instead to create a raw pointer
    |
145 |         let ctr = unsafe { addr_of!(CTR) };

If I understand correctly, for the moment this is just a warning, but it will soon become a compile error. We will need some workarounds for this.

Todo

  • Decide on an approach for avoiding the warning or more safely handling mutable statics.
  • Fix deferred call
  • Fix non-boards uses
  • Handle PROCESSES, CHIP, etc. in boards.
@bradjc
Copy link
Contributor

bradjc commented Feb 9, 2024

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
Copy link
Contributor

bradjc commented Feb 16, 2024

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
Copy link
Contributor

bradjc commented Feb 21, 2024

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

@bradjc
Copy link
Contributor

bradjc commented Feb 21, 2024

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
Copy link
Member

lschuermann commented Feb 27, 2024

@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
Copy link
Contributor

bradjc commented Mar 6, 2024

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.

@kupiakos
Copy link
Contributor

kupiakos commented Mar 6, 2024

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.

@lschuermann
Copy link
Member

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.

@alevy alevy linked a pull request Mar 31, 2024 that will close this issue
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants