Update Rust Nightly to November 16 2024

[bradjc]

Pull Request Overview

Update to a new nightly.

The changes to use Self are to fix warnings like these:

warning: elided lifetime has a name
  --> capsules/extra/src/ambient_light.rs:57:10
   |
53 | impl<'a> AmbientLight<'a> {
   |      -- lifetime `'a` declared here
...
57 |     ) -> AmbientLight {
   |          ^^^^^^^^^^^^ this elided lifetime gets resolved as `'a`

Testing Strategy

travis

TODO or Help Wanted

This adds a bunch of warnings like:

warning: `raspberry_pi_pico` (bin "raspberry_pi_pico") generated 1 warning
warning: creating a shared reference to mutable static is discouraged
   --> chips/sam4l/src/dma.rs:223:17
    |
223 |                 NUM_ENABLED.fetch_add(1, atomic::Ordering::Relaxed);
    |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ shared reference to mutable static
    |
    = note: for more information, see <https://doc.rust-lang.org/nightly/edition-guide/rust-2024/static-mut-references.html>
    = note: shared references to mutable statics are dangerous; it's undefined behavior if the static is mutated or if a mutable reference is created for it while the shared reference lives
    = note: `#[warn(static_mut_refs)]` on by default

so...we need to do something about that.

Documentation Updated

• Updated the relevant files in /docs, or no updates are required.

Ran the update script.

Formatting

• Ran make prepush.

[brghena]

Shouldn't merge this until we deal with the warnings.

[bradjc]

Wow, clippy is really asking for a lot. Sorry, I should have made those changes on the original commits.

[ppannuto]

I'm not certain I want to block this PR on this, but I think might. While there are some global shared mut's that are more complicated, we have a bunch like these which we can now stable-ly replace with core atomics, i.e. the following diff compiles fine:

diff --git a/boards/clue_nrf52840/src/io.rs b/boards/clue_nrf52840/src/io.rs
index 01ddd7b4f..981c56be6 100644
--- a/boards/clue_nrf52840/src/io.rs
+++ b/boards/clue_nrf52840/src/io.rs
@@ -6,6 +6,7 @@ use core::fmt::Write;
 use core::panic::PanicInfo;
 use core::ptr::addr_of;
 use core::ptr::addr_of_mut;
+use core::sync::atomic::{AtomicBool, Ordering};
 use kernel::ErrorCode;
 
 use kernel::debug;
@@ -13,7 +14,6 @@ use kernel::debug::IoWrite;
 use kernel::hil::led;
 use kernel::hil::uart::Transmit;
 use kernel::hil::uart::{self};
-use kernel::utilities::cells::VolatileCell;
 use nrf52840::gpio::Pin;
 
 use crate::CHIP;
@@ -36,17 +36,17 @@ impl Write for Writer {
 const BUF_LEN: usize = 512;
 static mut STATIC_PANIC_BUF: [u8; BUF_LEN] = [0; BUF_LEN];
 
-static mut DUMMY: DummyUsbClient = DummyUsbClient {
-    fired: VolatileCell::new(false),
+static DUMMY: DummyUsbClient = DummyUsbClient {
+    fired: AtomicBool::new(false),
 };
 
 struct DummyUsbClient {
-    fired: VolatileCell<bool>,
+    fired: AtomicBool,
 }
 
 impl uart::TransmitClient for DummyUsbClient {
     fn transmitted_buffer(&self, _: &'static mut [u8], _: usize, _: Result<(), ErrorCode>) {
-        self.fired.set(true);
+        self.fired.store(true, Ordering::Release);
     }
 }
 
@@ -110,13 +110,13 @@ impl IoWrite for Writer {
                         n.clear_pending();
                         n.enable();
                     }
-                    if DUMMY.fired.get() {
+                    if DUMMY.fired.load(Ordering::Acquire) {
                         // buffer finished transmitting, return so we can output additional
                         // messages when requested by the panic handler.
                         break;
                     }
                 }
-                DUMMY.fired.set(false);
+                DUMMY.fired.store(false, Ordering::Release);
             });
         }
         buf.len()

This new warning very helpfully enumerates all the cases where we can make the better fix. I feel like we should use to opportunity to either
(a) change to atomics, or
(b) document more explicitly where/why we can't

[lschuermann]

My 2ct:

• I agree that we should make these changes in principle.
• I don't think this PR should make those changes. It's large enough already, and having its diff be mostly mechanical greatly aids reviewability.
• Are atomics actually the right tool here? I think that we should refrain from using atomics for cases where we aren't actually synchronizing between different software threads.
• Will atomics not cause issues on RISC-V platforms without the a extension? (of which we have a ton!)

[lschuermann]

Ultimately, it seems like CoreLocal is the long-term, proper solution for this.

[ppannuto]

I don't think this PR should make those changes. It's large enough already, and having its diff be mostly mechanical greatly aids reviewability.

I agree. If I were to commit to pushing back, which I'm not at yet, it would be to block this PR until a different PR can go in which fixes* all of these "correctly" that this can then rebase onto.

*defined as changing code, or documenting why the workaround is necessary for that variable [probably at the declaration site]

Are atomics actually the right tool here? I think that we should refrain from using atomics for cases where we aren't actually synchronizing between different software threads.

You're right, atomics aren't the right tool here. This should just be a Cell...

                loop {
                    if let Some(interrupt) = cortexm4::nvic::next_pending() {
                        if interrupt == 39 {
                            usb.handle_interrupt(); <-- this will eventually call DUMMY.fired.set()
                        }
                        let n = cortexm4::nvic::Nvic::new(interrupt);
                        n.clear_pending();
                        n.enable();
                    }
                    if DUMMY.fired.get() {
                        // buffer finished transmitting, return so we can output additional
                        // messages when requested by the panic handler.
                        break;
                    }
                }

I don't think this needs to be a VolatileCell at all; basic interior mutability is sufficient for the use case, and the static mut is just wrong. With that in mind, I think I more strongly would want to block this PR as we're mechanically silencing warnings for real misuses (for at least N=1 cases), so we should use this warning to evaluate if all the uses are really needed.

Will atomics not cause issues on RISC-V platforms without the a extension? (of which we have a ton!)

No longer relevant, but not an issue for code in boards/

[lschuermann]

No longer relevant, but not an issue for code in boards/

Not an issue for any code in Cortex-M arch crates and Cortex-M boards, but an issue for anything that could possibly run on a RISC-V non-a platform, right?

[lschuermann]

I don't think this needs to be a VolatileCell at all; basic interior mutability is sufficient for the use case, and the static mut is just wrong. With that in mind, I think I more strongly would want to block this PR as we're mechanically silencing warnings for real misuses (for at least N=1 cases), so we should use this warning to evaluate if all the uses are really needed.

Yes. In fact, I think this is the exact issue that @alevy and I and others are trying to solve with #3945.

I'd really like to move this PR along. I'm fine with committing to prioritize #3945, but given just how much this PR changes, maintaining this diff and resolving conflicts until we solve this for good will be a lot of work. Just reviewing this massive PR took a good 30min already.

we're mechanically silencing warnings for real misuses (for at least N=1 cases),

If this refers to #4193 (comment), that'll require a major redesign of how we handle the PROCESSES array -- just switching to CoreLocal will not make this code sound. Again, something that we should definitely do, but I don't think that holding this PR up for that is warranted. This PR highlights these issues, but it's neither introducing them, nor making them any worse than they already are.

[bradjc]

I agree that the nightly PR bump isn't the place to make large changes. Static mut is a known problem and we agreed to go with the bandaid approach since the end of April and I don't think that has changed. We are still pending on the more proper fix.

Not worth blocking this for

[ppannuto]

Two small comment fixes; one I can't just hit apply on in web review though, so someone with editor needs to reflow/fix the doc comment sorry. Don't actually care about _disable.