wasm: preserve Memory64 DWARF through Binaryen rewrites - #4
Conversation
There was a problem hiding this comment.
Review: memory64 DWARF support
Thanks for tackling 64-bit address support in the DWARF rewriter. The core approach is clean — getAddressSize, the width-aware allOnesAddress helper, threading addressSize/rangeEntrySize through the range/loc passes, and widening DWARFYAML::Loc to uint64_t are all reasonable. Tests cover both --roundtrip and --asyncify.
A few correctness issues remain, mostly around places where the 64-bit path was only partially applied. Inline comments below.
Body-level notes (no single reliable diff line):
-
DWARF64 line-table length advance vs. emitter output. In
updateDebugLines(src/wasm/wasm-debug.cpp:809-811) the new comment/code advancenewLocationby12for DWARF64 line tables, but the line-table emitter (third_party/llvm-project/DWARFEmitter.cpp:362) unconditionally writes a 4-byte length prefix (writeInteger((uint32_t)Size, ...)) with no DWARF64 escape path. If a DWARF64 line table is ever reached,newLocationover-advances by 8 bytes and corrupts subsequent.debug_lineoffsets (DW_AT_stmt_list). Since this PR targets memory64 (DWARF32 format), this is likely not hit by the added test, but the two sides should agree — either emit a real DWARF64 length or note the path as unsupported. -
BinaryLocationremainsuint32_t(src/wasm.h:2308).updateRanges/readDIEAddressRangeswiden locals touint64_tbut truncate back viaBinaryLocation(...)before callinggetNewStart/getNewEnd/isTombstone. For wasm64 this is acceptable only because DWARF code addresses are wasm binary offsets (<4 GiB); it is not sound for arbitrary 64-bit address values and reads as fragile. A brief comment stating that assumption (or widening the key type) would help future readers.
Findings without inline locations
src/wasm/wasm-debug.cpp:340: Discriminator opcode length is wrong for memory64.makeItem(DW_LNE_set_discriminator, 5)hardcodesExtLen = 5(1 subopcode + 4 data bytes), but the emitter writesOp.Datausing the CU's address width —DW_LNE_set_discriminatorshares theDW_LNE_set_addresscase inDWARFEmitter.cpp:309-312and usesAddrSizebytes. On a wasm64 module (AddrSize == 8) the actual content is1 + 8 = 9bytes whileExtLenstill declares 5, producing a malformed extended opcode. Note the siblingDW_LNE_set_addressabove was correctly parameterized to1 + addressSizein this same PR; this one was missed. ConsidermakeItem(DW_LNE_set_discriminator, 1 + addressSize)(and the stale comment// len = 1 (subopcode) + 4 (wasm32 address)— a discriminator isn't an address).
|
Addressed the remaining review findings in b5f893d:
The new DWARF64/discriminator, location-marker and out-of-range-range regressions pass; |
Memory64 modules with DWARF lose valid line and range information when Binaryen rewrites them. For example,
wasm-emscripten-finalize --dwarfturns a linked LLGo J64 module that passesllvm-dwarfdump --verifyinto one with 5,259mismatching address size ... expected 0x08 found 0x04diagnostics. This blocks the debug acceptance gate in xgo-dev/llgo#2632 even with the earlier scope-range repair in this branch.Use the module's 4- or 8-byte address width when reading and emitting DWARF line addresses, range entries, location entries, and dead-address markers. Keep the DWARF32/64 initial-length format separate from target address width. The review follow-up also preserves DWARF64 line-table prefixes, encodes line discriminators as ULEB128, guards
.debug_locemission without compile units, and rejects out-of-range 64-bit addresses before mapping them through 32-bit Wasm binary offsets.Validation:
.debug_loc, and out-of-range.debug_rangescases.llvm-dwarfdump --verifypasses on the checked-in Memory64 fixture after roundtrip and Asyncify, and on the synthetic DWARF64 line table.wasm-optandwasm-emscripten-finalizepass final-artifactllvm-dwarfdump --verifyand run under Node at J64 O0/O2. J32 O0 and W32 O0 passed before the review follow-up.test_cluster_fuzzfailure because the installed Android D8 is selected as a JavaScript engine. This PR's DWARF tests pass.Scope: this PR preserves debug information on linked Memory64 modules through finalize, roundtrip and Asyncify. The separate
--memory64-loweringpath still needs DIE-reference relocation when changing compilation-unit address width.Base:
llgo.