Use emscripten_stack_set_limits in wasm_worker_initialize.S #22755
Conversation
sbc100
force-pushed
the
emscripten_stack_set_limits
branch
from
719db6c
to
fd63f2d
Compare
There was a problem hiding this comment.
LGTM, with one tiny nitpick. With those globals being refactored out, I'm wondering what is left before this could just be a simple C function instead of being written in assembly. I see that there is still a global reference to a __stack_pointer that is needed here. I wonder if it would be possible to expose a function that is just emscripten_reset_stack_pointer or something like that, which would just set __stack_pointer to __stack_base? (Also probably not for this PR, I'm just thinking out loud a bit).
The problem is that you cannot safely call any C function until the stack pointer has been set. I mean in theory you could try to second guess the C compiler and try to write your C function in way you are think it won't itself use the stack pointer, but its very fragile. Better to write these very low level startup routines asm I think |
sbc100
force-pushed
the
emscripten_stack_set_limits
branch
from
fd63f2d
to
e481c5f
Compare
| "a.wasm": 1879, | ||
| "a.wasm.gz": 1066, | ||
| "total": 3277, | ||
| "total_gz": 2032 |
There was a problem hiding this comment.
This is great - I'm puzzled to understand how this saved code size though..?
There was a problem hiding this comment.
I'm also a little surprised.
My initial guess was that perhaps emscripten_stack_set_limits was being included prior to this change and so this change reduces overall duplication... but I don't think thats true. I could investigate further but but this seems like a win so I think we just take it.
There was a problem hiding this comment.
I wonder if the earlier sizes were out of date, was there still possibility for slack %?
There was a problem hiding this comment.
I think i double checked that. I normally rebaseline all tests before landing changes like this, just to be sure I'm seeing the real deltas.
Followup to emscripten-core#22683. This change avoids making __stack_base and __stack_end into public/external symbols and avoids reimplementing `emscripten_stack_set_limits`.
sbc100
force-pushed
the
emscripten_stack_set_limits
branch
from
e481c5f
to
68c43f8
Compare
Followup to #22683. This change avoids making __stack_base and __stack_end into public/external symbols and avoids reimplementing
emscripten_stack_set_limits.This is a minor code size win too.