[WebAssembly] Fixed byval args missing DWARF DW_AT_LOCATION

[aardappel]

Summary

A struct in C passed by value did not get debug information. Such values are currently
lowered to a Wasm local even in -O0 (not to an alloca like on other archs), which becomes
a Target Index operand (TI_LOCAL). The DWARF writing code was not emitting locations
in for TI's specifically if the location is a single range (not a list).

In addition, the ExplicitLocals pass which removes the ARGUMENT pseudo instructions did
not update the associated DBG_VALUEs, and couldn't even find these values since the code
assumed such instructions are adjacent, which is not the case here.

Also fixed asm printing of TIs needed by a test.

[aardappel]

@dblaikie Added you for review since this modifies common DWARF code, would love to hear if that is problematic or not. It would seem not, since we're the only users of these TI's it seems, and the new code is generic enough that it could work for future targets using TIs?

[dblaikie]

In D94140#2480989, @aardappel wrote:

@dblaikie Added you for review since this modifies common DWARF code, would love to hear if that is problematic or not. It would seem not, since we're the only users of these TI's it seems, and the new code is generic enough that it could work for future targets using TIs?

Yeah, since you folks are the ones that are using this feature for now (I guess/can you confirm that the TI things were added specifically for WebAssembly?) it's pretty much up to you. For more nuanced review of debug info locations, @aprantl is probably a more useful reference point - the actual optimized debug info variable location tracking, etc, isn't something I've stayed particularly close to.

[aprantl]

Mechanically this LGTM.

llvm/test/CodeGen/WebAssembly/dbgvalue.ll

4

it would be nicer if we piped the output to llvm-dwarfdump and CHECK that output rather than checking for a comment in the assembler output.

[dschuff]

In D94140#2481074, @dblaikie wrote:

In D94140#2480989, @aardappel wrote:

@dblaikie Added you for review since this modifies common DWARF code, would love to hear if that is problematic or not. It would seem not, since we're the only users of these TI's it seems, and the new code is generic enough that it could work for future targets using TIs?

Yeah, since you folks are the ones that are using this feature for now (I guess/can you confirm that the TI things were added specifically for WebAssembly?) it's pretty much up to you. For more nuanced review of debug info locations, @aprantl is probably a more useful reference point - the actual optimized debug info variable location tracking, etc, isn't something I've stayed particularly close to.

I can confirm that the TI things were added for wasm. We tried to make them so that if another target used TargetIndex operands the same way, then it would just work (although of course TI operands are intended to be used however a target wants, so there's no guarantee).

[aardappel]

llvm/test/CodeGen/WebAssembly/dbgvalue.ll

4

Agreed dwarfdump is the more exact way to do this. Here I am fixing an existing test though, and we have plenty of dwarfdump based tests already.

lld/test/wasm/debuginfo.test

 CHECK-NEXT:                DW_AT_high_pc
 CHECK-NEXT:                DW_AT_frame_base
 CHECK-NEXT:                DW_AT_name ("test")
 CHECK-NEXT:                DW_AT_decl_file  ("/Users/yury/llvmwasm{{(/|\\)}}hi.c")
 CHECK-NEXT:                DW_AT_decl_line  (3)
 CHECK-NEXT:                DW_AT_prototyped (true)
 
 CHECK:     DW_TAG_formal_parameter
+CHECK-NEXT:                  DW_AT_location (DW_OP_WASM_location 0x0 0x0, DW_OP_stack_value)
 CHECK-NEXT:                  DW_AT_name ("t")
 CHECK-NEXT:                  DW_AT_decl_file  ("/Users/yury/llvmwasm{{(/|\\)}}hi.c")
 CHECK-NEXT:                  DW_AT_decl_line  (3)
 
 CHECK:   DW_TAG_subprogram
 CHECK-NEXT:                DW_AT_low_pc
 CHECK-NEXT:                DW_AT_high_pc
 CHECK-NEXT:                DW_AT_frame_base
 CHECK-NEXT:                DW_AT_name ("_start")
 CHECK-NEXT:                DW_AT_decl_file  ("/Users/yury/llvmwasm{{(/|\\)}}hi.c")
 CHECK-NEXT:                DW_AT_decl_line  (7)
 
 CHECK:   DW_TAG_base_type
 CHECK-NEXT:                DW_AT_name ("int")
 CHECK-NEXT:                DW_AT_encoding (DW_ATE_signed)
 CHECK-NEXT:                DW_AT_byte_size  (0x04)
 
 CHECK: DW_TAG_compile_unit
 CHECK-NEXT:              DW_AT_producer ("clang version 7.0.0 (trunk {{.*}})")
 CHECK-NEXT:              DW_AT_language (DW_LANG_C99)
 CHECK-NEXT:              DW_AT_name ("hi_foo.c")
 
 CHECK:   DW_TAG_variable
 CHECK-NEXT:                DW_AT_name ("y")
-CHECK-NEXT:                DW_AT_type (0x000000a7 "int[2]")
+CHECK-NEXT:                DW_AT_type (0x000000ac "int[2]")
 CHECK-NEXT:                DW_AT_external (true)
 CHECK-NEXT:                DW_AT_decl_file  ("{{.*}}hi_foo.c")
 CHECK-NEXT:                DW_AT_decl_line  (1)
 CHECK:                     DW_AT_location (DW_OP_addr 0x400)
 
 CHECK:   DW_TAG_array_type
 
 CHECK:     DW_TAG_subrange_type
 
 CHECK:   DW_TAG_base_type
 CHECK-NEXT:                DW_AT_name ("int")
 CHECK-NEXT:                DW_AT_encoding (DW_ATE_signed)
 CHECK-NEXT:                DW_AT_byte_size  (0x04)
 
 CHECK:   DW_TAG_base_type
 CHECK-NEXT:                DW_AT_name ("__ARRAY_SIZE_TYPE__")
 CHECK-NEXT:                DW_AT_byte_size  (0x08)
 CHECK-NEXT:                DW_AT_encoding (DW_ATE_unsigned)
 
 CHECK:   DW_TAG_variable
 CHECK-NEXT:                DW_AT_name ("z")
-CHECK-NEXT:                DW_AT_type (0x000000a7 "int[2]")
+CHECK-NEXT:                DW_AT_type (0x000000ac "int[2]")
 CHECK-NEXT:                DW_AT_external (true)
 CHECK-NEXT:                DW_AT_decl_file  ("{{.*}}hi_foo.c")
 CHECK-NEXT:                DW_AT_decl_line  (8)
 CHECK-NEXT:                DW_AT_location (DW_OP_addr 0xffffffff)
 
 CHECK:   DW_TAG_subprogram
 CHECK-NEXT:                DW_AT_low_pc
 CHECK-NEXT:                DW_AT_high_pc
 CHECK-NEXT:                DW_AT_frame_base
 CHECK-NEXT:                DW_AT_name ("foo")
 CHECK-NEXT:                DW_AT_decl_file  ("{{.*}}hi_foo.c")
 CHECK-NEXT:                DW_AT_decl_line  (3)
 
 CHECK:     DW_TAG_formal_parameter
+CHECK-NEXT:                  DW_AT_location (DW_OP_WASM_location 0x0 0x0, DW_OP_stack_value)
 CHECK-NEXT:                  DW_AT_name ("p")
 CHECK-NEXT:                  DW_AT_decl_file  ("{{.*}}hi_foo.c")
 CHECK-NEXT:                  DW_AT_decl_line  (3)
 

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

if (MI->getDebugOperand(0).isFPImm()) {
     }
   } else if (MI->getDebugOperand(0).isImm()) {
     OS << MI->getDebugOperand(0).getImm();
   } else if (MI->getDebugOperand(0).isCImm()) {
     MI->getDebugOperand(0).getCImm()->getValue().print(OS, false /*isSigned*/);
   } else if (MI->getDebugOperand(0).isTargetIndex()) {
     auto Op = MI->getDebugOperand(0);
     OS << "!target-index(" << Op.getIndex() << "," << Op.getOffset() << ")";
+    // NOTE: Want this comment at start of line, don't emit with AddComment.
+    AP.OutStreamer->emitRawComment(OS.str());
     return true;
   } else {
     Register Reg;
     if (MI->getDebugOperand(0).isReg()) {
       Reg = MI->getDebugOperand(0).getReg();
     } else {
       assert(MI->getDebugOperand(0).isFI() && "Unknown operand type");
       const TargetFrameLowering *TFI = AP.MF->getSubtarget().getFrameLowering();

llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp

else if (DVal->isInt()) {
                   dwarf::DW_FORM_data1, *DwarfExpr.TagOffset);
 
       } else
         addConstantValue(*VariableDie, DVal->getInt(), DV.getType());
     } else if (DVal->isConstantFP()) {
       addConstantFPValue(*VariableDie, DVal->getConstantFP());
     } else if (DVal->isConstantInt()) {
       addConstantValue(*VariableDie, DVal->getConstantInt(), DV.getType());
+    } else if (DVal->isTargetIndexLocation()) {
+      DIELoc *Loc = new (DIEValueAllocator) DIELoc;
+      DIEDwarfExpression DwarfExpr(*Asm, *this, *Loc);
+      const DIBasicType *BT = dyn_cast<DIBasicType>(
+          static_cast<const Metadata *>(DV.getVariable()->getType()));
+      DwarfDebug::emitDebugLocValue(*Asm, BT, *DVal, DwarfExpr);
+      addBlock(*VariableDie, dwarf::DW_AT_location, DwarfExpr.finalize());
     }
     return VariableDie;
   }
 
   // .. else use frame index.
   if (!DV.hasFrameIndexExprs())
     return VariableDie;
 

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

if (Value.isInt()) {
     const TargetRegisterInfo &TRI = *AP.MF->getSubtarget().getRegisterInfo();
     if (!DwarfExpr.addMachineRegExpression(TRI, Cursor, Location.getReg()))
       return;
     return DwarfExpr.addExpression(std::move(Cursor));
   } else if (Value.isTargetIndexLocation()) {
     TargetIndexLocation Loc = Value.getTargetIndexLocation();
     // TODO TargetIndexLocation is a target-independent. Currently only the WebAssembly-specific
     // encoding is supported.
+    assert(AP.TM.getTargetTriple().isWasm());
     DwarfExpr.addWasmLocation(Loc.Index, static_cast<uint64_t>(Loc.Offset));
       DwarfExpr.addExpression(std::move(ExprCursor));
       return;
   } else if (Value.isConstantFP()) {
     if (AP.getDwarfVersion() >= 4 && !AP.getDwarfDebug()->tuneForSCE()) {
       DwarfExpr.addConstantFP(Value.getConstantFP()->getValueAPF(), AP);
       return;
     } else if (Value.getConstantFP()

llvm/lib/Target/WebAssembly/WebAssemblyDebugValueManager.cpp

 #include "WebAssembly.h"
 #include "WebAssemblyMachineFunctionInfo.h"
 #include "llvm/CodeGen/MachineInstr.h"
 
 using namespace llvm;
 
 WebAssemblyDebugValueManager::WebAssemblyDebugValueManager(
     MachineInstr *Instr) {
-  Instr->collectDebugValues(DbgValues);
+  // This code differs from MachineInstr::collectDebugValues in that it scans
+  // the whole BB, not just contiguous DBG_VALUEs.
+  if (!Instr->getOperand(0).isReg())
+    return;
+
+  MachineBasicBlock::iterator DI = *Instr;
+  ++DI;
+  for (MachineBasicBlock::iterator DE = Instr->getParent()->end(); DI != DE;
+       ++DI) {
+    if (DI->isDebugValue() &&
+        DI->getDebugOperandForReg(Instr->getOperand(0).getReg()))
+      DbgValues.push_back(&*DI);
+  }
 }
 
 void WebAssemblyDebugValueManager::move(MachineInstr *Insert) {
   MachineBasicBlock *MBB = Insert->getParent();
   for (MachineInstr *DBI : reverse(DbgValues))
     MBB->splice(Insert, DBI->getParent(), DBI);
 }
 

llvm/lib/Target/WebAssembly/WebAssemblyExplicitLocals.cpp

for (MachineBasicBlock::iterator I = MF.begin()->begin(),
     MachineInstr &MI = *I++;
     if (!WebAssembly::isArgument(MI.getOpcode()))
       break;
     Register Reg = MI.getOperand(0).getReg();
     assert(!MFI.isVRegStackified(Reg));
     auto Local = static_cast<unsigned>(MI.getOperand(1).getImm());
     Reg2Local[Reg] = Local;
     checkFrameBase(MFI, Local, Reg);
+
+    // Update debug value to point to the local before removing.
+    WebAssemblyDebugValueManager(&MI).replaceWithLocal(Local);
+
     MI.eraseFromParent();
     Changed = true;
   }
 
   // Start assigning local numbers after the last parameter.
   unsigned CurLocal = static_cast<unsigned>(MFI.getParams().size());
 
   // Precompute the set of registers that are unused, so that we can insert

llvm/test/CodeGen/WebAssembly/dbgvalue.ll

 ; RUN: llc < %s -O0 -verify-machineinstrs | FileCheck %s
 
 ; CHECK: %bb.0
-; CHECK: #DEBUG_VALUE: usage:self <- %4
+; CHECK: #DEBUG_VALUE: usage:self <- !target-index(0,0)
 ; CHECK: %bb.1
 ; CHECK: DW_TAG_variable
 source_filename = "test/CodeGen/WebAssembly/dbgvalue.ll"
 target datalayout = "e-m:e-p:32:32-i64:64-n32:64-S128"
 target triple = "wasm32-unknown-unknown"
 
 @key = external local_unnamed_addr global [15 x i8], align 1
 @.str = external unnamed_addr constant [33 x i8], align 1

llvm/test/MC/WebAssembly/debug-byval-struct.ll

+; RUN: llc -filetype=obj %s -o - | llvm-dwarfdump -  | FileCheck %s
+
+; Test that byval args get correct DWARF debug locations.
+; These end up in the callee as Wasm locals, which is atypical.
+
+; From C code:
+
+; typedef union { int x; float y; } u;
+; typedef struct { int x; float y; } s;
+; int some_func(int x, u some_union, s some_struct, int a[3]) {
+;     return x + some_union.x + some_struct.x + a[0];
+; }
+
+
+; ModuleID = 't.c'
+source_filename = "t.c"
+target datalayout = "e-m:e-p:32:32-i64:64-n32:64-S128"
+target triple = "wasm32-unknown-unknown"
+
+%union.u = type { i32 }
+%struct.s = type { i32, float }
+
+; Function Attrs: noinline nounwind optnone
+define hidden i32 @some_func(i32 %x, %union.u* byval(%union.u) align 4 %some_union, %struct.s* byval(%struct.s) align 4 %some_struct, i32* %a) #0 !dbg !7 {
+entry:
+  %x.addr = alloca i32, align 4
+  %a.addr = alloca i32*, align 4
+  store i32 %x, i32* %x.addr, align 4
+  call void @llvm.dbg.declare(metadata i32* %x.addr, metadata !23, metadata !DIExpression()), !dbg !24
+  call void @llvm.dbg.declare(metadata %union.u* %some_union, metadata !25, metadata !DIExpression()), !dbg !26
+  call void @llvm.dbg.declare(metadata %struct.s* %some_struct, metadata !27, metadata !DIExpression()), !dbg !28
+  store i32* %a, i32** %a.addr, align 4
+  call void @llvm.dbg.declare(metadata i32** %a.addr, metadata !29, metadata !DIExpression()), !dbg !30
+  %0 = load i32, i32* %x.addr, align 4, !dbg !31
+  %x1 = bitcast %union.u* %some_union to i32*, !dbg !32
+  %1 = load i32, i32* %x1, align 4, !dbg !32
+  %add = add nsw i32 %0, %1, !dbg !33
+  %x2 = getelementptr inbounds %struct.s, %struct.s* %some_struct, i32 0, i32 0, !dbg !34
+  %2 = load i32, i32* %x2, align 4, !dbg !34
+  %add3 = add nsw i32 %add, %2, !dbg !35
+  %3 = load i32*, i32** %a.addr, align 4, !dbg !36
+  %arrayidx = getelementptr inbounds i32, i32* %3, i32 0, !dbg !36
+  %4 = load i32, i32* %arrayidx, align 4, !dbg !36
+  %add4 = add nsw i32 %add3, %4, !dbg !37
+  ret i32 %add4, !dbg !38
+}
+
+; Function Attrs: nofree nosync nounwind readnone speculatable willreturn
+declare void @llvm.dbg.declare(metadata, metadata, metadata) #1
+
+attributes #0 = { noinline nounwind optnone "disable-tail-calls"="false" "frame-pointer"="none" "less-precise-fpmad"="false" "min-legal-vector-width"="0" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="generic" "unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #1 = { nofree nosync nounwind readnone speculatable willreturn }
+
+!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 12.0.0 (https://github.com/llvm/llvm-project.git eccc734a69c0c012ae3160887b65a535b35ead3e)", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, nameTableKind: None)
+!1 = !DIFile(filename: "t.c", directory: "C:\\src\\llvm")
+!2 = !{}
+!3 = !{i32 7, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{i32 1, !"wchar_size", i32 4}
+!6 = !{!"clang version 12.0.0 (https://github.com/llvm/llvm-project.git eccc734a69c0c012ae3160887b65a535b35ead3e)"}
+!7 = distinct !DISubprogram(name: "some_func", scope: !1, file: !1, line: 11, type: !8, scopeLine: 11, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !0, retainedNodes: !2)
+!8 = !DISubroutineType(types: !9)
+!9 = !{!10, !10, !11, !17, !22}
+!10 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!11 = !DIDerivedType(tag: DW_TAG_typedef, name: "u", file: !1, line: 4, baseType: !12)
+!12 = distinct !DICompositeType(tag: DW_TAG_union_type, file: !1, line: 1, size: 32, elements: !13)
+!13 = !{!14, !15}
+!14 = !DIDerivedType(tag: DW_TAG_member, name: "x", scope: !12, file: !1, line: 2, baseType: !10, size: 32)
+!15 = !DIDerivedType(tag: DW_TAG_member, name: "y", scope: !12, file: !1, line: 3, baseType: !16, size: 32)
+!16 = !DIBasicType(name: "float", size: 32, encoding: DW_ATE_float)
+!17 = !DIDerivedType(tag: DW_TAG_typedef, name: "s", file: !1, line: 9, baseType: !18)
+!18 = distinct !DICompositeType(tag: DW_TAG_structure_type, file: !1, line: 6, size: 64, elements: !19)
+!19 = !{!20, !21}
+!20 = !DIDerivedType(tag: DW_TAG_member, name: "x", scope: !18, file: !1, line: 7, baseType: !10, size: 32)
+!21 = !DIDerivedType(tag: DW_TAG_member, name: "y", scope: !18, file: !1, line: 8, baseType: !16, size: 32, offset: 32)
+!22 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !10, size: 32)
+!23 = !DILocalVariable(name: "x", arg: 1, scope: !7, file: !1, line: 11, type: !10)
+!24 = !DILocation(line: 11, column: 19, scope: !7)
+!25 = !DILocalVariable(name: "some_union", arg: 2, scope: !7, file: !1, line: 11, type: !11)
+!26 = !DILocation(line: 11, column: 24, scope: !7)
+!27 = !DILocalVariable(name: "some_struct", arg: 3, scope: !7, file: !1, line: 11, type: !17)
+!28 = !DILocation(line: 11, column: 38, scope: !7)
+!29 = !DILocalVariable(name: "a", arg: 4, scope: !7, file: !1, line: 11, type: !22)
+!30 = !DILocation(line: 11, column: 55, scope: !7)
+!31 = !DILocation(line: 12, column: 12, scope: !7)
+!32 = !DILocation(line: 12, column: 27, scope: !7)
+!33 = !DILocation(line: 12, column: 14, scope: !7)
+!34 = !DILocation(line: 12, column: 43, scope: !7)
+!35 = !DILocation(line: 12, column: 29, scope: !7)
+!36 = !DILocation(line: 12, column: 47, scope: !7)
+!37 = !DILocation(line: 12, column: 45, scope: !7)
+!38 = !DILocation(line: 12, column: 5, scope: !7)
+
+
+
+
+; CHECK-LABEL:  DW_TAG_formal_parameter
+; CHECK-NEXT:     DW_AT_location        (DW_OP_fbreg +12)
+; CHECK-NEXT:     DW_AT_name    ("x")
+
+; CHECK-LABEL:  DW_TAG_formal_parameter
+; CHECK-NEXT:     DW_AT_location        (DW_OP_WASM_location 0x0 0x1, DW_OP_stack_value)
+; CHECK-NEXT:     DW_AT_name    ("some_union")
+
+; CHECK-LABEL:  DW_TAG_formal_parameter
+; CHECK-NEXT:     DW_AT_location        (DW_OP_WASM_location 0x0 0x2, DW_OP_stack_value)
+; CHECK-NEXT:     DW_AT_name    ("some_struct")
+
+; CHECK-LABEL:  DW_TAG_formal_parameter
+; CHECK-NEXT:     DW_AT_location        (DW_OP_fbreg +8)
+; CHECK-NEXT:     DW_AT_name    ("a")
+