Wasmtime GitHub notifications bot (Jun 11 2024 at 01:33): 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_linessection, this commit adds comments to the output ofwasmtime compile --emit-clifindicating 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:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
Wasmtime GitHub notifications bot (Jun 12 2024 at 22:11):jameysharp updated PR #8771.
Wasmtime GitHub notifications bot (Jun 12 2024 at 22:25):jameysharp updated PR #8771.
Wasmtime GitHub notifications bot (Jun 12 2024 at 22:25):jameysharp has marked PR #8771 as ready for review.
Wasmtime GitHub notifications bot (Jun 12 2024 at 22:25):jameysharp requested wasmtime-core-reviewers for a review on PR #8771.
Wasmtime GitHub notifications bot (Jun 12 2024 at 22:25):jameysharp requested pchickey for a review on PR #8771.
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.
Wasmtime GitHub notifications bot (Jun 12 2024 at 23:51):pchickey requested fitzgen for a review on PR #8771.
Wasmtime GitHub notifications bot (Jun 13 2024 at 16:02):fitzgen submitted PR review:
Excited for this! but I have a concern, see below
Wasmtime GitHub notifications bot (Jun 13 2024 at 16:02):fitzgen submitted PR review:
Excited for this! but I have a concern, see below
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.
Wasmtime GitHub notifications bot (Jun 13 2024 at 18:00):jameysharp submitted PR review.
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 addr2lineand how it uses theaddr2linecrate, but I got confused. I had an easier time understanding how to usegimlidirectly.Also,
addr2linewas 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 fixedgimliupstream to be more forgiving of those errors, which are introduced bywasm-opt, but I don't think we've pulled in the new version yet. Maybe we can useaddr2linewithout that feature? I haven't checked.It certainly makes sense to use the
addr2linecrate 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?
Wasmtime GitHub notifications bot (Jun 13 2024 at 21:08):fitzgen submitted PR review.
Wasmtime GitHub notifications bot (Jun 13 2024 at 21:08):fitzgen created PR review comment:
I fixed
gimliupstream to be more forgiving of those errors, which are introduced bywasm-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 thanaddr2linelong 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
addr2linewithout that feature? I haven't checked.I think you should just be able to do
find_locationto get the leaf-most source location, rather than an iterator of inlined frames.
Wasmtime GitHub notifications bot (Jun 13 2024 at 22:35):jameysharp submitted PR review.
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 addr2lineagainst my version of gimli and it fixed the worst of the issues I had, although the output was not quite as complete asllvm-addr2lineon that particular input, for reasons I haven't tracked down.Next week I can try to figure out how to use
addr2lineinstead of my hand-rolled cache.
Last updated: Nov 22 2024 at 16:03 UTC