Remove guesswork from getStackPointerGlobal. NFC#8679
Conversation
kripken
left a comment
kripken
left a comment
There was a problem hiding this comment.
Does this mean emscripten will need wasm-ld to always emit modules with the name section? I guess we'll need to strip it in optimized builds?
|
|
sbc100
force-pushed
the
stack_pointer_name
branch
2 times, most recently
from
0bd8f34 to
c13f503
Compare
|
Requiring a name section seems brittle? It could be stripped, or not updated after a transform? |
|
From the emscipten side this is not brittle. wasm-emscripten-finalize is always the first thing we do after linking and this pass only ever runs as part of I don't know that there are any other users of this pass other than emscripten? |
Do you mean during the same run of binaryen that uses the name? The name section is only depended on here when the module is read in. If we strip the name section I don't think that changes the |
|
if it turns out there are other users, and they don't want to use the name section we can perhaps introduce a flag to specify which global represents the stack pointer? |
Without this the github CI often shows the errors in confusing locations interspersed with the test names. I confirmed in #8679 that this addresses the issue. Maybe there is a better place to do this but this is good improvement for now.
|
WDYT @dschuff are you ok with this? |
dschuff
left a comment
dschuff
left a comment
There was a problem hiding this comment.
Yeah, I guess if this really is just for an emscripten feature it doesn't matter much and we can always update it again later.
We were just assuming that if no `__stack_pointer` was imported then the first global must be the `__stack_pointer`. Instead we can just require that the global be names correctly in the name section. This will require an emscripten-side change to ensure names are generated when running this pass. Needed for fixing emscripten-core/emscripten#24964.
This change is needed so that WebAssembly/binaryen#8679 can roll in. The combination will then fix #24964
3 participants
We were just assuming that if no
__stack_pointerwas imported then the first global must be the__stack_pointer.Instead we can just require that the global be names correctly in the name section. This will require an emscripten-side change to ensure names are generated when running this pass.
Needed for fixing emscripten-core/emscripten#24964.