Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should remove remove (reduce usage of) withStackSave? #21763

Open
sbc100 opened this issue Apr 15, 2024 · 1 comment
Open

Should remove remove (reduce usage of) withStackSave? #21763

sbc100 opened this issue Apr 15, 2024 · 1 comment

Comments

@sbc100
Copy link
Collaborator

sbc100 commented Apr 15, 2024

The withStackSave helper is a convenient way to avoid having the remember to write stackRestore at the end of a JS library function.

However, this construct has some cost. Firstly it create an inner closure object, and secondly it relies on try/finally control flow to restore the stack pointer.

At the time when I originally wrong withStackSave I was under the impression that restore in the stack, even in the exceptional case, was important. However, my new/current understanding is that the shadow stack pointer doesn't need to be restored during unwinding until an exception is caught. i.e. its up the outer try/catch to return the stack pointer.

The other useful think about withStackSave is that for function with multiple exit points one doesn't need to repeat the stackRestore statement for each one. Perhaps for such functions we can keep this helper around, but I suspect that are few functions that have enough exit points to justify this helper.

@sbc100
Copy link
Collaborator Author

sbc100 commented Apr 15, 2024

Currently we have 41 uses of withStackSave in the codebase.

It looks like about 1/2 of them are trivially replaceable with a pair of stackSave/stackRestore calls.

sbc100 added a commit to sbc100/emscripten that referenced this issue Apr 15, 2024
The remaining usage is all in library_wasmfs.js where it make the code
a fair amount more readable to continue to use `withStackSave()`
See emscripten-core#21763
sbc100 added a commit to sbc100/emscripten that referenced this issue Apr 15, 2024
The remaining usage is all in library_wasmfs.js where it make the code
a fair amount more readable to continue to use `withStackSave()`
See emscripten-core#21763
sbc100 added a commit to sbc100/emscripten that referenced this issue Apr 15, 2024
The remaining usage is all in library_wasmfs.js where it make the code
a fair amount more readable to continue to use `withStackSave()`
See emscripten-core#21763
sbc100 added a commit to sbc100/emscripten that referenced this issue Apr 15, 2024
The remaining usage is all in library_wasmfs.js where it make the code
a fair amount more readable to continue to use `withStackSave()`
See emscripten-core#21763
sbc100 added a commit that referenced this issue Apr 16, 2024
The remaining usage is all in library_wasmfs.js where it make the code
a fair amount more readable to continue to use `withStackSave()`
See #21763
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

No branches or pull requests

1 participant