SQUASH: changes requested in code review

Author: cbiffle

sys/kern/src/arch/arm_m.rs

@@ -1185,10 +1185,14 @@ unsafe extern "C" fn pendsv_entry() {
     crate::profiling::event_secondary_syscall_enter();
 
     let current = CURRENT_TASK_PTR.load(Ordering::Relaxed);
+    // We're being slightly pedantic about this, since it's straightforward (if
+    // weird) for pre-kernel main to trigger a PendSV, and it's not possible to
+    // disable this interrupt source.
     uassert!(!current.is_null()); // irq before kernel started?
 
     // Safety: we're dereferencing the current task pointer, which we're
-    // trusting the rest of this module to maintain correctly.
+    // trusting the rest of this module to maintain correctly (and we've just
+    // checked that the kernel has started).
     let current = usize::from(unsafe { (*current).descriptor().index });
 
     with_task_table(|tasks| {
@@ -1222,10 +1226,19 @@ pub unsafe extern "C" fn DefaultHandler() {
     };
 
     let current = CURRENT_TASK_PTR.load(Ordering::Relaxed);
-    uassert!(!current.is_null()); // irq before kernel started?
 
-    {
-        let t = unsafe { &mut *current };
+    // Because this handler is reachable in response to an NMI, it's possible to
+    // get here before the kernel has started. So before dereferencing the task
+    // pointer, check for NULL (the state at reset). If we manage to get here
+    // before the kernel has started, we'll skip the stack watermark update and
+    // hit a panic below.
+
+    // Safety: this dereferences the pointer only if it isn't NULL. If it
+    // isn't NULL, that means we've initialized it elsewhere in this module
+    // and it's a valid (but aliased) pointer to a task. We can safely
+    // dereference it as long as we throw it away before hitting
+    // `with_task_table` below.
+    if let Some(t) = unsafe { current.as_mut() } {
         t.update_stack_watermark();
     }
 
@@ -1251,6 +1264,8 @@ pub unsafe extern "C" fn DefaultHandler() {
                 .get(abi::InterruptNum(irq_num))
                 .unwrap_or_else(|| panic!("unhandled IRQ {irq_num}"));
 
+            // with_task_table will panic if the kernel has not yet been
+            // started. This is good.
             let switch = with_task_table(|tasks| {
                 disable_irq(irq_num, false);
 

sys/kern/src/task.rs

@@ -50,19 +50,12 @@ pub struct Task {
     /// restarted.
     descriptor: &'static TaskDesc,
 
-    /// Tracks the lowest stack pointer value (e.g. fullest stack) observed on
-    /// any kernel entry for this instance of this task.
+    /// Stack watermark tracking support.
     ///
-    /// Initialized to `u32::MAX` if the task has not yet run.
+    /// This field is completely missing if the feature is disabled to make that
+    /// clear to debug tools.
     #[cfg(feature = "stack-watermark")]
-    stack_pointer_low: u32,
-
-    /// Tracks the lowest stack pointer value (e.g. fullest stack) observed on
-    /// any kernel entry across *any* instance of this task.
-    ///
-    /// Initialized to `u32::MAX` if the task has not yet run.
-    #[cfg(feature = "stack-watermark")]
-    past_stack_pointer_low: u32,
+    stack_watermark: StackWatermark,
 }
 
 impl Task {
@@ -84,9 +77,7 @@ impl Task {
             save: crate::arch::SavedState::default(),
             timer: crate::task::TimerState::default(),
             #[cfg(feature = "stack-watermark")]
-            stack_pointer_low: u32::MAX,
-            #[cfg(feature = "stack-watermark")]
-            past_stack_pointer_low: u32::MAX,
+            stack_watermark: StackWatermark::default(),
         }
     }
 
@@ -341,9 +332,11 @@ impl Task {
 
         #[cfg(feature = "stack-watermark")]
         {
-            self.past_stack_pointer_low =
-                u32::min(self.past_stack_pointer_low, self.stack_pointer_low);
-            self.stack_pointer_low = u32::MAX;
+            self.stack_watermark.past_low = u32::min(
+                self.stack_watermark.past_low,
+                self.stack_watermark.current_low,
+            );
+            self.stack_watermark.current_low = u32::MAX;
         }
 
         crate::arch::reinitialize(self);
@@ -356,8 +349,10 @@ impl Task {
     pub fn update_stack_watermark(&mut self) {
         #[cfg(feature = "stack-watermark")]
         {
-            self.stack_pointer_low =
-                u32::min(self.stack_pointer_low, self.save().stack_pointer());
+            self.stack_watermark.current_low = u32::min(
+                self.stack_watermark.current_low,
+                self.save().stack_pointer(),
+            );
         }
     }
 
@@ -418,6 +413,32 @@ impl Task {
     }
 }
 
+#[cfg(feature = "stack-watermark")]
+#[derive(Copy, Clone, Debug)]
+struct StackWatermark {
+    /// Tracks the lowest stack pointer value (e.g. fullest stack) observed on
+    /// any kernel entry for this instance of this task.
+    ///
+    /// Initialized to `u32::MAX` if the task has not yet run.
+    current_low: u32,
+
+    /// Tracks the lowest stack pointer value (e.g. fullest stack) observed on
+    /// any kernel entry across *any* instance of this task.
+    ///
+    /// Initialized to `u32::MAX` if the task has not yet run.
+    past_low: u32,
+}
+
+#[cfg(feature = "stack-watermark")]
+impl Default for StackWatermark {
+    fn default() -> Self {
+        Self {
+            current_low: u32::MAX,
+            past_low: u32::MAX,
+        }
+    }
+}
+
 /// Interface that must be implemented by the `arch::SavedState` type. This
 /// gives architecture-independent access to task state for the rest of the
 /// kernel.