emscripten-core / emscripten-core/emscripten
Binaryen's stack check pass duplicates its own copies of __stack_base and __stack_end variables
- Dominant language
- C++
- Stars
- 27.6k
- Forks
- 3.6k
- Avg merge
- 1d 1h
- Merged PRs (30d)
- 105
Description
We have the `__stack_base` and `__stack_end` variables defined in file stack_limits.S:
https://github.com/emscripten-core/emscripten/blob/ed8acb3ee53a2d85fb6da73d32efa19b0990088c/system/lib/compiler-rt/stack_limits.S#L21-L24
When building in `-sSTACK_OVERFLOW_CHECK=2` mode, Binaryen emits its own stack variables (`__stack_base` and `__stack_limit`), at
https://github.com/WebAssembly/binaryen/blob/e8cc7965ffdc8926734079f8188408140a04fe7a/src/passes/StackCheck.cpp#L146
It then expects that the startup runtime defines those variables by calling the function `__set_stack_limits`:
https://github.com/WebAssembly/binaryen/blob/e8cc7965ffdc8926734079f8188408140a04fe7a/src/passes/StackCheck.cpp#L36
Wasm Workers consolidates the initialization of all of `__stack_base`, `__stack_end`, `__stack_pointer` and `__tls_base` into one compact function:
https://github.com/emscripten-core/emscripten/blob/ed8acb3ee53a2d85fb6da73d32efa19b0990088c/system/lib/compiler-rt/stack_limits.S#L84-L85
Because these variables are duplicated by Binaryen's StackCheck pass, I would need to initialize also Binaryen's duplicate versions of these stack variables here.
To do that, I try to call `__set_stack_limits` from the .S file:
```S
global.get __stack_base
global.get __stack_end
.functype __set_stack_limits (PTR, PTR) -> ()
call __set_stack_limits
```
however, I get the following error:
```
E:\code\emsdk\emscripten\main>emcc tests\wasm_worker\thread_stack.c -o a.html -sWASM_WORKERS -sSTACK_OVERFLOW_CHECK=2
Fatal: Module::addFunction: __set_stack_limits already exists
emcc: error: 'E:/code/emsdk/binaryen/main_vs2019_64bit_binaryen\bin\wasm-emscripten-finalize --dyncalls-i64 --check-stack-overflow a.wasm -o a.wasm --detect-features' failed (returned 1)
```
My interpretation of this error is that by declaring that function to be called from within compiled code, there will be an undefined reference to `__set_stack_limits` in the compiled code, so StackCheck.cpp refuses to add a definition to it?
Looking at the existing code, this function has only ever been called from JS side so far:
```
E:\code\emsdk\emscripten\main\src\library_async.js:
440
441 #if STACK_OVERFLOW_CHECK >= 2
442: Module['___set_stack_limits'](stack_base, stack_max);
443 #endif
444
E:\code\emsdk\emscripten\main\src\library_dylink.js:
575 }
576 #if STACK_OVERFLOW_CHECK >= 2
577: moduleExports['__set_stack_limits'](_emscripten_stack_get_base(), _emscripten_stack_get_end())
578 #endif
579
E:\code\emsdk\emscripten\main\src\library_pthread.js:
1016 // Set stack limits used by `emscripten/stack.h` function. These limits are
1017 // cached in wasm-side globals to make checks as fast as possible.
1018 _emscripten_stack_set_limits(stackTop, stackMax);
1019 // Set stack limits used by binaryen's `StackCheck` pass.
1020 // TODO(sbc): Can this be combined with the above.
1021: ___set_stack_limits(stackTop, stackMax);
E:\code\emsdk\emscripten\main\src\library_wasm_worker.js:
59 _emscripten_wasm_worker_initialize(m['sb'], m['sz']);
60 // The above function initializes the stack for this Worker, but C code cannot
61: // call to extern __set_stack_limits() function, or Binaryen breaks with
62: // "Fatal: Module::addFunction: __set_stack_limits already exists".
63 // So for now, invoke the function from JS side. TODO: remove this in the future.
64 #if STACK_OVERFLOW_CHECK >= 2
65: ___set_stack_limits(_emscripten_stack_get_base(), _emscripten_stack_get_end());
66 #endif
67
E:\code\emsdk\emscripten\main\src\postamble_minimal.js:
81 writeStackCookie();
82 #if STACK_OVERFLOW_CHECK >= 2
83: ___set_stack_limits(_emscripten_stack_get_base(), _emscripten_stack_get_end());
84 #endif
85 #endif
E:\code\emsdk\emscripten\main\src\preamble.js:
375 #if STACK_OVERFLOW_CHECK >= 2
376 #if RUNTIME_LOGGING
377: err('__set_stack_limits: ' + _emscripten_stack_get_base() + ', ' + _emscripten_stack_get_end());
378 #endif
379: ___set_stack_limits(_emscripten_stack_get_base(), _emscripten_stack_get_end());
380 #endif
381 <<< ATINITS >>>
```
Could we:
1. remove Binaryen from generating its own stack check variables altogether, and just reuse the main `__stack_base` and `__stack_end` variables? That way we would not have duplicate globals for the same end, and all the above `#if STACK_OVERFLOW_CHECK >= 2` places would become obsolete.
2. alternatively, can Binaryen's StackCheck.cpp pass emit the `__set_stack_limits` function even if there are undefined references to it already, so that it would be possible to call it from a .S file? That could also make some of these STACK_OVERFLOW_CHECK locations obsolete.
Contributor guide
Assessment
This issue has not been assessed yet.