Implement --check-stack-overflow flag for wasm-emscripten-finalize#2278
Implement --check-stack-overflow flag for wasm-emscripten-finalize#2278quantum5 merged 9 commits intoWebAssembly:masterfrom
Conversation
kripken
left a comment
There was a problem hiding this comment.
I feel I am missing the big picture here, as I would expect that you just check the values assigned to the stack pointer to see if they are within range. Instead I see some local tracking and canBeSubtract etc. which seems to do something more complex and specific..?
Comments might have helped avoid my confusion :)
| } | ||
| wasm.updateMaps(); | ||
|
|
||
| if (checkStackOverflow && !isSideModule) { |
There was a problem hiding this comment.
instead of silently doing nothing for side modules, how about Fatal() << "stack overflow checks are not supported with side modules"; ?
There was a problem hiding this comment.
Well, we actually need to do nothing for side modules because all stack operations gets turned into stackSave and stackRestore, which are exported from the main module. In the main module, stackRestore performs the overflow check.
There was a problem hiding this comment.
Is that right? I thought side modules manage their own stack in their own memory. At least that's how fastcomp does it - is it completely different in the wasm backend?
There was a problem hiding this comment.
Seems like it. This is the prologue of a function in the side module:
(func $f (; 5 ;) (type $2) (result i32)
(local $0 i32)
(local $1 i32)
(local $2 i32)
...
(local.set $0
(call $stackSave)
)
(local.set $1
(i32.const 8192)
)
(local.set $2
(i32.sub
(local.get $0)
(local.get $1)
)
)
(call $stackRestore
(local.get $2)
)and this is the epilogue:
(local.set $7
(i32.const 8192)
)
(local.set $8
(i32.add
(local.get $2)
(local.get $7)
)
)
(call $stackRestore
(local.get $8)
)stackSave and stackRestore are imported:
(import "env" "stackSave" (func $stackSave (result i32)))
(import "env" "stackRestore" (func $stackRestore (param i32)))There was a problem hiding this comment.
That's very surprising. Let's see what @sbc100 says. But that doesn't need to block this PR landing (we may need a followup though).
There was a problem hiding this comment.
Why is this surprising? The sp with the wasm backend is mutable global but we don't have any way to import or export mutable globals (yet), so the side module has to manipulate the global via these helper functions.
There was a problem hiding this comment.
I see, thanks @sbc100
I guess I'm used to the fastcomp model where not just threads but shared modules also got their own stack.
|
The local tracking and other stuff are there to avoid an additional check on the function epilogue, which should be incapable of causing a stack overflow. |
|
I see, thanks. I think it's better to just check every assign to the stack pointer. This is a debugging tool so optimizing every bit of perf seems less important than clarity and simplicity. |
|
My question about side modules was marked "resolved" but what's the answer? :) |
kripken
left a comment
There was a problem hiding this comment.
lgtm aside from the side module issue
When
--check-stack-overflowis passed towasm-emscripten-finalize, a function called__set_stack_limitis exported. Calling this function with the low end of the stack will signal to the module how far the stack is allowed to grow.When the stack pointer dips below the value set by
__set_stack_limit, an imported function__handle_stack_overflowis called.If
__set_stack_limitis not called, then stack overflow check is disabled.Refs emscripten-core/emscripten#9039.