Stream: git-wasmtime

Topic: wasmtime / issue #6547 Set compiler flag 'unwind_info' to...

Wasmtime GitHub notifications bot (Jun 09 2023 at 11:45):

github-actions[bot] commented on issue #6547:

Subscribe to Label Action

cc @peterhuene

<details>
This issue or pull request has been labeled: "wasmtime:api", "wasmtime:config"

Thus the following users have been cc'd because of the following labels:

• peterhuene: wasmtime:api

To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

Wasmtime GitHub notifications bot (Jun 09 2023 at 11:45):

github-actions[bot] commented on issue #6547:

Label Messager: wasmtime:config

It looks like you are changing Wasmtime's configuration options. Make sure to
complete this check list:

• [ ] If you added a new Config method, you wrote extensive documentation for
  it.

  <details>

  Our documentation should be of the following form:

  ```text
  Short, simple summary sentence.

  More details. These details can be multiple paragraphs. There should be
  information about not just the method, but its parameters and results as
  well.

  Is this method fallible? If so, when can it return an error?

  Can this method panic? If so, when does it panic?

  Example

  Optional example here.
  ```

  </details>

• [ ] If you added a new Config method, or modified an existing one, you
  ensured that this configuration is exercised by the fuzz targets.

  <details>

  For example, if you expose a new strategy for allocating the next instance
  slot inside the pooling allocator, you should ensure that at least one of our
  fuzz targets exercises that new strategy.

  Often, all that is required of you is to ensure that there is a knob for this
  configuration option in [wasmtime_fuzzing::Config][fuzzing-config] (or one
  of its nested structs).

  Rarely, this may require authoring a new fuzz target to specifically test this
  configuration. See [our docs on fuzzing][fuzzing-docs] for more details.

  </details>

• [ ] If you are enabling a configuration option by default, make sure that it
  has been fuzzed for at least two weeks before turning it on by default.

[fuzzing-config]: https://github.com/bytecodealliance/wasmtime/blob/ca0e8d0a1d8cefc0496dba2f77a670571d8fdcab/crates/fuzzing/src/generators.rs#L182-L194
[fuzzing-docs]: https://docs.wasmtime.dev/contributing-fuzzing.html

---

<details>

To modify this label's message, edit the <code>.github/label-messager/wasmtime-config.md</code> file.

To add new label messages or remove existing label messages, edit the
<code>.github/label-messager.json</code> configuration file.

Learn more.

</details>

Wasmtime GitHub notifications bot (Jun 09 2023 at 16:51):

jameysharp commented on issue #6547:

I think it might be important to make the native_unwind_info field be an Option<bool> or similar, so that if nobody calls Config::native_unwind_info then we know we can set it whichever way makes sense.

Also, I think the existing error messages, about unwind info being necessary for profiling, might be misleading. On Linux, perf defaults to frame-pointer unwinding, which I believe is sufficient today on x86 since we always produce frame pointers. We also have a Cranelift setting to force frame pointers, which we could use when profiling instead of registering unwind info. I think Nick said we'll need more sophisticated unwinding once we have exception-handling support though.

In short, I think we need to sort out exactly when native unwind info is actually necessary before reviewing this PR in detail, because I think the overall logic may not be right.

Wasmtime GitHub notifications bot (Jun 09 2023 at 16:54):

TimonPost commented on issue #6547:

Yes I just mimicked the other unwind function, can we lift the conversation to https://github.com/bytecodealliance/wasmtime/issues/6541, or would you like a new issue, especially for this? I can make this PR a draft in the meantime.

Wasmtime GitHub notifications bot (Jun 09 2023 at 16:54):

TimonPost edited a comment on issue #6547:

Yes I just mimicked the other unwind function, can we lift the conversation to https://github.com/bytecodealliance/wasmtime/issues/6541, or would you like a new issue, specifically for this? I can make this PR a draft in the meantime.

Wasmtime GitHub notifications bot (Jun 09 2023 at 16:56):

TimonPost edited a comment on issue #6547:

Good points! I just mimicked the other unwind function, can we lift the conversation to https://github.com/bytecodealliance/wasmtime/issues/6541, or would you like a new issue, or continue here? I can make this PR a draft in the meantime.

Wasmtime GitHub notifications bot (Jun 09 2023 at 17:07):

jameysharp commented on issue #6547:

I think it makes sense to discuss when native unwind info is necessary separately from #6541, which is about native unwind info being slow. But I don't think we need to open a new issue for it; this PR is a fine place to sort out the details, in my opinion.

I'm hoping both @bjorn3 and @fitzgen can give more context on what the native unwind info is actually used for.

Wasmtime GitHub notifications bot (Jun 09 2023 at 18:40):

fitzgen commented on issue #6547:

I tested locally, and we do indeed produce identical binaries, both with .eh_frame (I'm on Linux) regardless whether this option is set to true or false. Whoops! We should definitely fix this!

---

Also, I think the existing error messages, about unwind info being necessary for profiling, might be misleading. On Linux, perf defaults to frame-pointer unwinding, which I believe is sufficient today on x86 since we always produce frame pointers. We also have a Cranelift setting to force frame pointers, which we could use when profiling instead of registering unwind info.

A couple small corrections / refinements:

1. perf defaults to frame pointers but doesn't always use them. In fact, there are libc asm functions that often end up in Wasmtime profiles that don't preserve frame pointers IIRC but do have CFI pragmas, and perf will only capture these correctly when you're using DWARF stack walking. (I may have this inverted, though). In any case, assuming that perf is always in frame pointers mode is a no go.

2. Wasmtime always forces the compiler to emit frame pointers (to enable fast+simple stack walking) regardless of platform. (The Cranelift x86 backend will always emit frame pointers regardless of the Cranelift setting to preserve them or not.)

So with (1), the message saying native unwind info is required for profiling is probably a bit strong, but it also isn't not required. It is subtle, and emitting the unwind info in that case, even if it doesn't end up used by the profiler's exact configuration, doesn't hurt so I'd like to continue to emit native unwind info whenever profiling support is configured.

---

Backing up a bit: the reason we enable native unwind info by default is to be good citizens of the platform and do our best to interoperate with external tooling (such as profilers or debuggers or whatever) that are usually expecting to see the platform's unwind info. We don't rely on it for correctness, which is why we can (modulo bugs) turn it off. I think the good-citizen angle is still well-motivated, so I don't want to change defaults here.

Wasmtime GitHub notifications bot (Jun 09 2023 at 18:44):

fitzgen commented on issue #6547:

Option<bool> and then reading it something like self.native_unwind_info.unwrap_or(DEFAULT_NATIVE_UNWIND_INFO) makes sense to me.

Wasmtime GitHub notifications bot (Jun 12 2023 at 18:05):

alexcrichton commented on issue #6547:

I might have missed some context reading over this, but how come the profiling strategy is consulted when determining whether to enable this? I know that jitdump/perfmap/guest don't use native unwinding information (perf can't find it even if we emit it), although I don't know about vtune. Given that I don't think we should force enable the unwinding information if profiling is enabled, instead sticking to whatever the default was.

Wasmtime GitHub notifications bot (Jun 13 2023 at 18:13):

fitzgen commented on issue #6547:

I might have missed some context reading over this, but how come the profiling strategy is consulted when determining whether to enable this? I know that jitdump/perfmap/guest don't use native unwinding information (perf can't find it even if we emit it), although I don't know about vtune. Given that I don't think we should force enable the unwinding information if profiling is enabled, instead sticking to whatever the default was.

Ah, I must be operating on outdated information (just like the configuration code itself must have been) because I was under the impression that perf used this information when configured to consume DWARF.

If not, then yeah, we can probably decouple profiling and native unwind info.

Wasmtime GitHub notifications bot (Jun 13 2023 at 19:52):

jameysharp commented on issue #6547:

Hey @fitzgen, the description of Cranelift's unwind_info setting says:

This increases metadata size and compile time, but allows for the debugger to trace frames, is needed for GC tracing that relies on libunwind (such as in Wasmtime), and is unconditionally needed on certain platforms (such as Windows) that must always be able to unwind.

Does Wasmtime GC still require libunwind? I assumed the fast stack walking is used there now.

I'm wondering if we should make this Cranelift setting default to "false" at some point.

Wasmtime GitHub notifications bot (Jun 13 2023 at 22:03):

bjorn3 commented on issue #6547:

I'm wondering if we should make this Cranelift setting default to "false" at some point.

That would break panic backtraces. The only way wasmtime can influence panic backtrace printing is by registering unwindinfo. It can't hook unwinding.

Wasmtime GitHub notifications bot (Jun 13 2023 at 22:34):

jameysharp commented on issue #6547:

@bjorn3 I'm confused. Do you mean it would break backtraces for cg-clif?

For Wasmtime, as far as I understand:

• It should be impossible to panic the host while executing guest code.
• If the guest calls back into Rust then that runs within a panic::catch_unwind block, and uses Wasmtime's own stack unwinding to tunnel the panic back out to the next Rust frame on the stack.

If you mean that cg-clif requires this, I think it would be reasonable to require cg-clif to ensure that the unwind-info flag is enabled. As far as I understand I don't think this flag is required for handling panics in Wasmtime. Have I missed something?

Wasmtime GitHub notifications bot (Jun 14 2023 at 05:46):

bjorn3 commented on issue #6547:

I mean in wasmtime. And there not the unwinding, but the unwinding to get a backtrace as printed to the user. Backtrace printing is entirely unaffected by catch_unwind.

Wasmtime GitHub notifications bot (Jun 14 2023 at 08:50):

bjorn3 edited a comment on issue #6547:

I mean in wasmtime. And there not the unwinding to propagate panics to the caller, but the unwinding to get a backtrace as printed to the user. Backtrace printing is entirely unaffected by catch_unwind.

Wasmtime GitHub notifications bot (Jun 14 2023 at 15:45):

fitzgen commented on issue #6547:

Does Wasmtime GC still require libunwind? I assumed the fast stack walking is used there now.

That is correct, we no longer require libunwind since the fast stack walking patches. Those docs are just outdated.

Wasmtime GitHub notifications bot (Jun 14 2023 at 16:13):

jameysharp commented on issue #6547:

CI says that this PR breaks fuzzing because crates/fuzzing/src/generators/config.rs may randomly try to disable unwind info even on Windows. So we need the config generator to not do that on Windows.

I think these configs are never used for cross-compiling, so it should be good enough to use cfg!(target_os = "windows") rather than having to plumb the target through. But I'd appreciate if @fitzgen or @alexcrichton could check me on that.

Assuming that's true, then the to_wasmtime method in crates/fuzzing/src/generators/config.rs can just replace

.native_unwind_info(self.wasmtime.native_unwind_info)

with

.native_unwind_info(cfg!(target_os = "windows") || self.wasmtime.native_unwind_info)

Wasmtime GitHub notifications bot (Jun 14 2023 at 16:16):

alexcrichton commented on issue #6547:

That looks like the right fix to me :+1:

Wasmtime GitHub notifications bot (Aug 24 2023 at 02:38):

rockwotj commented on issue #6547:

@TimonPost would love to see this get merged, is there anything I can do to help?

Wasmtime GitHub notifications bot (Aug 24 2023 at 07:59):

TimonPost commented on issue #6547:

Good one, i'll patch it up, should be ready in a bit!

Wasmtime GitHub notifications bot (Aug 24 2023 at 09:19):

TimonPost commented on issue #6547:

Rebased and updated the PR. @fitzgen can you review this?

Wasmtime GitHub notifications bot (Aug 24 2023 at 10:45):

github-actions[bot] commented on issue #6547:

Subscribe to Label Action

cc @fitzgen

<details>
This issue or pull request has been labeled: "fuzzing"

Thus the following users have been cc'd because of the following labels:

• fitzgen: fuzzing

To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

Wasmtime GitHub notifications bot (Aug 24 2023 at 18:08):

rockwotj commented on issue #6547:

I'm not sure about the CI failures, but just figured I'd popin to say I'm using this patch with #6896 and it fixed some issues with C++ exceptions and wasmtime used in conjunction (there is a lock in libgcc that was having some contention issues).

Last updated: Nov 22 2024 at 16:03 UTC