Stream: git-wasmtime

Topic: wasmtime / PR #5049 Don't re-capture backtraces when prop...

Wasmtime GitHub notifications bot (Oct 12 2022 at 15:21):

fitzgen requested alexcrichton for a review on PR #5049.

Wasmtime GitHub notifications bot (Oct 12 2022 at 15:21):

fitzgen opened PR #5049 from keep-existing-trap-backtraces to main:

This fixes some accidentally quadratic code where we would re-capture a Wasm
stack trace (takes O(n) time) every time we propagated a trap through a host
frame back to Wasm (can happen O(n) times). And O(n) * O(n) = O(n^2), of
course. Whoops. After this commit, it trapping with a call stack that is n
frames deep of Wasm-to-host-to-Wasm calls just captures a single backtrace and
is therefore just a proper O(n) time operation, as it is intended to be.

Now we explicitly track whether we need to capture a Wasm backtrace or not when
raising a trap. This unfortunately isn't as straightforward as one might hope,
however, because of the split between wasmtime::Trap and
wasmtime_runtime::Trap. We need to decide whether or not to capture a Wasm
backtrace inside wasmtime_runtime but in order to determine whether to do that
or not we need to reflect on the anyhow::Error and see if it is a
wasmtime::Trap that already has a backtrace or not. This can't be done the
straightforward way because it would introduce a cyclic dependency between the
wasmtime and wasmtime-runtime crates. We can't merge those two Trap
types-- at least not without effectively merging the whole wasmtime and
wasmtime-runtime crates together, which would be a good idea in a perfect
world but would be a ton of ocean boiling from where we currently are --
because wasmtime::Trap does symbolication of stack traces which relies on
module registration information data that resides inside the wasmtime crate
and therefore can't be moved into wasmtime-runtime. We resolve this problem by
adding a boolean to wasmtime_runtime::raise_user_trap that controls whether we
should capture a Wasm backtrace or not, and then determine whether we need a
backtrace or not at each of that function's call sites, which are in wasmtime
and therefore can do the reflection to determine whether the user trap already
has a backtrace or not. Phew!

Fixes #5037

Wasmtime GitHub notifications bot (Oct 12 2022 at 15:35):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Oct 12 2022 at 15:35):

alexcrichton created PR review comment:

IIRC this has a OnceCell initialized internally here which ignores insertion of a backtrace over a prior backtrace. Could some debug assertions and such be added around here? For example can this debug assert that needs_backtrace is the same as backtrace.is_some()? Additionally record_backtrace could debug_assert! that insertion is always successful to.

I'm not sure if we can actually trip the assertions but it would probably be good to be aware of any cases if there are.

Wasmtime GitHub notifications bot (Oct 12 2022 at 15:35):

alexcrichton created PR review comment:

This snippet is shared amongst a few places below as well, so couldd a helper function be added to the wasmtime crate which is the raise_trap(e: Error) of old?

Wasmtime GitHub notifications bot (Oct 12 2022 at 15:35):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Oct 12 2022 at 18:41):

fitzgen updated PR #5049 from keep-existing-trap-backtraces to main.

Wasmtime GitHub notifications bot (Oct 13 2022 at 14:22):

fitzgen merged PR #5049.

Last updated: Nov 22 2024 at 16:03 UTC