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

wasm2c: Add macro and tests to allow disabling stack exhaustion checks #2356

Merged
merged 1 commit into from
Jan 2, 2024

Conversation

shravanrn
Copy link
Collaborator

(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.

Copy link
Member

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems reasonable to me. IIUC this approach is what you and @keithw disccussed in #2354?

test/run-tests.py Outdated Show resolved Hide resolved
@shravanrn
Copy link
Collaborator Author

Seems reasonable to me. IIUC this approach is what you and @keithw disccussed in #2354?

Yup, I believe so.
@keithw is this what you had in mind?

test/spec-wasm2c-prefix.c Outdated Show resolved Hide resolved
@keithw
Copy link
Member

keithw commented Dec 15, 2023

lgtm % comment. Could we call it WASM_RT_NONCONFORMING_UNCHECKED_STACK_EXHAUSTION or something to be very clear about what's "unchecked" about this?

@shravanrn
Copy link
Collaborator Author

@keithw Done. Could you sign off on the change when you have a chance?

Copy link
Member

@keithw keithw left a 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...

.github/workflows/build.yml Outdated Show resolved Hide resolved
@keithw
Copy link
Member

keithw commented Dec 16, 2023

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.

@shravanrn
Copy link
Collaborator Author

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.

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 WASM_RT_STACK_EXHAUSTION_CHECKED_BY_EMBEDDER which basically say its the host's responsibility to take care of this (by installing signal handlers etc.). We can do what we need to from the rlbox side while wasm2c can say it doesn't promote explicit non-conformance

@shravanrn
Copy link
Collaborator Author

@keithw Given the discussion in #2357, I think we can now retry landing this. Please let me know your thoughts

@keithw
Copy link
Member

keithw commented Dec 31, 2023

Go for it!

@shravanrn shravanrn enabled auto-merge (rebase) January 2, 2024 14:30
@shravanrn shravanrn merged commit b85ecbe into WebAssembly:main Jan 2, 2024
16 checks passed
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

Successfully merging this pull request may close these issues.

None yet

3 participants