Rework max-stack analysis crate, address some limitations by jamesmunns · Pull Request #2589 · oxidecomputer/hubris

jamesmunns · 2026-07-06T16:03:20Z

Note: this builds on top of #2587, which broke out the relevant functions into a standalone crate.

This PR consists of four main parts:

A direct fix for Current max stack analysis code is (significantly) underestimating necessary stack space for tasks #2588 "Program Counter Relative Branching" defect. This is a ~1 line change when filtering the potential instructions for branching
An attempt to address Current max stack analysis code is (significantly) underestimating necessary stack space for tasks #2588 "Missing sanity cross-checks", which looks for any functions not present in the call graph, and adds the largest call stack of those functions to the evaluated call graph, as a potential "fudge factor" for indirect branching/dyn dispatch we were unable to follow
Updating of manifests to increase stack sizes based on 1 + 2
A major refactor of the current max stack analysis code

The 4th part isn't strictly necessary, however while investigating and making myself familiar with how our current code works, it was useful to me to split things out so I could follow what was going on, and restructure the analysis to allow for some more interactive debugging/diffing. I personally think the code is a bit more accessible now, but that could be more personal preference than anything else.

The 1st and 3rd parts are achievable with a much smaller (a few lines) diff. If this PR is too difficult to review, happy to bail on this or at least defer it.

The 2nd part would be more difficult (but still possible) to achieve without the refactorings made in the 4th part, as the new call graph analysis lets us more easily keep track of which functions have been visiting when traversing the graph.

If we merge this PR, I'd consider the "urgent" part of #2588 resolved, though there are still some "known defects", and we might want to increase the baseline stack requirements to handle register stacking (this would bump most task stack sizes in most manifests by 104 bytes, and +36 for gimletlet).

I intend to do some rework of the max-stack analysis code, and would like to break out the max-stack analysis into its own crate to make testing this in isolation easier. As this code also depends on the `xtask::elf` module, I broke that out to avoid circular deps. This is the minimum change necessary to split things out before any refactoring.

No functional changes expected.

jamesmunns · 2026-07-06T17:28:30Z

Issues I've found so far:

At least some code does meaningfully have recursion cycles

$ cargo xtask dist test/tests-stm32g0/app-g070.toml
thread 'main' (17436042) panicked at build/stack/src/lib.rs:285:27:
unable to resove 0x08004BD8: Refusing to handle recursion: [0x08004BD8, 0x08004BE8] -> 0x08004BD8
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

 08004bd8 <core::str::slice_error_fail>:
 8004bd8: b580          push    {r7, lr}
 8004bda: af00          add     r7, sp, #0x0
 8004bdc: b082          sub     sp, #0x8
 8004bde: 68bc          ldr     r4, [r7, #0x8]
 8004be0: 9400          str     r4, [sp]
 8004be2: f000 f801     bl      0x8004be8 <core::str::slice_error_fail_rt> @ imm = #0x2
 8004be6: d4d4          bmi     0x8004b92 <<u32 as core::fmt::Display>::fmt+0xaa> @ imm = #-0x58

08004be8 <core::str::slice_error_fail_rt>:
 8004be8: b580          push    {r7, lr}
 8004bea: af00          add     r7, sp, #0x0
...
 8004c92: 9200          str     r2, [sp]
 8004c94: 4622          mov     r2, r4
 8004c96: f7ff ff9f     bl      0x8004bd8 <core::str::slice_error_fail> @ imm = #-0xc2

At least some code is missing proper symbol table metadata

objdump -xSC ../../target/oxide-rot-1-selfsigned/dist/attest.tmp | grep '161f4'
000161f4 l       .text	00000000 $t
000161f4 g     F .text	00000000 fe25519_add_asm
                        ^^^^^^^^ -> size = 0

the function gets filtered out because of this

thread 'main' (17388305) panicked at build/stack/src/main.rs:43:48:
called `Result::unwrap()` on an `Err` value: 000161f4: no function data
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

// The original .a doesn't have info for this object. Some functions do, this one doesn't.

$ objdump -xSC ~/Downloads/salty-asm.a
SYMBOL TABLE:
00000000 l    d  .text  00000000 .text
00000000 l    d  .data  00000000 .data
00000000 l    d  .bss   00000000 .bss
00000000 l       .text  00000000 $t
00000000 l    d  .ARM.attributes        00000000 .ARM.attributes
00000000 g     F .text  00000000 fe25519_add_asm

Disassembly of section .text:

00000000 <fe25519_add_asm>:
       0: b4f0          push    {r4, r5, r6, r7}
       2: f04f 0701     mov.w   r7, #0x1

hawkw

this is mostly style nitpicking because i didn't really understand the old code and i am not sure if i fully understand the new code either.

hawkw · 2026-07-06T20:29:10Z

+/// A function that has been visited and "filled in" by the [`Resolver`].
 ///
-/// This does not take dynamic function calls into account, which could cause
-/// underestimation.  Overestimation is less likely, but still may happen if
-/// there are logically impossible call trees (e.g. `A -> B` and `B -> C`, but
-/// `B` never calls `C` if called by `A`).
-pub fn get_max_stack(
-    elf: &Path,
-    task_name: &str,
-    verbose: bool,
-) -> Result<Vec<(u64, String)>> {
-    // Open the statically-linked ELF file
-    let data = std::fs::read(elf).context("could not open ELF file")?;
-    let elf = goblin::elf::Elf::parse(&data)?;
+/// Typically stored in an [`Rc`] as each node may appear multiple times across
+/// the call graph of a program.
+pub struct ResolvedNode {
+    pub addr: u32,


Take it or leave it: it seems like these are (almost?) always being stored in BTreeMaps with the addr as the key. This seems like it might be a perfect candidate for using iddqd::IdOrdMap and implementing IdOrdItem for Rc<ResolvedNode>, rather than std::collections::BTreeMap, so that we can ensure that the value of the addr field stored by the ResolvedNode is always what we use as the key into those maps.

I'll look into iddqd, or at least clarify why they are separate if I can't do that. The short answer is "these didn't used to be bundled up, it was more sequential, but now I put them all together so you could look at all the intermediary analysis stages", but that's more of an explanation rather than a defense of a good API.

hawkw · 2026-07-06T20:30:02Z

+    pub function_items: BTreeMap<u32, FunctionData>,
+    pub addr_to_frame_size: BTreeMap<u32, u64>,
+    pub names: BTreeMap<u32, String>,


do we expect that a given address will always have an entry in all these maps, or not? what is the difference between something having an entry in addr_to_frame_size or names from a FunctionData's name or frame_size values? It would be nice if there was a comment explaining this.

hawkw · 2026-07-06T20:31:21Z

+pub struct Resolver {
+    pub call_stack: Vec<u32>,
+    pub all_resolved: BTreeMap<u32, Rc<ResolvedNode>>,
+    pub fn_items: BTreeMap<u32, FunctionData>,


similarly, I wonder if we might want FunctionData to have an addr field, ane make this an IdOrdMap as well.

hawkw · 2026-07-06T20:32:12Z

+    pub fn resolve_by_name(&mut self, entry: &str) -> Result<Rc<ResolvedNode>> {
+        let Some(item) = self.fn_items.iter().find(|(_k, v)| &v.name == entry)
+        else {
+            bail!("Not found");


nit: maybe

Suggested change

bail!("Not found");

bail!("function `{entry}` not found");

hawkw · 2026-07-06T20:32:43Z

+
+        // no, we havent. Get the node info from the fn data
+        let Some(item) = self.fn_items.get(&addr) else {
+            bail!("no function data");


nit: maybe

Suggested change

bail!("no function data");

bail!("no function data for {addr:#x}");

hawkw · 2026-07-06T21:10:06Z

+                    // based jump here
                    let arm::ArmOperandType::Imm(target) = op.op_type else {
+                        if verbose {
+                            println!(


nit: should this perhaps be

Suggested change

println!(

eprintln!(

I think the old code was printing to stdout with --verbose, not stderr, but I will double check.

hawkw · 2026-07-06T21:10:23Z

+                    // Check if this is now not needed anymore
+                    if !is_grp_rel(g) && is_tail_call(g) {
+                        panic!("this check is NOT obsolete!");
+                    }


can you explain this?

will do, I think the new change superceded the old "tail call" check, but wasn't very confident about this. This was a tombstone to tell me if that was in fact not true. I will add a comment or remove this.

hawkw · 2026-07-06T21:11:15Z

+                    // On Arm/Thumb, a jump always has some kind of operand,
+                    // which is where we are jumping to. Get the last operand so
+                    // we can determine if we can follow this.
                    let arch = detail.arch_detail();
-                    let ops = arch.operands();
-                    let op = ops.last().unwrap_or_else(|| {
+                    let ArchDetail::ArmDetail(details) = arch else {
+                        panic!("Unsupported arch");
+                    };
+                    let op = details.operands().last().unwrap_or_else(|| {
                        panic!("missing operand!");


nit, take it or leave it: maybe the panic message could be where we say that ARM jumps always have an operand?

hawkw · 2026-07-06T21:15:00Z

+                    // Avoid recursive calls to the same function, and ignore
+                    // any control flow that directs back inside this function.


nit: maybe this comment ought to be reworded a bit --- "avoid recursive calls" felt confusing to me especially since we are now counting the recursive calls, which feels like a substantially different thing than "avoiding" them...

This is true, I used to bail! here, until I realized that we couldn't actually do this in practice. Will update the comment.

hawkw · 2026-07-06T21:16:54Z

-        let name = &f.short_name;
-        out.push((f.frame_size.unwrap_or(0), name.clone()));
+fn name_shortener(name: &str) -> String {
+    // Strip the trailing hash from the name for ease of printing


are there situations in which this might be collidey?

This is a more targeted, partial fix for #2588, and contains the most immediate relief from the larger refactoring in #2589. This catches the largest "novel" defect: ignoring of relative branches, and raises all the stack numbers to what the new analysis in #2589 shows. This is intended as a stop-gap to prevent seeing stack overflows in test benches. Split into two commits, one with manifest updates, and one with the stack analysis change.

labbott

I think this is a good approach, I'll take another pass later

labbott · 2026-07-07T13:34:37Z

+//! 1. This approach does not handle recursion, as we have no way to annotate a
+//!    potential upper bound of recursive iterations. Currently, the code
+//!    counts the number of direct recursion instances detected (e.g. self-calls
+//!    of a function), and refuses to resolve call stacks with cycles.


Would this also let us detect recursion? I almost want to say we should be disallowing recursion in hubris given the difficulties with stack sizing but I would settle for a warning.

Unfortunately, I tried to disallow this, but at least tests-stm32g0 does meaningfully have recursion: #2589 (comment)

labbott · 2026-07-07T13:36:41Z

+//! 2. This approach does not handle "indirect" branching, which looks something
+//!    like `blx r5` in assembly, and is often (but not exclusively) generated
+//!    when calling a function through a vtable method, like `dyn Format`.
+//!    Currently, the code silently skips considering instances of indirect
+//!    branching found.


Insert rant about panic and formatting (I agree we can't fix that here)

jamesmunns added 12 commits July 4, 2026 18:30

Split out some nested items from the function

156ba53

Start extracting portions of get_max_stack

e087d34

No functional changes expected.

Some more refactoring

d340c3e

Add a small harness for stack

5064164

More touch-ups

c7990a9

Add fix for missing stack, add some tooling for catching more

aa26151

Wire up more of the new resolver

184b17a

Final docs/refactoring/reorg pass

24c2dbe

Add docs from the issue writeup, and tweak recursion detection

a8bdeeb

Tweak fudge factor to "max" instead of "sum" of missing items

fcb73aa

Update stack limits

e7bf246

jamesmunns requested review from cbiffle, evan-oxide, iximeow and labbott July 6, 2026 16:03

jamesmunns changed the title ~~James/enhance stack~~ Rework max-stack analysis crate, address some limitations Jul 6, 2026

hawkw reviewed Jul 6, 2026

View reviewed changes

jamesmunns mentioned this pull request Jul 6, 2026

Increase stack limits, check relative branches #2590

Merged

jamesmunns force-pushed the james/extract-stack branch from 42089bb to 50a9c90 Compare July 7, 2026 13:24

labbott reviewed Jul 7, 2026

View reviewed changes

Base automatically changed from james/extract-stack to master July 7, 2026 13:41

jamesmunns added 2 commits July 7, 2026 15:53

Merge remote-tracking branch 'origin/master' into james/enhance-stack

8adc77d

Fix bad merge

3aa8112

	bail!("no function data");
	bail!("no function data for {addr:#x}");

		// Avoid recursive calls to the same function, and ignore
		// any control flow that directs back inside this function.

Uh oh!

Conversation

jamesmunns commented Jul 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jamesmunns commented Jul 6, 2026

At least some code does meaningfully have recursion cycles

At least some code is missing proper symbol table metadata

Uh oh!

hawkw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

labbott left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jamesmunns commented Jul 6, 2026 •

edited

Loading