ISLE: Make block codegen non-recursive #8935
Conversation
In bytecodealliance#8919 we learned that `Codegen::emit_block` can overrun the stack when generating Rust source for deeply-nested blocks. Stack usage can be worse than one might expect because ISLE is called from a build script, and those are always built in debug mode. This commit avoids the problem by using an explicit heap-allocated stack instead of recursion. I recommend turning on the "ignore whitespace" option in your diff tool of choice when reviewing this commit. The amount of stack space that this recursive function uses is affected by the version of rustc used to compile it, and the amount of stack space available depends on the host platform. As a result, this was only observed on Windows with a recent nightly version of rustc, but it could eventually affect other platforms or compiler versions, especially as ISLE rules grow in complexity. Note that there are several other places in ISLE which recurse over the user-provided rules. This commit does not change those, but they could also become a problem in the future.
cranelift/isle/isle/src/codegen.rs
Outdated
| } | ||
|
|
||
| fn end_nested(&mut self) -> std::fmt::Result { | ||
| let (_, end, scope) = self.stack.pop().unwrap(); |
There was a problem hiding this comment.
Can this method take these things as arguments, instead of popping them? IIUC, that would allow the while let loop to do ctx.stack.pop() instead of ctx.stack.last_mut() which would make reasoning about control flow and termination much more straightforward, IMO, since it would all be localized within the same function.
There was a problem hiding this comment.
Similarly, soft preference for having begin_nested (or begin_block, and removing begin_nested) return the stack entry and having callers do something like
let entry = ctx.begin_nested()?;
ctx.stack.push(entry);For the same reasons, I think this would make it a little easier to understand what's happening here.
| } | ||
| } | ||
| } | ||
|
|
||
| Nested::Arms(source, arms) => { |
There was a problem hiding this comment.
I can't seem to find anything that pops the Nested::Arms entries from the stack -- does that ever happen? If not, is that a bug? I would assume it is?
cranelift/isle/isle/src/codegen.rs
Outdated
| let Some(arm) = arms.next() else { | ||
| ctx.end_nested()?; | ||
| continue; | ||
| }; |
There was a problem hiding this comment.
D'oh it is the else { ctx.end_nested() } that does the pop.
This kinda highlights what I was talking about w.r.t. following control and reasoning about termination, though.
I see that it is a little tricky though because of how we are pumping this iterator and only actually popping this stack entry once the iterator is exhausted. It is, of course, possible to pop the iterator and then push it back on if it is not empty (would require a peekable iterator) and this would be fine. But it might be more natural or "simpler" (depending on your taste) to instead push a separate stack entry for each item in the iterator. This latter approach doesn't require any extra is-it-done-or-not wrangling, which is kind of nice. It does use probably mean more entries pushed onto the stack and therefore potentially a larger heap allocation, but I don't think that matters at all here.
github-actions
bot
added
cranelift
Subscribe to Label Action
This issue or pull request has been labeled: "cranelift", "isle"
Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
|
I've now made the stack local to It's actually important to have some special treatment of the end of a list of cases or match-arms: we need to print some stuff and reset some state about what's still in scope. So if I take your suggestion of pushing each case/arm individually onto the stack, I also would need to push something like |
* ISLE: Make block codegen non-recursive In bytecodealliance#8919 we learned that `Codegen::emit_block` can overrun the stack when generating Rust source for deeply-nested blocks. Stack usage can be worse than one might expect because ISLE is called from a build script, and those are always built in debug mode. This commit avoids the problem by using an explicit heap-allocated stack instead of recursion. I recommend turning on the "ignore whitespace" option in your diff tool of choice when reviewing this commit. The amount of stack space that this recursive function uses is affected by the version of rustc used to compile it, and the amount of stack space available depends on the host platform. As a result, this was only observed on Windows with a recent nightly version of rustc, but it could eventually affect other platforms or compiler versions, especially as ISLE rules grow in complexity. Note that there are several other places in ISLE which recurse over the user-provided rules. This commit does not change those, but they could also become a problem in the future. * Review comments: make stack manipulation local to emit_block
* ISLE: Make block codegen non-recursive In bytecodealliance#8919 we learned that `Codegen::emit_block` can overrun the stack when generating Rust source for deeply-nested blocks. Stack usage can be worse than one might expect because ISLE is called from a build script, and those are always built in debug mode. This commit avoids the problem by using an explicit heap-allocated stack instead of recursion. I recommend turning on the "ignore whitespace" option in your diff tool of choice when reviewing this commit. The amount of stack space that this recursive function uses is affected by the version of rustc used to compile it, and the amount of stack space available depends on the host platform. As a result, this was only observed on Windows with a recent nightly version of rustc, but it could eventually affect other platforms or compiler versions, especially as ISLE rules grow in complexity. Note that there are several other places in ISLE which recurse over the user-provided rules. This commit does not change those, but they could also become a problem in the future. * Review comments: make stack manipulation local to emit_block
* ISLE: Make block codegen non-recursive In #8919 we learned that `Codegen::emit_block` can overrun the stack when generating Rust source for deeply-nested blocks. Stack usage can be worse than one might expect because ISLE is called from a build script, and those are always built in debug mode. This commit avoids the problem by using an explicit heap-allocated stack instead of recursion. I recommend turning on the "ignore whitespace" option in your diff tool of choice when reviewing this commit. The amount of stack space that this recursive function uses is affected by the version of rustc used to compile it, and the amount of stack space available depends on the host platform. As a result, this was only observed on Windows with a recent nightly version of rustc, but it could eventually affect other platforms or compiler versions, especially as ISLE rules grow in complexity. Note that there are several other places in ISLE which recurse over the user-provided rules. This commit does not change those, but they could also become a problem in the future. * Review comments: make stack manipulation local to emit_block Co-authored-by: Jamey Sharp <jsharp@fastly.com>
In #8919 we learned that
Codegen::emit_blockcan overrun the stack when generating Rust source for deeply-nested blocks. Stack usage can be worse than one might expect because ISLE is called from a build script, and those are always built in debug mode.This commit avoids the problem by using an explicit heap-allocated stack instead of recursion.
I recommend turning on the "ignore whitespace" option in your diff tool of choice when reviewing this commit.
The amount of stack space that this recursive function uses is affected by the version of rustc used to compile it, and the amount of stack space available depends on the host platform. As a result, this was only observed on Windows with a recent nightly version of rustc, but it could eventually affect other platforms or compiler versions, especially as ISLE rules grow in complexity.
Note that there are several other places in ISLE which recurse over the user-provided rules. This commit does not change those, but they could also become a problem in the future.