emscripten-core / emscripten-core/emscripten

Binaryen's stack check pass duplicates its own copies of __stack_base and __stack_end variables

Open
#16,496 2 comments 0 reactions 0 assignees View on GitHub
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.