Set compiler flag 'unwind_info' to false if specified in config

[TimonPost]

Regarding issue #6541, it was reported that unloading a large module on macOS can result in significantly long loading times. One suggestion was to disable unwind frames, but this didn't seem to have any effect. Upon further investigation, it appears that this issue may be a regression introduced in a commit made last year in August. The specific commit can be found here: link to commit.

Currently, the config flag is enabled by default, and there is no option to turn it off via the native_unwind_info function. This pull request addresses the issue.

I would like to have it confirmed if this means that all crane lift users have been forced to compile with unwind frames, always, or perhaps I am missing something.

Also, second question, disabling this flag, it would only affect GC tracing and debugging right? So should be fine to expose this to users if they use neither of those?

As temporary workaround i can do:

unsafe {
    config.native_unwind_info(false);
    config.cranelift_flag_set("unwind_info", "false");
}

[github-actions]

Subscribe to Label Action

cc @peterhuene

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 .github/subscribe-to-label.json configuration file.

Learn more.

[github-actions]

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.

  Our documentation should be of the following form:

  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.
  

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

  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 (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 for more 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.

---

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

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

Learn more.

[jameysharp]

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.

[TimonPost]

Good points! I just mimicked the other unwind function, can we lift the conversation to #6541, or would you like a new issue, or continue here? I can make this PR a draft in the meantime.

[jameysharp]

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.

[fitzgen]

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.

[fitzgen]

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

[alexcrichton]

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.

[fitzgen]

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.

[fitzgen]

LGTM, modulo the new revelations around profiling. Thanks!

[jameysharp]

Okay, I think we have consensus on the conditions where this setting is needed, and now just need to agree on implementation.

[jameysharp]

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.

[jameysharp]

I failed to submit one of my review comments, whoops?

[bjorn3]

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.

[jameysharp]

@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?

[bjorn3]

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.

[jameysharp]

Great, I think this is all set. Thank you!

[fitzgen]

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.

[jameysharp]

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)

[alexcrichton]

That looks like the right fix to me 👍

[rockwotj]

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

[TimonPost]

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

[TimonPost]

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

[github-actions]

Subscribe to Label Action

cc @fitzgen

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 .github/subscribe-to-label.json configuration file.

Learn more.

[rockwotj]

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).

[jameysharp]

This PR still looks good to me, thanks for fixing the Windows fuzzing issue!

The CI failure is probably because you accidentally changed crates/wasi-nn/spec. I suspect you need to run git submodule update to undo that, and then hopefully everything will be fine.