StackCheck: Check both under and overflow #3091
Conversation
|
Given that we are trying to remove this stuff ASAP it seems strange to make this change now but lgtm I guess given that its done |
|
We are trying to remove this from running by default, but I think we will always want a safe stack mode? Or maybe I'm not sure what you mean. |
|
I mean we should be able to completely remove the __set_stack_limit function since the linker or at least finalize should know the stack bounds, once we remove all the post-link static allocations. This will allow stack checks to run in standalone mode too. |
|
Oh, I see. I'll close this then. What is the proper way to find the valid stack region's top and bottom? (after we remove all static allocations) |
|
I think its still ok to land this. Thinking about it I think we would need to pass the top and bottom values to binaryen since we want it to work on non-emscripten-built binaries too, right? But at least the values can be set a post-link time and not at runtime? |
|
Hmm yeah, on second thought let's land this, and then get that full testing PR landed on emscripten too. Then when we replace it with something better we'll have the tests to verify we did it right. |
|
And yeah, we may want to keep a variation of this pass that gets the range as an input. But in emscripten at least I hope we can add a little C code that gets the valid stack region in the same way as we can soon get the heap base. |
Code size improvements + stack improvements are inbound, WebAssembly/binaryen#3089 WebAssembly/binaryen#3091
Depends on WebAssembly/binaryen#3091 This both enables the check, and adds a mode that checks many small allocations instead of one huge one. That ensures we check both the lower and upper limits of the stack region, and so are safe from the overflow issue mentioned in #9039 (comment) Also fix when the initialization happened - it was in callMain but that didn't help cases without main, or using minimal runtime. Also fix the skipping in test_core.py which looked for the old option. With that fixed, wasm2ss (the test mode that runs with full stack checks) all passes. Fixes #9039 and re-enables `test_safe_stack_dylink` after the binaryen roll + these fixes.
See emscripten-core/emscripten#9039 (comment)
The valid stack area is a region
[A, B]in memory. Previously we just checked thatnew stack positions
SwereS >= A, which prevented us from growing too much(the stack grows down). But that only worked if the growth was small enough to not
overflow and become a big unsigned value. This PR makes us check the other way
too, which requires us to know where the stack starts out at.
This still supports the old way of just passing in the growth limit. We can remove it
after the roll.
In principle this can all be done on the LLVM side too after emscripten-core/emscripten#12057
but I'm not sure of the details there, and this is easy to fix here and get testing up
(which can help with later LLVM work).
This helps emscripten-core/emscripten#11860 by allowing us to clean up
some fastcomp-specific stuff in tests.