[WebAssembly] Move .debug_line section address of dead function outside section range

[yurydelendik]

Summary

Currently we are pointing all debug information that refer removed function code
to the beginning of the code section (offset = 0). A debugger may want to
resolve code offset to the debug information, which will collide with offsets
of the live functions.

Moving offsets of dead functions outside code section range.

[ruiu]

I believe for other architectures, when the linker emits debug info for a dead function, it treats relocations pointing a dead section as if it were pointing to address zero. Is there any reason to do that differnetly for wasm?

[yurydelendik]

I believe for other architectures, when the linker emits debug info for a dead function, it treats relocations pointing a dead section as if it were pointing to address zero. Is there any reason to do that differnetly for wasm?

The .debug_line section encodes instructions offset in relative form, so the next address advances by delta, and that introduces non-zero addresses for dead code. It is hard to tell if an address that is close to 0 points to dead section or just to the beginning of the code (unless you have relocation sections handy). It will make consumption of the debug information way easier if dead pointers will not point to the same locations where live data can reside.

[yurydelendik]

Fix comment

[sbc100]

I'm still not sure I understand why writing zero here is not a good idea.

wasm/InputFiles.cpp

152

Could/Should this be "->isLive()"?

[yurydelendik]

In D49446#1167210, @sbc100 wrote:

I'm still not sure I understand why writing zero here is not a good idea.

It will be probably easy to demonstrate on the example. When running the test case (test/wasm/debug-removed-fn.ll) without the patch, the .debug_line data contains the following info

0x0000000000000005      1      0      1   0             0  is_stmt
0x0000000000000008      2      3      1   0             0  is_stmt prologue_end
0x0000000000000009      2      3      1   0             0  is_stmt end_sequence
0x0000000000000000      5      0      1   0             0  is_stmt
0x0000000000000003      6      3      1   0             0  is_stmt prologue_end
0x0000000000000004      6      3      1   0             0  is_stmt end_sequence

With patch:

0x0000000000000005      1      0      1   0             0  is_stmt
0x0000000000000008      2      3      1   0             0  is_stmt prologue_end
0x0000000000000009      2      3      1   0             0  is_stmt end_sequence
0x00000000fdead000      5      0      1   0             0  is_stmt
0x00000000fdead003      6      3      1   0             0  is_stmt prologue_end
0x00000000fdead004      6      3      1   0             0  is_stmt end_sequence

Notice that the "bar" function debug data "claims" that the function can be found at addresses 0-4 (without the patch). Then same true for .debug_info.

If the both functions are exported, .debug_line looks like:

0x0000000000000005      1      0      1   0             0  is_stmt
0x0000000000000008      2      3      1   0             0  is_stmt prologue_end
0x0000000000000009      2      3      1   0             0  is_stmt end_sequence
0x000000000000000a      5      0      1   0             0  is_stmt
0x000000000000000d      6      3      1   0             0  is_stmt prologue_end
0x000000000000000e      6      3      1   0             0  is_stmt end_sequence

Does this explain the problem?

[yurydelendik]

wasm/InputFiles.cpp

152

InputFunction/InputChunk does not provide such method. Do you want to add this attribute to the InpurtFunction?

[dblaikie]

In D49446#1167267, @yurydelendik wrote:

In D49446#1167210, @sbc100 wrote:

I'm still not sure I understand why writing zero here is not a good idea.

It will be probably easy to demonstrate on the example. When running the test case (test/wasm/debug-removed-fn.ll) without the patch, the .debug_line data contains the following info

0x0000000000000005      1      0      1   0             0  is_stmt
0x0000000000000008      2      3      1   0             0  is_stmt prologue_end
0x0000000000000009      2      3      1   0             0  is_stmt end_sequence
0x0000000000000000      5      0      1   0             0  is_stmt
0x0000000000000003      6      3      1   0             0  is_stmt prologue_end
0x0000000000000004      6      3      1   0             0  is_stmt end_sequence

With patch:

0x0000000000000005      1      0      1   0             0  is_stmt
0x0000000000000008      2      3      1   0             0  is_stmt prologue_end
0x0000000000000009      2      3      1   0             0  is_stmt end_sequence
0x00000000fdead000      5      0      1   0             0  is_stmt
0x00000000fdead003      6      3      1   0             0  is_stmt prologue_end
0x00000000fdead004      6      3      1   0             0  is_stmt end_sequence

Notice that the "bar" function debug data "claims" that the function can be found at addresses 0-4 (without the patch). Then same true for .debug_info.

If the both functions are exported, .debug_line looks like:

0x0000000000000005      1      0      1   0             0  is_stmt
0x0000000000000008      2      3      1   0             0  is_stmt prologue_end
0x0000000000000009      2      3      1   0             0  is_stmt end_sequence
0x000000000000000a      5      0      1   0             0  is_stmt
0x000000000000000d      6      3      1   0             0  is_stmt prologue_end
0x000000000000000e      6      3      1   0             0  is_stmt end_sequence

Does this explain the problem?

Ish - but existing debuggers/linkers already tolerate this sort of debug info - so it'd be ideal/better if WAsm did too. Any address derived from zero would be considered invalid/uninteresting. (similarly for the debug_info itself - you'd find similarly confusing data, where functions start at zero and are X bytes long - so technically a bunch of non-zero bytes are covered by that range, but currently it seems existing debuggers/linkers are OK with this contract too).

[yurydelendik]

In D49446#1172887, @dblaikie wrote:

Ish - but existing debuggers/linkers already tolerate this sort of debug info - so it'd be ideal/better if WAsm did too. Any address derived from zero would be considered invalid/uninteresting. (similarly for the debug_info itself - you'd find similarly confusing data, where functions start at zero and are X bytes long - so technically a bunch of non-zero bytes are covered by that range, but currently it seems existing debuggers/linkers are OK with this contract too).

Currently, the wasm function (dead or alive) never starts from 0. The Sym->Function->getFunctionCodeOffset() returns size of the function size field that is placed before function body. If we want debug info to point strict 0, we can change the DEAD_FUNCTION_OFFSET to be 0 -- the rest of the patch will stay pretty much the same. I just took one step further and moved that completely on of code range -- with other platforms that's happening automatically since PC is almost never close to 0.

FWIW, the algorithm for the detection of dead function debug entries looks like https://github.com/kripken/emscripten/pull/6884/files#diff-d7e06c82407a72c0476db85b2cf5d3deR157 and requires linker to produce compact LEB values for functions size field length.

[dblaikie]

In D49446#1173389, @yurydelendik wrote:

In D49446#1172887, @dblaikie wrote:

Ish - but existing debuggers/linkers already tolerate this sort of debug info - so it'd be ideal/better if WAsm did too. Any address derived from zero would be considered invalid/uninteresting. (similarly for the debug_info itself - you'd find similarly confusing data, where functions start at zero and are X bytes long - so technically a bunch of non-zero bytes are covered by that range, but currently it seems existing debuggers/linkers are OK with this contract too).

Currently, the wasm function (dead or alive) never starts from 0. The Sym->Function->getFunctionCodeOffset() returns size of the function size field that is placed before function body. If we want debug info to point strict 0, we can change the DEAD_FUNCTION_OFFSET to be 0 -- the rest of the patch will stay pretty much the same. I just took one step further and moved that completely on of code range -- with other platforms that's happening automatically since PC is almost never close to 0.

FWIW, the algorithm for the detection of dead function debug entries looks like https://github.com/kripken/emscripten/pull/6884/files#diff-d7e06c82407a72c0476db85b2cf5d3deR157 and requires linker to produce compact LEB values for functions size field length.

Guess there's no chance of reusing the existing ELF logic for this? (I assume the ELF linker and WAsm linker are too different for that)

If it's not going to fall out naturally as a consequence of a single shared implementation, then I'm not too fussed with what value you give dead code - I'd still lean towards zero, but I'll leave it to Rui.

[sbc100]

wasm/InputFiles.cpp

152

But Sym does have this, no? See line 134 above? Maybe I'm missing something.

[sbc100]

wasm/InputFiles.cpp

152

You should be able use the same pattern as R_WEBASSEMBLY_MEMORY_ADDR_LEB above I think.

Doing that, should allow you to revert InputFiles.cpp and InputChunks.h making this patch really trivial. We could then consider the DEADFUNC thing as a followup?

[yurydelendik]

Use Sym->isLive; start dead function offset at 0.

test/wasm/debug-removed-fn.ll

+; RUN: llc -filetype=obj < %s -o %t.o
+; RUN: wasm-ld %t.o --no-entry --export=foo -o %t.wasm
+; RUN: llvm-dwarfdump -debug-line %t.wasm | FileCheck %s
+
+; CHECK: Address
+; CHECK: 0x0000000000000005
+; CHECK: 0x0000000000000000
+
+; ModuleID = 't.bc'
+target datalayout = "e-m:e-p:32:32-i64:64-n32:64-S128"
+target triple = "wasm32-unknown-unknown-wasm"
+
+; Function Attrs: noinline nounwind optnone
+define hidden i32 @foo() #0 !dbg !7 {
+entry:
+  ret i32 42, !dbg !11
+}
+
+; Function Attrs: noinline nounwind optnone
+define hidden i32 @bar() #0 !dbg !12 {
+entry:
+  ret i32 6, !dbg !13
+}
+
+attributes #0 = { noinline nounwind optnone "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="false" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="generic" "unsafe-fp-math"="false" "use-soft-float"="false" }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5}
+!llvm.ident = !{!6}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 7.0.0 (trunk 337293)", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !2)
+!1 = !DIFile(filename: "t.c", directory: "/d/y")
+!2 = !{}
+!3 = !{i32 2, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{i32 1, !"wchar_size", i32 4}
+!6 = !{!"clang version 7.0.0 (trunk 337293)"}
+!7 = distinct !DISubprogram(name: "foo", scope: !1, file: !1, line: 1, type: !8, isLocal: false, isDefinition: true, scopeLine: 1, isOptimized: false, unit: !0, retainedNodes: !2)
+!8 = !DISubroutineType(types: !9)
+!9 = !{!10}
+!10 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!11 = !DILocation(line: 2, column: 3, scope: !7)
+!12 = distinct !DISubprogram(name: "bar", scope: !1, file: !1, line: 5, type: !8, isLocal: false, isDefinition: true, scopeLine: 5, isOptimized: false, unit: !0, retainedNodes: !2)
+!13 = !DILocation(line: 6, column: 3, scope: !12)

wasm/InputFiles.cpp

uint32_t ObjFile::calcNewValue(const WasmRelocation &Reloc) const {
   case R_WEBASSEMBLY_TYPE_INDEX_LEB:
     return TypeMap[Reloc.Index];
   case R_WEBASSEMBLY_FUNCTION_INDEX_LEB:
     return getFunctionSymbol(Reloc.Index)->getFunctionIndex();
   case R_WEBASSEMBLY_GLOBAL_INDEX_LEB:
     return getGlobalSymbol(Reloc.Index)->getGlobalIndex();
   case R_WEBASSEMBLY_FUNCTION_OFFSET_I32:
     if (auto *Sym = dyn_cast<DefinedFunction>(getFunctionSymbol(Reloc.Index))) {
+      if (Sym->isLive())
         return Sym->Function->OutputOffset +
                Sym->Function->getFunctionCodeOffset() + Reloc.Addend;
     }
     return 0;
   case R_WEBASSEMBLY_SECTION_OFFSET_I32:
     return getSectionSymbol(Reloc.Index)->Section->OutputOffset + Reloc.Addend;
   default:
     llvm_unreachable("unknown relocation type");
   }
 }