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

Run Wasm code on a separate stack #2807

Merged
merged 13 commits into from
Mar 16, 2022
Merged

Run Wasm code on a separate stack #2807

merged 13 commits into from
Mar 16, 2022

Conversation

Amanieu
Copy link
Contributor

@Amanieu Amanieu commented Feb 25, 2022

This uses the corosensei crate to
run Wasm code on a separate stack from the main thread stack.

In trap handlers for stack overflows and memory out of bounds accesses,
we can now check whether we are executing on the Wasm stack and reset
execution back to the main thread stack when returning from the trap
handler.

When Wasm code needs to perform an operation which may modify internal
data structures (e.g. growing a memory) then execution must switch back
to the main thread stack using on_host_stack. This is necessary to avoid
leaving internal data structure in an inconsistent state when a stack
overflow happens.

In the future, this can also be used to suspend execution of a Wasm
module (#1127) by modeling it as an async function call.

Fixes #2757
Fixes #2562

@Amanieu Amanieu force-pushed the corosensei branch 2 times, most recently from f1262fb to 3d83d55 Compare February 25, 2022 17:19
void wasmer_unwind(void *JmpBuf) {
platform_jmp_buf *buf = (platform_jmp_buf*) JmpBuf;
platform_longjmp(*buf, 1);
}
Copy link
Member

Choose a reason for hiding this comment

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

Nice work removing the dependency on this c file!

Copy link
Contributor

Choose a reason for hiding this comment

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

That C file had some hack to allow correct debugging on mac. It would be nice if that hack culd be put somewhere else, as a comment or something optionnal?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can use an LLDB command to mask signals:

process handle SIGBUS --notify false --stop false

Copy link
Contributor

@ptitSeb ptitSeb Mar 1, 2022

Choose a reason for hiding this comment

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

no, that's not the issue. It's not about signal trapping.
that line of code
task_set_exception_ports(mach_task_self(), EXC_MASK_BAD_ACCESS, MACH_PORT_NULL, EXCEPTION_DEFAULT, 0);
allow EXC_BAD_ACCESS exception to be routed and trigger segfault. Without it, you get stuck at the exception and can debug anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a reason why this line is hidden behind an #if 0 by default? It seems that mono always does this.

Copy link
Contributor

Choose a reason for hiding this comment

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

No specific reason, I just let it disabled unless activated for debug to avoid changing previous behaviour. But I suppose it can just be enabled.

Copy link
Member

@syrusakbary syrusakbary left a comment

Choose a reason for hiding this comment

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

Should we eliminate vm/probestack.rs along with the PROBESTACK const, wasmer_vm_probestack, and LibCall::Probestack (in vm/libcalls.rs) ?

They seem to not be used any longer (although I'm not really sure)

@Amanieu
Copy link
Contributor Author

Amanieu commented Feb 25, 2022

Should we eliminate vm/probestack.rs along with the PROBESTACK const, wasmer_vm_probestack, and LibCall::Probestack (in vm/libcalls.rs) ?

They seem to not be used any longer (although I'm not really sure)

Cranelift still uses it for stack probing.

@Amanieu Amanieu force-pushed the corosensei branch 2 times, most recently from ef38f0d to 290c84e Compare March 2, 2022 22:59
tests/ignores.txt Outdated Show resolved Hide resolved
Copy link
Contributor

@ptitSeb ptitSeb left a comment

Choose a reason for hiding this comment

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

we need to remember to add some workaround for the macOS task_set_exception_ports(mach_task_self(), EXC_MASK_BAD_ACCESS, MACH_PORT_NULL, EXCEPTION_DEFAULT, 0); debuggin issue...

@Amanieu Amanieu force-pushed the corosensei branch 2 times, most recently from a747bdd to bee530b Compare March 4, 2022 15:49
@syrusakbary
Copy link
Member

bors r+

bors bot added a commit that referenced this pull request Mar 7, 2022
2807: Run Wasm code on a separate stack r=syrusakbary a=Amanieu

This uses the [corosensei](https://crates.io/crates/corosensei) crate to
run Wasm code on a separate stack from the main thread stack.

In trap handlers for stack overflows and memory out of bounds accesses,
we can now check whether we are executing on the Wasm stack and reset
execution back to the main thread stack when returning from the trap
handler.

When Wasm code needs to perform an operation which may modify internal
data structures (e.g. growing a memory) then execution must switch back
to the main thread stack using on_host_stack. This is necessary to avoid
leaving internal data structure in an inconsistent state when a stack
overflow happens.

In the future, this can also be used to suspend execution of a Wasm
module (#1127) by modeling it as an async function call.

Fixes #2757
Fixes #2562


Co-authored-by: Amanieu d'Antras <amanieu@gmail.com>
@syrusakbary
Copy link
Member

bors r-

@bors
Copy link
Contributor

bors bot commented Mar 7, 2022

Canceled.

@syrusakbary
Copy link
Member

Some context on why we did cancel:

We still need to add the macOS debugging workaround and publish a new version of corosensei.

Once that's addressed we should be good to merge

Stack probes must be done before the stack pointer is adjusted. This
ensures that the stack pointer is still within the bounds of the stack
when inspected by the signal handler.
This uses the [corosensei](https://crates.io/crates/corosensei) crate to
run Wasm code on a separate stack from the main thread stack.

In trap handlers for stack overflows and memory out of bounds accesses,
we can now check whether we are executing on the Wasm stack and reset
execution back to the main thread stack when returning from the trap
handler.

When Wasm code needs to perform an operation which may modify internal
data structures (e.g. growing a memory) then execution must switch back
to the main thread stack using on_host_stack. This is necessary to avoid
leaving internal data structure in an inconsistent state when a stack
overflow happens.

In the future, this can also be used to suspend execution of a Wasm
module (#1127) by modeling it as an async function call.

Fixes #2757
Fixes #2562
This is now handled by the generic trampoline code in the engine.
@Amanieu
Copy link
Contributor Author

Amanieu commented Mar 16, 2022

bors r+

@bors
Copy link
Contributor

bors bot commented Mar 16, 2022

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.

Improve stack overflow handling macOS function calls can be optimized
3 participants