Rework max-stack analysis crate, address some limitations#2589
Open
jamesmunns wants to merge 32 commits into
Open
Rework max-stack analysis crate, address some limitations#2589jamesmunns wants to merge 32 commits into
jamesmunns wants to merge 32 commits into
Conversation
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.
Contributor
Author
|
Issues I've found so far: At least some code does meaningfully have recursion cyclesAt least some code is missing proper symbol table metadatathe function gets filtered out because of this // The original |
hawkw
reviewed
Jul 6, 2026
hawkw
left a comment
Member
There was a problem hiding this comment.
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.
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.
42089bb to
50a9c90
Compare
labbott
reviewed
Jul 7, 2026
labbott
left a comment
Collaborator
There was a problem hiding this comment.
I think this is a good approach, I'll take another pass later
Contributor
Author
|
@hawkw I will likely punt on switching over to |
This was referenced Jul 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note: this builds on top of #2587, which broke out the relevant functions into a standalone crate.
This PR consists of four main parts:
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).