Conversation
|
This looks large and complicated, and we don't have deep DWARF expertise here, so I am worried. But let me ask first, as background: what is a |
|
Thanks. “nonzero tombstone” was imprecise shorthand rather than a formal DWARF term. The all-ones address is documented by DWARF issue 200609.1, accepted for DWARF v6, as the reserved address for a non-existent entity: https://dwarfstd.org/issues/200609.1.html LLVM implements this as https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/BinaryFormat/Dwarf.h The max-minus-one value ( https://reviews.llvm.org/D81784 So Binaryen already recognizes I will update the PR wording to use the precise terms and references. I can also split the small tombstone-preservation change from the broader scope-range repair to make the review easier. |
|
Thanks for the info. After reading some of that, I am afraid I don't think I have the expertise to review this. Can you say more about the use case that you want this for? Perhaps there is another way to achieve it. For example, our source maps support is a lot more robust, and maybe that is enough - it does provide source locations through transformations? |
Preserve nonzero tombstones, reject lost or reversed low/high pairs, normalize range lists, and repair parent scope ranges from surviving children. Ambiguous sibling scopes are made unavailable instead of being assigned incorrect code ranges.
eb002d1 to
a16642f
Compare
|
Thanks for following up! Let me clarify the fundamental difference in use case between Source Maps and DWARF, share concrete E2E verification results, provide options for splitting this PR to ease review, and provide reproducible test artifacts demonstrating why this change is essential. 1. Capability Comparison: Source Maps vs. DWARFSource maps and DWARF address two entirely different layers of debugging in WebAssembly:
2. Two Concrete Facts & VerificationFact 1: Source Maps fundamentally cannot inspect or evaluate native variablesWe verified this directly in Chrome DevTools / V8 Inspector Protocol. When paused at a breakpoint inside
Source maps only specify bytecode-to-source-line mappings. They have no protocol representation for variable names, stack offsets, or type layouts. DWARF is strictly required for actual variable inspection in native debuggers (Chrome DevTools DWARF extension, LLDB, GDB). Fact 2: Current
|
|
Thanks for the info, but I'd still like to understand your use case better. Specifically, you say you are doing this:
Can you debug a build without Asyncify? E.g. using JSPI instead, which is much more efficient. JSPI requires a modern browser, but if you are debugging locally, that is not a problem. In general that is what people do: use source maps for stack traces, and for full local debugging, use DWARF without Asyncify or wasm-opt. |
|
Yes, a JSPI build could be useful for debugging an isolated C/JavaScript interaction. For LLGo, though, Asyncify is part of the supported browser execution path: Emscripten Fibers use it to suspend Go goroutines while preserving synchronous C/C++ calls. JSPI handles the Promise boundary, but switching this runtime to JSPI also requires a different Go continuation and scheduler implementation. A debug-only JSPI build would therefore not reproduce bugs in the shipped execution path. We also tested a normal LLGo browser build with Emscripten 6.0.8 and Binaryen 132, rather than only the Binaryen fixture. Its final Wasm has 81 compilation units. Stock Binaryen produces 125 DWARF parent-containment errors and 24 overlaps; the LLGo backport passes |
|
I see, thanks for the extra context. Ok, those do sound like good reasons to move forward with this. I'll look into reviewing this PR. |
| // the high pcs, so we see the low pcs first. | ||
| BinaryLocation oldLowPC = 0, newLowPC = 0; | ||
| llvm::DWARFYAML::FormValue* lowPCValue = nullptr; | ||
| bool unresolvedZeroLowPC = false; |
There was a problem hiding this comment.
This seems nonobvious and worth a comment.
There was a problem hiding this comment.
Resolved in e394789: I removed unresolvedZeroLowPC. A zero returned by the location mapper now tombstones a non-CU DIE immediately, even when its end still maps. The new regression covers that case.
| // the DIE instead of emitting a wrapping or unrelated address range. | ||
| if (!isNonzeroTombstone(newLowPC) && | ||
| tag != llvm::dwarf::DW_TAG_compile_unit && | ||
| (newValue == 0 || newValue < newLowPC)) { |
There was a problem hiding this comment.
| (newValue == 0 || newValue < newLowPC)) { | |
| (newValue == 0 || newValue < newLowPC || newLowPC == 0)) { |
I believe newLowPC can be 0 if we didn't find a mapping for it? E.g. if the code was optimized out.
There was a problem hiding this comment.
Agreed for non-CU DIEs. A mapped newLowPC of zero now makes the DIE unavailable; a CU base of zero remains allowed. The new regression proves that a surviving high_pc cannot turn a missing start into a live range.
| if (!isNonzeroTombstone(*lowPC)) { | ||
| uint64_t start = *lowPC; | ||
| uint64_t end = highPCIsRelative ? start + *highPC : *highPC; | ||
| if (start > end) { |
There was a problem hiding this comment.
How can start > end? start is uint64_t and initially contains *lowPC (32 bits), to which we add *highPC (also 32 bits).
Perhaps better to do this math in BinaryLocations (32-bit)?
There was a problem hiding this comment.
You are right that the previous uint64 start > end check could not detect a relative 32-bit overflow. I kept wide addition to avoid wrapping, then check the result against the BinaryLocation maximum; reversed absolute endpoints are still rejected. I added the same bound check for range-list base plus offset.
| !infos[siblings[j]].ranges.empty() && | ||
| infos[siblings[i]].ranges.overlaps(infos[siblings[j]].ranges)) { | ||
| markUnavailable(infos, children, siblings[i]); | ||
| markUnavailable(infos, children, siblings[j]); |
There was a problem hiding this comment.
It seems odd to update .unavailable inside the loop here, when it is read a few lines above inside the same loop. That is, the update an affect later iterations. Is that ok for some reason? Worth a comment if so.
There was a problem hiding this comment.
Yes, the immediate update is intentional. Once two sibling scopes overlap, both become unavailable. Later pairs compare only surviving siblings, so a third scope can remain usable if it overlapped only a removed scope. I added a comment at the loop.
| if (ancestors.size() == info.depth) { | ||
| ancestors.push_back(index); | ||
| } else { | ||
| assert(ancestors.size() > info.depth); |
There was a problem hiding this comment.
How can this happen? Line 1189 ensures ancestors.size() <= info.depth (and line 1195 handles ==, so here it will actually always be <?)
There was a problem hiding this comment.
Agreed. After the pop loop, the greater-than-depth branch was unreachable. I removed it; an unexpected depth jump now marks the DIE malformed and appends safely, avoiding an out-of-bounds write in non-asserting builds.
|
|
||
| // A set of nonempty, half-open DWARF address ranges. Call normalize before | ||
| // querying containment or overlap; adding one set to another normalizes the | ||
| // result automatically. |
There was a problem hiding this comment.
Please explain what "normalization" is.
There was a problem hiding this comment.
Added the definition: normalization sorts the half-open ranges and merges overlapping or adjacent intervals into disjoint, address-ordered ranges.
|
|
||
| // In wasm32 the address size is 32 bits. | ||
| static const size_t AddressSize = 4; | ||
| static constexpr size_t RangeEntrySize = 2 * AddressSize; |
There was a problem hiding this comment.
Please add a comment. Perhaps something like "DWARF stores addresses in ranges, which are 64-bit.
There was a problem hiding this comment.
Added a more precise comment: in the wasm32 input, each legacy .debug_ranges entry contains two 4-byte addresses, giving an 8-byte entry. Range arithmetic is widened separately to detect overflow.
| // tombstone, but is context-dependent because it can also be a valid address. | ||
| static constexpr BinaryLocation AllOnesAddress = BinaryLocation(-1); | ||
| static constexpr BinaryLocation LegacyTombstoneAddress = BinaryLocation(-2); | ||
| static constexpr BinaryLocation EmptyRangeAddress = 1; |
There was a problem hiding this comment.
Please add comments for the last two.
There was a problem hiding this comment.
Added inline comments for the LLVM legacy -2 tombstone, the nonzero empty range that avoids the (0, 0) terminator, and the no-parent sentinel.
|
Independent follow-up review found one additional regression beyond the inline comments: for a wasm64 CU, the vendored DWARFYAML emitter still writes 4-byte .debug_ranges entries, while the new repair had appended a list using 8-byte-CU offsets. A two-function wasm64 fixture verified with llvm-dwarfdump reproduced invalid DW_AT_ranges after this PR but not on main. Commit 34ba0d6 keeps non-wasm32 CUs on the previous updater path and adds a regression test. Full 64-bit range-list emission remains a separate limitation of the vendored emitter. Post-review local validation: 430 C++ tests passed (1 existing platform skip), all 6 DWARF Python tests passed, the complete non-torture wasm-opt suite passed, and the wasm64 probe now verifies with the same address-size warnings as main. |
| @@ -0,0 +1,12 @@ | |||
| H4sIAAAAAAAAA31TzU7bQBCeteOQ2MUpSVugqCIHUA+WHSVCiFKUQnuB3jlVKrWdjbWwXlu7ayAn | |||
There was a problem hiding this comment.
This file can just be a wasm binary, no need for compression and encoding.



Summary
Binaryen currently updates DWARF range endpoints independently. When optimization or Asyncify removes or reorders expressions, the resulting endpoints can wrap, overlap, or escape their parent scope. This is the same failure mode reported in #6406, extended to range lists and scope topology.
The repair is conservative: representable parent unions are preserved, while ambiguous scopes fail closed. Empty replacement range lists are appended rather than mutating lists that another DIE may share.
Implementation
Range-set normalization, union, containment, and overlap are isolated in
DwarfRangesand directly unit-tested. The DWARF adapter keeps encoding-specific constants and tombstone rules inwasm-debug.cpp, including the distinction between a validlow_pc = 0and range-list terminators.The repair builds an explicit parent/child index for each compilation unit. It first propagates malformed or unavailable scopes through that tree, then processes children before parents so sibling overlap checks see final ranges and range-list parents can be extended before their own containment check. This also avoids repeated descendant scans and avoids relying on default depths for null DIE terminators.
Tombstone handling
"Nonzero tombstone" was imprecise shorthand. DWARF issue 200609.1, accepted for DWARF v6, reserves the largest representable target address (for example,
0xfffffffffor wasm32) for a non-existent entity. LLVM implements this asdwarf::computeTombstoneAddressand has a WebAssembly-specific test for a dead wasm32 subprogram.The max-minus-one value (
-2) is an LLVM legacy compatibility encoding rather than a general DWARF value. It is recognized for legacy.debug_ranges/.debug_locdata because all-ones is already the base-address-selection marker and(0, 0)terminates the list. See LLVM D81784 and DWARFDebugRangeList.cpp.Binaryen already recognizes
0,-1, and-2in tombstone-aware contexts. This change preventsupdateDIEfrom passing-1/-2through the instruction-offset mapper, where they can be rewritten to zero and make a dead DIE appear to refer to address zero.Validation
DwarfRangestestspython3 check.py wasm-opt --no-torturesuiteDW_AT_low_pc = 0xffffffffvalues remain intactllvm-dwarfdump --verifyafter roundtrip forclass_with_dwarf_noprint,fannkuch3_manyopts_dwarf,fib2_dwarf,fib2_emptylocspan_dwarf,ignore_missing_func_dwarf,inlined_to_start_dwarf, andreverse_dwarf_abbrevsllvm-dwarfdump --verifyafter--asyncify -O -gforclass_with_dwarf_noprintA wasm32 object with
DW_AT_low_pc = 0xffffffffis reported by LLVM asdead code; before this fix,wasm-opt -O -grewrites it to0x00000000.Fixes #6406.