1282063 - Wasm baseline: signal stack frame overflow in a less mysterious way

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, enhancement, P5)

Description

[Lars T Hansen [:lth]]

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.

Comment 1

[Lars T Hansen [:lth]]

See js/src/wasm/WasmBaselineCompiler.cpp, in the function endFunction().

Comment 2

[Lars T Hansen [:lth]]

Attachment: bug1282063-signal-frame-overflow.patch

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.

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

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?

Comment 4

[Lars T Hansen [:lth]]

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.

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

OK, I think signaling the error at least would be a first step into the right direction.

Comment 6

[Lars T Hansen [:lth]]

Attachment: oflo.js

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.

Comment 7

[Lars T Hansen [:lth]]

Attachment: oflo2.js

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.

Comment 8

[Lars T Hansen [:lth]]

We'll get to this if it becomes necessary.

Comment 9

[Ryan Hunt [:rhunt]]

Attachment: Bug 1282063 - wasm: Unify max frame size and report overflow as an error. r?lth

Baseline has a limit to the size of the stack frame allowed for an
individual function. I saw comments that wasm-Ion had a similar
restriction but I couldn't find any such restriction.

This commit takes the hardcoded stack frame limit and makes it
an implementation limit in WasmConstants, and uses it explicitly
in wasm-Ion. (Open to discussion if there's an argument for different
limits or no limit in wasm-Ion)

For both stack frame checks, surpassing the limit is reported as
a hard error with a message instead of an OOM. This should provide
better debuggability to users who stumble into this. I think
re-using errors for this makes sense, but if not we could try to
re-use the warning machinery, although it would be a larger change.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c5d0338411ed
wasm: Unify max frame size and report overflow as an error. r=lth

Comment 11

[Cristina Cozmuta (:CrissCozmuta)]

Backed out for causing SM bustages. CLOSED TREE
Backout link : https://hg.mozilla.org/integration/autoland/rev/462b8ab0d119d85c044a1332446b609786fcabc7
Push with failures : https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Crunnable&revision=c5d0338411ed996139d799136bad285b534b3ad4&selectedTaskRun=cxs52NUfQkeYorNzvjkXyA.0
Link to failure log : https://treeherder.mozilla.org/logviewer?job_id=351803324&repo=autoland&lineNumber=6580

Comment 12

[Ryan Hunt [:rhunt]]

BadIncludesOrder error missed by testing these patches as part of a larger stack.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0cf96895f20c
wasm: Unify max frame size and report overflow as an error. r=lth

Comment 14

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/0cf96895f20c