-
Notifications
You must be signed in to change notification settings - Fork 725
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
wasm2c: Add macro and tests to allow disabling stack exhaustion checks #2356
Conversation
e1c8c24
to
156483e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
156483e
to
65b531b
Compare
lgtm % comment. Could we call it |
65b531b
to
9c09684
Compare
@keithw Done. Could you sign off on the change when you have a chance? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm % comment. I like how minimal this is...
Just saw @sunfishcode's comment on the other thread -- let's hold off on merging (especially since this one is more about sandboxing) to try to get consensus. |
9c09684
to
969d903
Compare
Alright. Fwiw, if we end up not wanting to expose the non-conforming approach on this change, the best path forward for us is to go back to the original commit I had here --- where we add a macro of the form |
Go for it! |
969d903
to
a50cf67
Compare
a50cf67
to
74971e3
Compare
(Continued from #2354)
This PR adds an macro
WASM_RT_NONCONFORMING_STACK_UNCHECKED
to allow the embedder to unsafely remove all stack checks from wasm2c. This would mean the embedder is not conformant with the Wasm spec. This configuration is useful for embedders like RLBox which are unlikely to be affected by stack exhaustion, and don't want to pay performance penalties for this. The wasm2c build in CI that uses rlbox flags has been correspondingly updated to capture this.