Stream: git-wasmtime

Topic: wasmtime / PR #8771 wasmtime: Annotate emit-clif output w...


view this post on Zulip Wasmtime GitHub notifications bot (Jun 11 2024 at 01:33):

jameysharp opened PR #8771 from jameysharp:dwarf-lines to bytecodealliance:main:

When we're compiling a WebAssembly module that contains a DWARF .debug_lines section, this commit adds comments to the output of wasmtime compile --emit-clif indicating which file/line/column each block of CLIF instructions originated from.

This is useful when trying to understand why we're generating the code we do when there's a lot of WebAssembly in a single function. That can happen either because there's a lot of source code in that function, or because the toolchain (e.g. LLVM) inlined a lot of other functions into it before generating WebAssembly.

<!--
Please make sure you include the following information:

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

view this post on Zulip Wasmtime GitHub notifications bot (Jun 12 2024 at 22:11):

jameysharp updated PR #8771.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 12 2024 at 22:25):

jameysharp updated PR #8771.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 12 2024 at 22:25):

jameysharp has marked PR #8771 as ready for review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 12 2024 at 22:25):

jameysharp requested wasmtime-core-reviewers for a review on PR #8771.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 12 2024 at 22:25):

jameysharp requested pchickey for a review on PR #8771.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 12 2024 at 23:51):

pchickey commented on PR #8771:

This looks great to me, but I'm going to re-assign reviewer to Nick because he knows this domain a lot better than me.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 12 2024 at 23:51):

pchickey requested fitzgen for a review on PR #8771.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 13 2024 at 16:02):

fitzgen submitted PR review:

Excited for this! but I have a concern, see below

view this post on Zulip Wasmtime GitHub notifications bot (Jun 13 2024 at 16:02):

fitzgen submitted PR review:

Excited for this! but I have a concern, see below

view this post on Zulip Wasmtime GitHub notifications bot (Jun 13 2024 at 16:02):

fitzgen created PR review comment:

We already depend on https://github.com/gimli-rs/addr2line/ in the workspace and it already does lazy parsing and caching to make repeated lookups fast -- is there a particular reason you didn't reach for that crate? Because this is a fair amount of code and there are a lot of corner cases with interpreting DWARF, especially when you start trying to be robust against buggy producers and toolchains. If there isn't a strong motivation to avoid that crate here, I'd much rather rely upon it.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 13 2024 at 18:00):

jameysharp submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 13 2024 at 18:00):

jameysharp created PR review comment:

Not a great reason, no. I started off by looking at the implementation of wasm-tools addr2line and how it uses the addr2line crate, but I got confused. I had an easier time understanding how to use gimli directly.

Also, addr2line was relying on parts of the DWARF that were constructed incorrectly in the wasm binary I was looking at: in addition to file/line number, it also tried to find which (possibly inlined) function frames included each instruction. I fixed gimli upstream to be more forgiving of those errors, which are introduced by wasm-opt, but I don't think we've pulled in the new version yet. Maybe we can use addr2line without that feature? I haven't checked.

It certainly makes sense to use the addr2line crate here but I suspect it'll be a bit before I can get the details right. How would you feel about merging this version now and coming back to this later?

view this post on Zulip Wasmtime GitHub notifications bot (Jun 13 2024 at 21:08):

fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 13 2024 at 21:08):

fitzgen created PR review comment:

I fixed gimli upstream to be more forgiving of those errors, which are introduced by wasm-opt, but I don't think we've pulled in the new version yet.

Have you checked if using that fixed version as a git dep works? If it does, I'd prefer just asking philipc to cut a release and then updating to it in a day or two.

If there are still issues on main, then I guess we could land this as-is, but I don't have confidence that we will do better here than addr2line long term, given that (a) we already use that elsewhere and (b) that crate is used by a bunch of folks (eg the firefox profiler) and is more likely to get bugs shaken out of it from sheer usage than this thing will.

Maybe we can use addr2line without that feature? I haven't checked.

I think you should just be able to do find_location to get the leaf-most source location, rather than an iterator of inlined frames.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 13 2024 at 22:35):

jameysharp submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 13 2024 at 22:35):

jameysharp created PR review comment:

philipc was delightfully prompt about cutting a new gimli release with my patches, we just have to pull that release in. And I believe I did test wasm-tools addr2line against my version of gimli and it fixed the worst of the issues I had, although the output was not quite as complete as llvm-addr2line on that particular input, for reasons I haven't tracked down.

Next week I can try to figure out how to use addr2line instead of my hand-rolled cache.


Last updated: Dec 23 2024 at 12:05 UTC