Open Bug 1282063 Opened 3 years ago Updated 6 months ago
Wasm baseline: signal stack frame overflow in a less mysterious way
The Wasm baseline compiler has a hard cutoff on the stack frame size at 256KB, for simplicity. If the frame is larger than that then an OOM is signaled. This is probably OK but it would be better to signal an error that carries more information. Bug 1280934 comment 7 has a longer discussion and some concrete suggestions.
See js/src/wasm/WasmBaselineCompiler.cpp, in the function endFunction().
This is half the solution, I think: Introduce a way for the function compiler to set the failure reason on the FuncCompileResults. (Benjamin, see your comment on the bug linked to from the earlier comment here for your suggestion among these lines.) We still need the other half, where this result is picked up and signaled as an error message.
Assignee: nobody → lhansen
Status: NEW → ASSIGNED
Comment on attachment 8808592 [details] [diff] [review] bug1282063-signal-frame-overflow.patch Review of attachment 8808592 [details] [diff] [review]: ----------------------------------------------------------------- The interface looks good, but I'd rather have the full solution reviewed and checked in at once. Thinking a bit about it: do you have a test case that triggers this issue? If so, does the same issue happen under Ion? I can imagine that in some cases, this issue gets triggered under one compiler but not the other; should we try to recompile the function with the other compiler, in this case, instead?
Yes, there is a test case that triggers the issue, though you could argue that it is partial: jit-test/tests/wasm/regress/too-large-frame.js. I have no idea if Ion has the same problem. It probably does, though it might manifest in some other way. I vaguely remember asking somebody about this and getting a vague answer that it might not be signaled properly (ie we'd probably crash with a stack overflow). But this is all from memory. I don't want this bug to be about what Ion does. Falling back from baseline to Ion might seem to be sensible but my hunch is that it doesn't really pay off to do that work.
Priority: P1 → P3
OK, I think signaling the error at least would be a first step into the right direction.
Assignee: lhansen → nobody
Status: ASSIGNED → NEW
Summary: Wasm baseline: signal too-large-stack-frame in a less mysterious way → Wasm baseline: signal stack frame overflow in a less mysterious way
Cause overflow by stacking too many intermediate results. Also see https://github.com/WebAssembly/design/issues/1163 for a discussion about frame limits, not sure where that will go.
A test case that also causes the stack frame to be too large in Baldr (linux x64 release). Baldr reacts the same way Rabaldr does, it signals OOM. But Baldr's stack frame limit is much larger, and it will be able to tolerate many locals in addition since in typical cases they will be optimized out.
You need to log in before you can comment on or make changes to this bug.