transpile: rewrite relooper to be iterative to avoid stack overflow on large CFGs - #1995
transpile: rewrite relooper to be iterative to avoid stack overflow on large CFGs#1995Hellboy28D wants to merge 2 commits into
Conversation
|
Please fix formatting issues. The PR body uses codex/gpt default style which explains how your agent tested the PR. It is better that you only present the information necessary for review; this is also more consistent with other PRs. Other than that, this looks good to me (thanks!) but @randomPoison must decide whether this PR can land. |
There was a problem hiding this comment.
Well, this seems better than your other PR; Tests pass, and there are few extraneous changes that break things. You need to fix formatting to satisfy CI, though.
The main thing I'd like to see changed is that we're still recursing at the end of make_loop to process the things that follow the loop. That means that a sufficiently long sequence of loops will still cause us to stack overflow. Change make_loop to not call relooper at the end, and in the cases where we're returning after calling make_loop that should change to continue.
You also need to update the issue_1821_transpiles_quickly test case to remove the extended stack limit so that we can verify that we no longer overflow the stack for that test case. That test case is setup to run in another thread as a hack, so that extra thread setup should also be able to be removed as part of this PR.
| entries: entries.clone(), | ||
| body, | ||
| span, | ||
| terminator, | ||
| }); | ||
|
|
||
| // Continue processing the successors of the block we just | ||
| // emitted instead of recursing. This keeps sequential runs | ||
| // of `Simple` blocks from growing the stack (c2rust#1908). | ||
| entries = new_entries; |
There was a problem hiding this comment.
You're cloning entries only to immediately throw away the old value and replace it with new_entries. You can avoid this by mem::takeing the old value, which will replace it with an empty IndexSet.
| let inlined: Vec<Label> = follow_entries | ||
| .iter() | ||
| .filter(|&e| self.global_predecessors[e].len() == 1); | ||
| .filter(|&e| self.global_predecessors[e].len() == 1) | ||
| .cloned() | ||
| .collect(); | ||
|
|
||
| // Move all nodes dominated by an inlined node into the loop. This will include | ||
| // the inlined node since all nodes dominate themself. | ||
| for inlined in inlined { | ||
| for dominated in &self.domination_sets[inlined] { | ||
| for entry in &inlined { | ||
| for dominated in &self.domination_sets[entry] { |
There was a problem hiding this comment.
What are these changed bits doing? I don't see a reason why we need to collect this iterator, and this change doesn't seem like it affects behavior at all.
|
|
||
| impl RelooperState { | ||
| /// Recursive helper for `reloop`. | ||
| /// Helper for `reloop`. |
There was a problem hiding this comment.
This function is still recursive.
| // can be reached by other entries. This means irreducible control flow, which | ||
| // we have to process by making a loop. | ||
| self.make_loop(&strict_reachable_from, blocks, entries, result); | ||
| return; |
There was a problem hiding this comment.
We don't want to return after make_loop, we want to continue iterating to process the blocks that follow the loop.
|
|
||
| result.push(Structure::Multiple { entries, branches }); | ||
|
|
||
| // Continue processing the follow blocks instead of recursing. |
There was a problem hiding this comment.
We don't need this "instead of recursing" comment.
| // `cfg.entries` is the CFG's single entry `Label`; `relooper` wants a set | ||
| // of entries so it can be reused uniformly for `Multiple` branches too. |
There was a problem hiding this comment.
This comment is unnecessary.
| /// | ||
| /// TODO: perhaps manually perform TCO? | ||
| /// Sequential chains — runs of `Simple` blocks, and the follow-blocks | ||
| /// after a `Multiple` — are processed with an explicit loop rather than | ||
| /// tail recursion, so large functions with long straight-line CFGs | ||
| /// cannot exhaust the stack (c2rust#1908). Recursion is still used for | ||
| /// the individual branches of a `Multiple` and inside `make_loop`, since | ||
| /// that recursion depth is bounded by nesting depth rather than block | ||
| /// count. |
There was a problem hiding this comment.
This comment is unnecessary.
3 participants
Summary
The relooper in
c2rust-transpileused a recursive algorithm for control-flowreconstruction. On functions with very large/deep CFGs (e.g. thousands of
sequential basic blocks, as in the BLAKE-256 example in #1821), this could
exhaust the stack and crash the transpiler.
This PR rewrites the relooper to work iteratively, using an explicit
dominator computation instead of recursion, so structured control-flow
reconstruction no longer depends on stack depth proportional to CFG size.
Changes
c2rust-transpile/src/cfg/relooper.rswith an iterative oneTesting
cargo test -p c2rust-transpile relooper— all 7 new dominator tests passbyte-identical to the output produced by upstream
master, confirming therewrite preserves translation semantics
increased
ulimit -sNotes
Happy to adjust the approach or add more tests based on maintainer feedback.