ringbuf: Handle toolchains >= 1.96

cmd/ringbuf/src/lib.rs

@@ -62,14 +62,16 @@
 //! See the [`ringbuf`
 //! documentation](https://github.com/oxidecomputer/hubris/blob/master/lib/ringbuf/src/lib.rs) for more details.
 
-use anyhow::{Result, bail};
+use anyhow::{Context, Result, anyhow, bail};
 use clap::Parser;
 use humility::core::Core;
 use humility::hubris::*;
 use humility::log::info;
 use humility::reflect::{self, Format, Load, Value};
 use humility_cli::{ExecutionContext, humility_cmd};
-use humility_doppel::{CountedRingbuf, CounterVariant, Ringbuf, StaticCell};
+use humility_doppel::{
+    CountedRingbuf, CounterVariant, Counters, Ringbuf, StaticCell,
+};
 
 #[derive(Parser, Debug)]
 #[clap(name = "ringbuf", about = env!("CARGO_PKG_DESCRIPTION"))]
@@ -112,6 +114,55 @@ struct TotalsOptions {
     no_totals: bool,
 }
 
+type LoadedRingbuf = (Option<Ringbuf>, Option<Counters>);
+type RingbufLoader = fn(&Value) -> Result<LoadedRingbuf>;
+
+// There are three possible shapes of ringbufs, depending on the age of the
+// firmware.
+// - Raw Ringbuf that is not wrapped by anything.
+// - Safe Ringbuf that is inside a StaticCell.
+// - CountedRingbuf, which consists of a `StaticCell<Ringbuf>` and a set of
+//   entry counters.
+//
+// Here we will attempt to handle all three, keeping all the error context of
+// all of them if none of them succeed.
+fn load_ringbuf(ringbuf_val: &Value) -> Result<LoadedRingbuf> {
+    // Pre-hubris#219
+    fn raw(v: &Value) -> Result<LoadedRingbuf> {
+        Ok((Some(Ringbuf::from_value(v)?), None))
+    }
+    // Post-hubris#219
+    fn static_cell(v: &Value) -> Result<LoadedRingbuf> {
+        let cell: StaticCell<Ringbuf> = StaticCell::from_value(v)?;
+        Ok((Some(cell.cell.value), None))
+    }
+    // Post-hubris#1621
+    fn counted(v: &Value) -> Result<LoadedRingbuf> {
+        let CountedRingbuf { ringbuf, counters } =
+            CountedRingbuf::from_value(v)?;
+        Ok((ringbuf, Some(counters)))
+    }
+
+    let methods: [(&str, RingbufLoader); 3] = [
+        ("CountedRingbuf", counted),
+        ("Ringbuf", raw),
+        ("StaticCell<Ringbuf>", static_cell),
+    ];
+    let mut errs = vec![];
+    for (name, method) in methods {
+        match method(ringbuf_val) {
+            Ok(r) => return Ok(r),
+            Err(e) => errs.push((name, e)),
+        }
+    }
+
+    let mut err = Err(anyhow!("Failed to load ringbuf"));
+    for (name, e) in errs {
+        err = err.with_context(|| format!("Tried '{name}', got: {e:#}"));
+    }
+    err.context("Failed to load ringbuf using any known layout")
+}
+
 fn ringbuf_dump(
     subargs: &RingbufArgs,
     hubris: &HubrisArchive,
@@ -127,28 +178,10 @@ fn ringbuf_dump(
     core.read_bulk(ringbuf_var.addr, buf.as_mut_slice())?;
     core.run()?;
 
-    // There are three possible shapes of ringbufs, depending on the age of the
-    // firmware.
-    // - Raw Ringbuf that is not wrapped by anything.
-    // - Safe Ringbuf that is inside a StaticCell.
-    // - CountedRingbuf, which consists of a `StaticCell<Ringbuf>` and a set of
-    //   entry counters.
-    //
-    // Here we will attempt to handle all three -- first the counted ringbuf,
-    // then raw, then fallback.
     let ringbuf_val: Value =
         Value::Struct(reflect::load_struct(hubris, &buf, definition, 0)?);
 
-    let (ringbuf, counters) = CountedRingbuf::from_value(&ringbuf_val)
-        .map(|CountedRingbuf { ringbuf, counters }| (ringbuf, Some(counters)))
-        .or_else(|_e| {
-            Ringbuf::from_value(&ringbuf_val).map(|r| (Some(r), None))
-        })
-        .or_else(|_e| {
-            let cell: StaticCell<Ringbuf> =
-                StaticCell::from_value(&ringbuf_val)?;
-            Ok::<_, anyhow::Error>((Some(cell.cell.value), None))
-        })?;
+    let (ringbuf, counters) = load_ringbuf(&ringbuf_val)?;
 
     if let (Some(mut counters), false) = (counters, no_totals) {
         const TOTAL_WIDTH: usize = 8;
@@ -291,6 +324,7 @@ fn ringbuf(subargs: RingbufArgs, context: &mut ExecutionContext) -> Result<()> {
     }
 
     let core = &mut *context.cli.attach_live_or_dump_match(hubris)?;
+    let mut show_hint = false;
     for (v, def) in ringbufs {
         // Try not to use `?` here, because it causes one bad ringbuf to make
         // them all unavailable.
@@ -304,6 +338,7 @@ fn ringbuf(subargs: RingbufArgs, context: &mut ExecutionContext) -> Result<()> {
                 if subargs.verbose {
                     info!(log, "ringbuf dump failed: {e:?}");
                 } else {
+                    show_hint = true;
                     info!(log, "ringbuf dump failed: {e}");
                 }
             }
@@ -312,6 +347,16 @@ fn ringbuf(subargs: RingbufArgs, context: &mut ExecutionContext) -> Result<()> {
         }
     }
 
+    // If any decoding failed, show a hint that rerunning with -v will show the
+    // whole error chain.
+    if show_hint {
+        info!(
+            log,
+            "one or more ringbufs failed to load. Rerun with `-v` to show more \
+            details.",
+        );
+    }
+
     Ok(())
 }
 

humility-bin/tests/cmd/ringbuf-arg/ringbuf-arg.panic-on-boot.stderr

@@ -3,3 +3,4 @@ humility: ringbuf dump failed: loading value of type struct UnsafeCell<ringbuf::
 humility: ringbuf dump failed: loading value of type struct UnsafeCell<ringbuf::Ringbuf<drv_stm32xx_i2c::Trace, 48>> at address 0x0
 humility: ringbuf dump failed: loading value of type struct UnsafeCell<ringbuf::Ringbuf<drv_stm32xx_i2c::Trace, 48>> at address 0x0
 humility: ringbuf dump failed: loading value of type struct UnsafeCell<ringbuf::Ringbuf<drv_stm32xx_i2c_server::Trace, 8>> at address 0x0
+humility: one or more ringbufs failed to load. Rerun with `-v` to show more details.

humility-bin/tests/cmd/ringbuf-full-totals/ringbuf-full-totals.panic-on-boot.stderr

@@ -18,3 +18,4 @@ humility: ringbuf dump failed: loading value of type struct UnsafeCell<ringbuf::
 humility: ringbuf dump failed: loading value of type struct UnsafeCell<ringbuf::Ringbuf<task_thermal::Trace, 32>> at address 0x0
 humility: ringbuf dump failed: loading value of type struct UnsafeCell<ringbuf::Ringbuf<task_validate::Trace, 64>> at address 0x0
 humility: ringbuf dump failed: loading value of type struct UnsafeCell<ringbuf::Ringbuf<vsc85xx::Trace, 16>> at address 0x0
+humility: one or more ringbufs failed to load. Rerun with `-v` to show more details.

humility-bin/tests/cmd/ringbuf/ringbuf.panic-on-boot.stderr

@@ -18,3 +18,4 @@ humility: ringbuf dump failed: loading value of type struct UnsafeCell<ringbuf::
 humility: ringbuf dump failed: loading value of type struct UnsafeCell<ringbuf::Ringbuf<task_thermal::Trace, 32>> at address 0x0
 humility: ringbuf dump failed: loading value of type struct UnsafeCell<ringbuf::Ringbuf<task_validate::Trace, 64>> at address 0x0
 humility: ringbuf dump failed: loading value of type struct UnsafeCell<ringbuf::Ringbuf<vsc85xx::Trace, 16>> at address 0x0
+humility: one or more ringbufs failed to load. Rerun with `-v` to show more details.

humility-core/src/reflect.rs

@@ -962,7 +962,19 @@ pub fn load_enum(
 /// That extra level is why images built by the newer toolchains stopped
 /// decoding. We peel all of these, so the payload can be presented the same way
 /// whichever toolchain built the image.
-const TRANSPARENT_WRAPPERS: &[&str] = &["ManuallyDrop", "MaybeDangling"];
+const TRANSPARENT_WRAPPERS: &[&str] = &[
+    "ManuallyDrop",
+    "MaybeDangling",
+    // Starting in Rust 1.96, `Atomic` types now are aliases and have an
+    // inner private `Align*` type.
+    //
+    // See https://github.com/rust-lang/rust/pull/153015 for details.
+    "Align1",
+    "Align2",
+    "Align4",
+    "Align8",
+    "Align16",
+];
 
 /// Returns true if `name` names one of the [`TRANSPARENT_WRAPPERS`].
 ///

humility-doppel/src/lib.rs

@@ -678,10 +678,22 @@ impl humility::reflect::Load for CounterVariant {
             return Ok(Self::Single(0));
         }
 
+        // Starting in Rust 1.96, `Atomic` types now are aliases of
+        // `Atomic<N>`.
+        //
+        // See https://github.com/rust-lang/rust/pull/153015 for details.
+        let is_atomic_u32 = |name: &str| {
+            // Pre-hubris#2775
+            let legacy = name.starts_with("AtomicU32");
+            // Post-hubris#2775
+            let generic = name == "Atomic<u32>";
+            legacy || generic
+        };
+
         // If the value is directly an AtomicU32, we treat it as a single
         // counter value.
         if let Ok(counter) = value.as_struct()
-            && counter.name().starts_with("AtomicU32")
+            && is_atomic_u32(counter.name())
         {
             let cell = UnsafeCell::<u32>::from_value(&counter["v"])
                 .context("ringbuf count must be a u32")?;