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

NEW
Unassigned

Status

()

Core
JavaScript Engine: JIT
P3
normal
2 years ago
6 months ago

People

(Reporter: lth, Unassigned)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Reporter)

Description

2 years ago
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.

Updated

2 years ago
Priority: -- → P2
(Reporter)

Comment 1

2 years ago
See js/src/wasm/WasmBaselineCompiler.cpp, in the function endFunction().
(Reporter)

Comment 2

2 years ago
Created attachment 8808592 [details] [diff] [review]
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.
Assignee: nobody → lhansen
Status: NEW → ASSIGNED
Attachment #8808592 - Flags: review?(bbouvier)
(Reporter)

Updated

2 years ago
Blocks: 1316801
No longer blocks: 1188259
(Reporter)

Updated

2 years ago
Priority: P2 → P1
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?
Attachment #8808592 - Flags: review?(bbouvier)
(Reporter)

Comment 4

2 years ago
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.
(Reporter)

Updated

2 years ago
Assignee: lhansen → nobody
Status: ASSIGNED → NEW
(Reporter)

Updated

6 months ago
Summary: Wasm baseline: signal too-large-stack-frame in a less mysterious way → Wasm baseline: signal stack frame overflow in a less mysterious way
(Reporter)

Comment 6

6 months ago
Created attachment 8934902 [details]
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.
(Reporter)

Comment 7

6 months ago
Created attachment 8935273 [details]
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.
(Reporter)

Updated

6 months ago
See Also: → bug 1423831
You need to log in before you can comment on or make changes to this bug.