Fix the setup of the stack pointer in wasm backend+threads mode, It should point to the top of the stack, not the bottom. #8811

vargaz · 2019-06-17T19:36:06Z

No description provided.

kripken

Thanks @vargaz! Please add yourself to AUTHORS.

The CI errors look random, although oddly there are a bunch of pthreads ones. I re-ran that part of the CI to see.

kripken · 2019-06-18T00:46:28Z

system/lib/libc/musl/src/thread/pthread_attr_get.c

@@ -44,7 +44,7 @@ int pthread_attr_getstack(const pthread_attr_t *restrict a, void **restrict addr
 //		return EINVAL;

 	*size = a->_a_stacksize + DEFAULT_STACK_SIZE;
-	*addr = (void *)(a->_a_stackaddr - *size);
+	*addr = (void *)(a->_a_stackaddr);
 	return 0;


I don't understand this change, but then I don't understand the code from before either I think :)

This change is for both fastcomp and the wasm backend, it seems, since it's not ifdef'd? So was this always wrong?

@juj you know this code the best, what do you think?

I'm confused. The base musl implementation does the same thing:
https://github.com/kraj/musl/blob/cbf41860664b2f408653cc2169b454a293096ccf/src/thread/pthread_attr_get.c#L38-L45

and the pthread_attr_getstack doco says:

The base (lowest addressable byte) of the storage shall be addr

So maybe I'm wrong, but the the current behaviour looks right?
What bug is this trying to fix?

juj · 2019-06-18T11:39:12Z

This change does look wrong to me. In musl convention for the _a_stacksize field, the size is offset by a value of DEFAULT_STACK_SIZE. The intent of this is that if one creates a pthread via a memset(threadAttrs, 0, sizeof(pthread_attr_t)); cleared attr field, then the result is to get a thread with default stack size. I.e. the internally encoded value in _a_stacksize field is always DEFAULT_STACK_SIZE less than the actual desired stack size (and it is not possible to create threads with stack size smaller than DEFAULT_STACK_SIZE, which has a value 81920 bytes.

Second, musl has an internal meaning for the _a_stackaddr attribute that it always points to the top end of the stack, exclusive. I.e. the address pointed to by _a_stackaddr is not part of the stack. This does not relate to the convention of if stack grows up or down (on all platforms musl behaves this way).

The function pthread_attr_getstack() is supposed to return the lowest address of the stack, independent of stack grow direction. (https://linux.die.net/man/3/pthread_attr_getstack). So subtracting _a_stacksize + DEFAULT_STACK_SIZE from _a_stackaddr before returning to user should be the right thing to do here.

Perhaps the root issue for stack base vs stack end is somewhere else? The original implementation of pthread_attr_getstack looks proper to me.

juj · 2019-06-18T11:42:18Z

To double-check, what is the convention of STACKTOP and stackRestore() in wasm backend? In wasm backend, stack grows down, so STACKTOP should point to the highest used address on the stack? If so, then stackRestore(max); part of this PR does look proper instead of stackRestore(base);?

vargaz · 2019-06-18T15:14:39Z

Yes, in the wasm backend, the stack grows down, so stackRestore () needs to be called with the top address.
As for pthread_attr_getstack (), in my testing, a->_a_stackaddr points to the lowest address of the stack, not the highest, so substracting the stack size produces an incorrect value, at least with the wasm backend, causing the newly added test to fail. a->a_stackaddr is initialized from t->stack by:

https://github.com/emscripten-core/emscripten/blob/incoming/system/lib/libc/musl/src/thread/pthread_getattr_np.c#L11

t->stack is initialized from threadParams.stackBase:

https://github.com/emscripten-core/emscripten/blob/incoming/src/library_pthread.js#L436

vargaz · 2019-06-18T15:32:52Z

Changed it so pthread_t.stack points to the stack top in the wasm backend, so pthread_attr_getstack() remains the same.

kripken · 2019-06-18T17:34:47Z

Interestingly, this looks like it may fix #8718!

The CI failure on browser.test_emscripten_main_loop looks real - in the pthreads segment there it hits the assert mentioned in the logging, and i see it locally too,

[client logging: /?exception=uncaught exception: abort("Stack overflow! Stack cookie has been overwritten, expected hex dwords 0x89BACDFE and 0x02135467, but received 0x0 4052c000") at jsStackTrace@http://localhost:8888/test.js:1190:13

That check is here. It doesn't look obviously wrong to me - STACK_MAX is the lowest address the stack can reach, when the stack goes down?

vargaz · 2019-06-18T18:13:03Z

STACK_MAX is the highest address, from library_pthread.js:

$establishStackSpaceInJsModule: function(stackBase, stackMax) {
STACK_BASE = STACKTOP = stackBase;
STACK_MAX = stackMax;
},

kripken · 2019-06-18T22:33:49Z

Hmm, then that code may be wrong. We set up the stack as something like

var STACK_BASE = 5246688,
    STACKTOP = STACK_BASE,
    STACK_MAX = 3808;

So max is the maximum the stack can be incremented to (the lowest address, if the stack increments down). Sounds like we need to update a few places in the code to get this right. Let me know if that turns out complicated and if I can help.

vargaz · 2019-06-18T22:35:27Z

Another location is in worker.js:

  {{{ makeAsmExportAndGlobalAssignTargetInPthread('STACK_BASE') }}} = {{{ makeAsmExportAndGlobalAssignTargetInPthread('STACKTOP') }}} = e.data.stackBase;
  {{{ makeAsmExportAndGlobalAssignTargetInPthread('STACK_MAX') }}} = STACK_BASE + e.data.stackSize;

vargaz · 2019-06-18T22:38:15Z

Do these places need an ifdef #WASM_BACKEND ?

kripken · 2019-06-19T00:04:05Z

It's possible, yeah, that we need to ifdef in places like that. But I'm not sure - I don't have it all in my head what the various stack variables all mean.

If you get stuck let me know - I can find time to do a larger refactoring of this code. But I'm hoping it's simple enough to fix this, and we don't need that.

vargaz · 2019-06-19T12:39:42Z

Made a try.
I think the JS variables mean the following:

STACK_BASE is the lowest address.
STACK_MAX is the limit during stack frame allocation.
STACK_TOP is the initial value.

Not sure whenever to use #ifdef WASM or #ifdef WASM_BACKEND.

kripken · 2019-06-19T20:45:21Z

For the wasm backend, WASM_BACKEND is correct (just WASM is true for asm2wasm as well, using the fastcomp backend).

src/library_pthread.js

kripken · 2019-06-19T20:51:38Z

src/worker.js

 #if ASSERTIONS
      assert(threadInfoStruct);
      assert(selfThreadId);
      assert(parentThreadId);
      assert(STACK_BASE != 0);
-      assert(STACK_MAX > STACK_BASE);


how about keeping this assertion for fastcomp, and checking the other direction for the wasm backend?

I don't see this change - was it reverted perhaps?

kripken · 2019-06-19T20:56:05Z

src/library_pthread.js

@@ -1169,8 +1171,15 @@ var LibraryPThread = {

 #if MODULARIZE
  $establishStackSpaceInJsModule: function(stackBase, stackMax) {


To avoid confusion, how about renaming these params lowAddr, highAddr? Or stackLow, stackhigh?

I think we should do the same type of renaming in the rest of library_pthread.js and worker.js, where these terms appear. That is, I think it's risky and confusing to have STACK_BASE mean one thing (the initial location of the stack) but stackBase mean another thing (the lowest address of the stack). On fastcomp they were the same, but on the wasm backend the direction change means we are using the same words for very different things...

Instead, I suggest we can keep the meanings of "base" (initial position), "top" (current position), "max" (position we should not increment past). And for terms like lowest address and highest address, which we need in the pthreads code (e.g. since we malloc to get the lowest address), we should use lowAddr/highAddr or stackLow/stackHigh (I don't have a strong feeling between those pairs). What do you think?

stackBase always means the lowest address everywhere i think, only make needs to be renamed.

I'd suggest renaming STACK_TOP to SP or STACK_POINTER, and STACK_MAX to STACK_LIMIT. stackBase/stackMax could then be used to refer to the lowest/highest address. Would that work ?

stackBase always means the lowest address everywhere i think, only make needs to be renamed.

I don't think that's true for STACK_BASE. Here is what hello world gets:

var STATIC_BASE = 1024, STACK_BASE = 5246688, STACKTOP = STACK_BASE, STACK_MAX = 3808, DYNAMIC_BASE = 5246688, DYNAMICTOP_PTR = 3776;

STACK_BASE is the initial value of the stack pointer, which for the wasm backend is the highest address. And as mentioned above I think it's important we keep STACK_BASE and stackBase consistent.

I'd suggest renaming STACK_TOP to SP or STACK_POINTER, and STACK_MAX to STACK_LIMIT. stackBase/stackMax could then be used to refer to the lowest/highest address. Would that work ?

I'm not picky about the names, but I slightly worry about expanding this PR to such a larger renaming - I think it will end up a nontrivial amount of work. In particular we may want to do the same for DYNAMIC_* and STATIC_* for consistency as they all use the same naming convention now. Another issue is that we hope to get rid of fastcomp eventually - it won't happen soon, but once it does, we'll just have one direction for the stack, so all this will become easier to refactor. So I'd prefer to leave a larger refactoring to a later PR (by you, if you're interested, of course).

However, if you'd rather do it here, then that sounds ok too. Those names sound generally ok. I like that LIMIT makes sense regardless of the direction the stack grows. Instead of base/max how about min/max for the lowest/highest addresses? And for the current location, STACK_POINTER is probably least confusing, but then we'd want DYNAMIC_POINTER, STATIC_POINTER for those other two, which I'm not sure about... but maybe we can leave those aside.

vargaz · 2019-06-20T21:18:02Z

Did some renaming:
STACK_MAX -> STACK_LIMIT
stackBase/stackMax -> stackLow/stackHigh

Should we also rename the variables in the pthread/threadParams structure as well ?

sbc100

Would you mind doing any renaming as separate PR? Its good to separate out non-function changes from the functional ones.

vargaz · 2019-06-20T21:30:53Z

Ok, reverted the last 2 commits.

kripken · 2019-06-20T21:48:33Z

Thanks, this looks good overall, aside from the test comment I just wrote. The test_pthread_attr_getstack seems to fail on CI though, with (test_browser.browser) ... [client logging: /?exception=RuntimeError: index out of bounds / undefined ], so perhaps something is not quite right yet.

…hould point to the top of the stack, not the bottom.

…sl convention.

…west address since the wasm stack grows downwards.

vargaz · 2019-06-20T22:50:32Z

Should be fixed.

vargaz · 2019-06-20T22:53:46Z

The stack cookie check would store to STACK_MAX-1 overwriting the malloc header when using the wasm backend.

kripken · 2019-06-21T19:30:10Z

Great, thanks @vargaz!

…now that #8811 has landed

* After #8811 landed a lot more tests can pass, this enables all those that can. * Remove unnecessary stuff in test_pthread_gcc_atomic_fetch_and_op. * Add more runtime assertions for the stack position. There are still a few tests that don't pass, so this doesn't close #8718. But they are quite few at this point (and look unrelated to stack issues - something else going on there).

) * Set STACK_MAX properly in the wasm backend, it should point to the lowest address since the wasm stack grows downwards. * Fix the stack overflow test to work with the wasm backend.

* After emscripten-core#8811 landed a lot more tests can pass, this enables all those that can. * Remove unnecessary stuff in test_pthread_gcc_atomic_fetch_and_op. * Add more runtime assertions for the stack position. There are still a few tests that don't pass, so this doesn't close emscripten-core#8718. But they are quite few at this point (and look unrelated to stack issues - something else going on there).

I believe these values have always been written the wrong place (off by one word) since they were adapted for the downward growing stack in #8811. In the downward growing stack that last work in the stack lives a `max` and not `max + 4`.

vargaz mentioned this pull request Jun 17, 2019

Fix pthread_attr_getstack (), it should return the base of the stack. #8812

Closed

kripken reviewed Jun 18, 2019

View reviewed changes

kripken mentioned this pull request Jun 18, 2019

Remaining test failures with wasm-backend pthreads #8718

Closed

kripken reviewed Jun 19, 2019

View reviewed changes

sbc100 reviewed Jun 20, 2019

View reviewed changes

vargaz force-pushed the fix-stack branch from bacb3ed to 672a364 Compare June 20, 2019 21:30

vargaz added 7 commits June 20, 2019 18:34

Fix the setup of the stack pointer in wasm backend+threads mode, It s…

253de31

…hould point to the top of the stack, not the bottom.

Fix pthread_attr_getstack (), it should return the base of the stack.

7ef45a5

Store the stack top in pthread_t.stack in the wasm backend per the mu…

42f06a6

…sl convention.

Add to AUTHORS.

ff04f2f

Set STACK_MAX properly in the wasm backend, it should point to the lo…

821f0d2

…west address since the wasm stack grows downwards.

Rename a local to better reflect its purpose. Add back an assertion.

60c2cc3

Fix the stack overflow test to work with the wasm backend.

ffdfd04

vargaz force-pushed the fix-stack branch from 672a364 to ffdfd04 Compare June 20, 2019 22:50

kripken merged commit 4e8ad61 into emscripten-core:incoming Jun 21, 2019

kripken added a commit that referenced this pull request Jun 21, 2019

Add a wasm backend pthreads assertion and enable some pthreads tests …

7963bf3

…now that #8811 has landed

vargaz deleted the fix-stack branch June 21, 2019 19:57

kripken mentioned this pull request Jun 21, 2019

Wasm backend pthreads testing improvements #8842

Merged

sbc100 mentioned this pull request Jun 28, 2019

libvpx data corruption with USE_PTHREADS, MODULARIZE on upstream backend #8887

Closed

sbc100 mentioned this pull request Mar 4, 2022

Fix stack cookie location used by STACK_OVERFLOW_CHECK. NFC #16422

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix the setup of the stack pointer in wasm backend+threads mode, It should point to the top of the stack, not the bottom. #8811

Fix the setup of the stack pointer in wasm backend+threads mode, It should point to the top of the stack, not the bottom. #8811

vargaz commented Jun 17, 2019

kripken left a comment

kripken Jun 18, 2019

VirtualTim Jun 18, 2019

juj commented Jun 18, 2019

juj commented Jun 18, 2019

vargaz commented Jun 18, 2019

vargaz commented Jun 18, 2019

kripken commented Jun 18, 2019

vargaz commented Jun 18, 2019

kripken commented Jun 18, 2019

vargaz commented Jun 18, 2019

vargaz commented Jun 18, 2019

kripken commented Jun 19, 2019

vargaz commented Jun 19, 2019 •

edited

Loading

kripken commented Jun 19, 2019

kripken Jun 19, 2019

vargaz Jun 20, 2019

kripken Jun 20, 2019

kripken Jun 19, 2019 •

edited

Loading

vargaz Jun 19, 2019

vargaz Jun 20, 2019

kripken Jun 20, 2019 •

edited

Loading

vargaz commented Jun 20, 2019

sbc100 left a comment

vargaz commented Jun 20, 2019

kripken commented Jun 20, 2019

vargaz commented Jun 20, 2019

vargaz commented Jun 20, 2019

kripken commented Jun 21, 2019

		@@ -1169,8 +1171,15 @@ var LibraryPThread = {

		#if MODULARIZE
		$establishStackSpaceInJsModule: function(stackBase, stackMax) {

Fix the setup of the stack pointer in wasm backend+threads mode, It should point to the top of the stack, not the bottom. #8811

Fix the setup of the stack pointer in wasm backend+threads mode, It should point to the top of the stack, not the bottom. #8811

Conversation

vargaz commented Jun 17, 2019

kripken left a comment

Choose a reason for hiding this comment

kripken Jun 18, 2019

Choose a reason for hiding this comment

VirtualTim Jun 18, 2019

Choose a reason for hiding this comment

juj commented Jun 18, 2019

juj commented Jun 18, 2019

vargaz commented Jun 18, 2019

vargaz commented Jun 18, 2019

kripken commented Jun 18, 2019

vargaz commented Jun 18, 2019

kripken commented Jun 18, 2019

vargaz commented Jun 18, 2019

vargaz commented Jun 18, 2019

kripken commented Jun 19, 2019

vargaz commented Jun 19, 2019 • edited Loading

kripken commented Jun 19, 2019

kripken Jun 19, 2019

Choose a reason for hiding this comment

vargaz Jun 20, 2019

Choose a reason for hiding this comment

kripken Jun 20, 2019

Choose a reason for hiding this comment

kripken Jun 19, 2019 • edited Loading

Choose a reason for hiding this comment

vargaz Jun 19, 2019

Choose a reason for hiding this comment

vargaz Jun 20, 2019

Choose a reason for hiding this comment

kripken Jun 20, 2019 • edited Loading

Choose a reason for hiding this comment

vargaz commented Jun 20, 2019

sbc100 left a comment

Choose a reason for hiding this comment

vargaz commented Jun 20, 2019

kripken commented Jun 20, 2019

vargaz commented Jun 20, 2019

vargaz commented Jun 20, 2019

kripken commented Jun 21, 2019

vargaz commented Jun 19, 2019 •

edited

Loading

kripken Jun 19, 2019 •

edited

Loading

kripken Jun 20, 2019 •

edited

Loading