Skip to content

Rework max-stack analysis crate, address some limitations#2589

Open
jamesmunns wants to merge 32 commits into
masterfrom
james/enhance-stack
Open

Rework max-stack analysis crate, address some limitations#2589
jamesmunns wants to merge 32 commits into
masterfrom
james/enhance-stack

Conversation

@jamesmunns

@jamesmunns jamesmunns commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

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

This PR consists of four main parts:

  1. 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
  2. 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
  3. Updating of manifests to increase stack sizes based on 1 + 2
  4. 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).

jamesmunns added 12 commits July 4, 2026 18:30
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

Copy link
Copy Markdown
Contributor Author

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

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

@hawkw hawkw left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread build/stack/src/lib.rs
Comment thread build/stack/src/lib.rs Outdated
Comment thread build/stack/src/lib.rs
Comment thread build/stack/src/lib.rs Outdated
Comment thread build/stack/src/lib.rs Outdated
Comment thread build/stack/src/lib.rs Outdated
Comment thread build/stack/src/lib.rs Outdated
Comment thread build/stack/src/lib.rs Outdated
Comment thread build/stack/src/lib.rs Outdated
Comment thread build/stack/src/lib.rs Outdated
jamesmunns added a commit that referenced this pull request Jul 6, 2026
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.
@jamesmunns jamesmunns force-pushed the james/extract-stack branch from 42089bb to 50a9c90 Compare July 7, 2026 13:24

@labbott labbott left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread build/stack/src/lib.rs
Comment thread build/stack/src/lib.rs
Base automatically changed from james/extract-stack to master July 7, 2026 13:41
@jamesmunns

Copy link
Copy Markdown
Contributor Author

@hawkw I will likely punt on switching over to iqqdq, I agree with you it's a good fit, but I probably need to stop working on this for a bit, and would like to get it landed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants