Stream: git-wasmtime

Topic: wasmtime / PR #5580 Fix a debug assert with `wasm_backtra...

Wasmtime GitHub notifications bot (Jan 17 2023 at 02:23):

alexcrichton opened PR #5580 from wasm-backtrace to main:

This commit fixes an issue where when backtraces were disabled but a host function returned an error it would trigger a debug assertion within Wasmtime. The fix here is to update the condition of the debug assertion and add a test doing this behavior to ensure it works in the future.

I've also further taken the liberty in this commit to remove the deprecation notice for Config::wasm_backtrace. We don't really have a strong reason for removing this functionality at this time and users have multiple times now reported issues with performance that seem worthwhile to keep the option. The latest issue, #5577, has a use case where it appears the quadratic behavior is back in a way that Wasmtime won't be able to detect. Namely with lots of wasm interleaved with host on the stack if the original error isn't threaded through the entire time then each host error will trigger a new backtrace since it doesn't see a prior backtrace in the error being returned.

While this could otherwise be fixed with only capturing one contiguous backtrace perhaps this seems reasonable enough to leave the wasm_backtrace config option for now.

Closes #5577

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Jan 17 2023 at 02:23):

alexcrichton has marked PR #5580 as ready for review.

Wasmtime GitHub notifications bot (Jan 17 2023 at 02:23):

alexcrichton requested fitzgen for a review on PR #5580.

Wasmtime GitHub notifications bot (Jan 17 2023 at 17:32):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Jan 17 2023 at 17:32):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Jan 17 2023 at 17:32):

fitzgen created PR review comment:

Shouldn't we just set needs_backtrace = false when the store has backtraces disabled, rather than having two places we need to check for consistency here?

Wasmtime GitHub notifications bot (Jan 17 2023 at 19:14):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jan 17 2023 at 19:14):

alexcrichton created PR review comment:

I agree yeah it would be good but it's not trivial at least for now. Some refactoring could probably fix this but it doesn't seem too worth it at this time so I'm going to go ahead and merge and leave this for a future rainy day

Wasmtime GitHub notifications bot (Jan 17 2023 at 19:14):

alexcrichton merged PR #5580.

Last updated: Dec 23 2024 at 12:05 UTC