Skip to content

Conversation

@sbc100
Copy link
Collaborator

@sbc100 sbc100 commented May 25, 2022

When running a worker we don't want to write the stack cookie until the
worker is actually running a pthread. This is done in
establishStackSpace.

For this reason we don't want to run writeStackCookie during
run (which is used only to initiialize the worker, before its
running a thread).

The extra call to writeStackCookie was not harmfull but its writing
the default stack location (the one used by the main thread!).

@sbc100 sbc100 force-pushed the pthread_stack_check branch from 6acab22 to 33bbfa5 Compare May 25, 2022 22:32
@sbc100 sbc100 requested a review from kripken May 25, 2022 22:33
Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC pthreads will now only call writeStackCookie() - which they always have - but no longer call stackCheckInit() - which they used to before this PR, but that was wrong. But do we not need the other parts of stackCheckInit(), namely emscripten_stack_set_limits /emscripten_stack_init?

@sbc100
Copy link
Collaborator Author

sbc100 commented May 26, 2022

IIUC pthreads will now only call writeStackCookie() - which they always have - but no longer call stackCheckInit() - which they used to before this PR, but that was wrong. But do we not need the other parts of stackCheckInit(), namely emscripten_stack_set_limits /emscripten_stack_init?

I think the answer is that pthreads do all this stuff in establishStackSpace:

$establishStackSpace: function() {
var pthread_ptr = _pthread_self();
var stackTop = {{{ makeGetValue('pthread_ptr', C_STRUCTS.pthread.stack, 'i32') }}};
var stackSize = {{{ makeGetValue('pthread_ptr', C_STRUCTS.pthread.stack_size, 'i32') }}};
var stackMax = stackTop - stackSize;
#if PTHREADS_DEBUG
err('establishStackSpace: ' + stackTop + ' -> ' + stackMax);
#endif
#if ASSERTIONS
assert(stackTop != 0);
assert(stackMax != 0);
assert(stackTop > stackMax);
#endif
// Set stack limits used by `emscripten/stack.h` function. These limits are
// cached in wasm-side globals to make checks as fast as possible.
_emscripten_stack_set_limits(stackTop, stackMax);
#if STACK_OVERFLOW_CHECK >= 2
// Set stack limits used by binaryen's `StackCheck` pass.
// TODO(sbc): Can this be combined with the above.
___set_stack_limits(stackTop, stackMax);
#if MAIN_MODULE
// With dynamic linking we could have any number of pre-loaded libraries
// that each need to have their stack limits set.
setDylinkStackLimits(stackTop, stackMax);
#endif
#endif
// Call inside wasm module to set up the stack frame for this pthread in wasm module scope
stackRestore(stackTop);
#if STACK_OVERFLOW_CHECK
// Write the stack cookie last, after we have set up the proper bounds and
// current position of the stack.
writeStackCookie();
#endif
},

emscripten_stack_set_limits and emscripten_stack_init both do the same job so only one of them should be called.

In the case of a pthread its the explicit emscripten_stack_set_limits that is called from establishStackSpace.

@sbc100 sbc100 force-pushed the pthread_stack_check branch 4 times, most recently from 7204fee to 864aebb Compare May 26, 2022 02:41
When running a worker we don't want to write the stack cookie until the
worker is actually running a pthread.  This is done in
`establishStackSpace`.

For this reason we don't want to run `writeStackCookie` during
`run` (which is used only to initiialize the worker, before its
running a thread).
@sbc100 sbc100 requested a review from kripken May 26, 2022 17:59
@sbc100 sbc100 force-pushed the pthread_stack_check branch from 864aebb to 943e0a5 Compare May 26, 2022 17:59
@sbc100 sbc100 merged commit b2dc6cf into main May 27, 2022
@sbc100 sbc100 deleted the pthread_stack_check branch May 27, 2022 04:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants