Stream: git-wasmtime

Topic: wasmtime / PR #4183 make backtrace collection a Config fi...

Wasmtime GitHub notifications bot (May 24 2022 at 00:34):

pchickey opened PR #4183 from pch/trap_backtrace_config to main:

<!--

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 (May 24 2022 at 14:18):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (May 24 2022 at 14:18):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (May 24 2022 at 14:18):

alexcrichton created PR review comment:

If you'd like the ref mut here can all be dropped actually

Wasmtime GitHub notifications bot (May 24 2022 at 14:18):

alexcrichton created PR review comment:

It's ok to omit this line now that the one above is unconditionally executed.

Wasmtime GitHub notifications bot (May 24 2022 at 14:18):

alexcrichton created PR review comment:

If you're feeling ambitious one possibility here is to do something like:

struct Trap { reason: TrapReason, backtrace: Option<Backtrace> }

enum TrapReason {
    User(Error),
    Jit(usize),
    Code(TrapCode),
    Oom,
}

enum UnwindReason {
    Panic(Box<dyn Any>),
    Trap(TrapReason),
}

... hm I'm thinking about this and I'd ideally like to use this to prevent the double capture problem I described below. In any case this is fine to defer to a future PR. My rough thinking is that we would replace raise_{lib,user}_trap with simply raise_trap which takes a Trap and conversion from Error or wasmtime::Trap would appropriately create a wasmtime_runtime::Trap which is configured to not capture a backtrace if one is already contained (or something like that). It still involves a lot of converting back and forth between the wasmtime crate and the wasmtime_runtime crate though which is also something I'd like to avoid so this is an underbaked idea.

Wasmtime GitHub notifications bot (May 24 2022 at 14:18):

alexcrichton created PR review comment:

I think it's ok to omit this because because backtraces are necessary on reference types only for gc purposes, but that's unconditionally enabled at runtime.

One thing this'll need to do though is that when enabling unwind information in cranelift it should be done either when reference types are enabled or when wasm backtraces are requested.

Wasmtime GitHub notifications bot (May 24 2022 at 14:18):

alexcrichton created PR review comment:

I think this could probably use try_insert instead to avoid checking the flag twice?

Otherwise we talked a bit about this yesterday and the main reason I wanted this to be an assert originally is that this otherwise means we're spending the time to capture a backtrace which otherwise gets discarded so we ideally wouldn't capture the backtrace in the first place. The use case for this though is pretty niche (and it definitely makes sense given the docs here why this is necessary), so I think it's best to leave something like that to a future refactoring.

Could this add a FIXME though with an issue to avoid capturing the second backtrace here if we already have one?

Wasmtime GitHub notifications bot (May 24 2022 at 20:40):

pchickey submitted PR review.

Wasmtime GitHub notifications bot (May 24 2022 at 20:40):

pchickey created PR review comment:

thanks I can't figure out why I had to put them there at all, I am rusty on my rust!

Wasmtime GitHub notifications bot (May 24 2022 at 20:43):

pchickey updated PR #4183 from pch/trap_backtrace_config to main.

Wasmtime GitHub notifications bot (May 24 2022 at 21:13):

pchickey updated PR #4183 from pch/trap_backtrace_config to main.

Wasmtime GitHub notifications bot (May 24 2022 at 21:28):

pchickey updated PR #4183 from pch/trap_backtrace_config to main.

Wasmtime GitHub notifications bot (May 24 2022 at 22:05):

pchickey updated PR #4183 from pch/trap_backtrace_config to main.

Wasmtime GitHub notifications bot (May 25 2022 at 14:06):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (May 25 2022 at 14:06):

alexcrichton created PR review comment:

I think technically this isn't quite right because capturing a backtrace on trap is still independently controlled from this setting, but this does enable unwinding information to get compiled within a module.

You know as I type this out if wasm backtraces are disabled we should probably conditionally define unwinding information in a module depending on whether it actually uses reference types or not.

Wasmtime GitHub notifications bot (May 25 2022 at 14:06):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (May 25 2022 at 14:06):

alexcrichton created PR review comment:

Could this link to Trap::trace perhaps?

Wasmtime GitHub notifications bot (May 25 2022 at 17:33):

pchickey submitted PR review.

Wasmtime GitHub notifications bot (May 25 2022 at 17:33):

pchickey created PR review comment:

The comment is out of date, so I deleted it. The implementation should now behave as you describe.

Wasmtime GitHub notifications bot (May 25 2022 at 17:38):

pchickey updated PR #4183 from pch/trap_backtrace_config to main.

Wasmtime GitHub notifications bot (May 25 2022 at 17:38):

pchickey has marked PR #4183 as ready for review.

Wasmtime GitHub notifications bot (May 25 2022 at 17:39):

pchickey submitted PR review.

Wasmtime GitHub notifications bot (May 25 2022 at 17:39):

pchickey created PR review comment:

I started down this path and it made a lot of code motion but I didn't get it to pay off. I could put more thought into it another time but for now I'm going to leave it be

Wasmtime GitHub notifications bot (May 25 2022 at 17:57):

pchickey updated PR #4183 from pch/trap_backtrace_config to main.

Wasmtime GitHub notifications bot (May 25 2022 at 18:17):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (May 25 2022 at 19:25):

pchickey merged PR #4183.

Last updated: Nov 22 2024 at 16:03 UTC