Wasmtime GitHub notifications bot (Sep 04 2023 at 15:10): Wasmtime GitHub notifications bot (Sep 04 2023 at 15:10):SingleAccretion opened PR #6960 from SingleAccretion:DI-Vars to bytecodealliance:main:
Implements the plan essentially outlined in https://github.com/bytecodealliance/wasmtime/issues/2856#issuecomment-1192419659:
1) Emit or use the SysV-style unwind info if debug info is requested.
2) Reference that unwind info in the form of.debug_frameusingDW_OP_call_frame_cfa.
3) Plumb the right offsets through.Fixes #3884 (at least to some degree - however, that issue does not contain anything actionable), and also the example I was looking at in C#, though only for
-opt-level 0codegen.
Wasmtime GitHub notifications bot (Sep 04 2023 at 17:16):SingleAccretion updated PR #6960.
Wasmtime GitHub notifications bot (Sep 04 2023 at 17:31):SingleAccretion edited PR #6960:
Implements the plan essentially outlined in https://github.com/bytecodealliance/wasmtime/issues/2856#issuecomment-1192419659:
1) Emit or use SysV-style unwind info if debug info is requested.
2) Reference that unwind info in the form of.debug_frameusingDW_OP_call_frame_cfa.
3) Plumb the right offsets through.Fixes #3884 (at least to some degree - however, that issue does not contain anything actionable), and also the example I was looking at in C#, though only for
-opt-level 0codegen.
Wasmtime GitHub notifications bot (Sep 04 2023 at 18:30):SingleAccretion has marked PR #6960 as ready for review.
Wasmtime GitHub notifications bot (Sep 04 2023 at 18:30):SingleAccretion requested abrown for a review on PR #6960.
Wasmtime GitHub notifications bot (Sep 04 2023 at 18:30):SingleAccretion requested wasmtime-compiler-reviewers for a review on PR #6960.
Wasmtime GitHub notifications bot (Sep 04 2023 at 18:30):SingleAccretion requested pchickey for a review on PR #6960.
Wasmtime GitHub notifications bot (Sep 04 2023 at 18:30):SingleAccretion requested wasmtime-core-reviewers for a review on PR #6960.
Wasmtime GitHub notifications bot (Sep 04 2023 at 20:49):pchickey requested alexcrichton for a review on PR #6960.
Wasmtime GitHub notifications bot (Sep 05 2023 at 19:06):alexcrichton submitted PR review:
Looks all reasonable to me, thanks again for working on this! I'm no expert myself in debuginfo but I don't think we necessarily have a resident expert to turn to, so happy to review at a somewhat higher level and lean on you for the DWARF correctness and all that.
One final comment I'd have in addition to those below is that our current debuginfo tests are pretty primitive, so if you're interested or so inspired I think it'd be great to soup them up. For example I'd love to have "just" a directory of C and/or Rust files which are compiled automatically in CI and the source itself has various special comments for commands/breakpoints/etc.
Wasmtime GitHub notifications bot (Sep 05 2023 at 19:06):alexcrichton submitted PR review:
Looks all reasonable to me, thanks again for working on this! I'm no expert myself in debuginfo but I don't think we necessarily have a resident expert to turn to, so happy to review at a somewhat higher level and lean on you for the DWARF correctness and all that.
One final comment I'd have in addition to those below is that our current debuginfo tests are pretty primitive, so if you're interested or so inspired I think it'd be great to soup them up. For example I'd love to have "just" a directory of C and/or Rust files which are compiled automatically in CI and the source itself has various special comments for commands/breakpoints/etc.
Wasmtime GitHub notifications bot (Sep 05 2023 at 19:06):alexcrichton created PR review comment:
I realize that this is preexisting, so there's no need to fix this, but ideally we wouldn't check in wasm binaries for tests. In the ideal world CI would have a pinned version of wasi-sdk that it downloads and uses to compile these guest programs (as part of the test).
In lieu of this though could you perhaps comment in the test itself how to generate the wasm file? For example what C flags were passed. That's probably sufficient for now and a future commit could come through to delete the wasm files and build them on CI instead.
Wasmtime GitHub notifications bot (Sep 05 2023 at 19:06):alexcrichton created PR review comment:
This makes me pause for a bit for a few reasons. One is could this assert that at most one of these fields are
SystemVunwind information? Hopefully to head off any possible future confusion.The second reason is that in Wasmtime only
unwind_infois consulted meaning that it seems to be like we wouldn't emit the DWARF unwinding information? It also seems though like you're probably running/testing on Windows, so I'm curious if you are how this is working?
Wasmtime GitHub notifications bot (Sep 05 2023 at 19:06):alexcrichton created PR review comment:
To confirm, this specific change is not necessarily relevant to stack slots, it's an independent optimization?
(it's ok to have, just wanted to confirm)
Wasmtime GitHub notifications bot (Sep 05 2023 at 19:44):SingleAccretion submitted PR review.
Wasmtime GitHub notifications bot (Sep 05 2023 at 19:44):SingleAccretion created PR review comment:
Correct.
Wasmtime GitHub notifications bot (Sep 05 2023 at 20:03):SingleAccretion submitted PR review.
Wasmtime GitHub notifications bot (Sep 05 2023 at 20:03):SingleAccretion created PR review comment:
Could this assert that at most one of these fields are SystemV unwind information?
Sure, will add. It's not really _incorrect_ for them to be
{SysV, SysV}, just wasteful in terms of memory.I'm curious how this is working?
This function is a callee of
append_dwarf, which itself adds to the final image. So on Windows we will have.debug_frame, much like the other DWARF sections.It is interesting to note what happens when we are on Unix - in that case the unwind info will be effectively emitted twice (note this is pre-existing behavior). AFAICT from the code the "unwind" part will be emitted into
.eh_frame, so they don't conflict.In effect, the "debug info" use of this information is completely decoupled from the "unwind" use.
Wasmtime GitHub notifications bot (Sep 05 2023 at 20:06):SingleAccretion submitted PR review.
Wasmtime GitHub notifications bot (Sep 05 2023 at 20:06):SingleAccretion created PR review comment:
Agree, binaries in source control seem like a large question mark. Will add the arguments.
Wasmtime GitHub notifications bot (Sep 05 2023 at 20:32):SingleAccretion updated PR #6960.
Wasmtime GitHub notifications bot (Sep 05 2023 at 20:53):alexcrichton submitted PR review:
Ok and no worries, I wanted to confirm for my own curiosity mostly. I think it's reasonable to basically incrementally improve things and any work in this space is very much appreciated!
Wasmtime GitHub notifications bot (Sep 05 2023 at 20:54):alexcrichton has enabled auto merge for PR #6960.
Wasmtime GitHub notifications bot (Sep 05 2023 at 21:58):alexcrichton merged PR #6960.
Last updated: Nov 22 2024 at 16:03 UTC