[WebAssembly] Nullify DBG_VALUE_LISTs in DebugValueManager

[aheejin]

Summary

WebAssemblyDebugValueManager class currently does not handle
DBG_VALUE_LIST instructions correctly for two reasons, which are
explained in https://bugs.llvm.org/show_bug.cgi?id=50361.

This effectively nullifies DBG_VALUE_LISTs in
WebAssemblyDebugValueManager so that the info will appear as "optimized
out" in debuggers but still be at least correct in the meantime.

[aheejin]

Comment

[dschuff]

If I'm understanding this correctly, we only create a WebAssemblyDebugValueManager object when we actually stackify a reg and move the def, correct?
I guess that still ends up invalidating a lot of debug info, since we always run explicit locals.
In the code you've seen, does the debug info all or mostly consist of DBG_VALUE_LISTs now, or is it still mostly DBG_VALUE, or a mix?
I'm just trying to get a sense for how much of a degradation in debug info quality we have now.
(Not that we shouldn't do this; clearly this is better than the loop-explosion we have now. I'm just trying to get a sense for how urgent the "real" fix is).

[jmorse]

Hi,

In the code you've seen, does the debug info all or mostly consist of DBG_VALUE_LISTs now, or is it still mostly DBG_VALUE, or a mix?
I'm just trying to get a sense for how much of a degradation in debug info quality we have now.

Until recently, variable locations that could not be described became "DBG_VALUE $noreg" instructions. With some recent patches from @StephenTozer , some of those locations can now be described with DBG_VALUE_LIST instructions. If I understand this patch correctly, for WebAssembly those DBG_VALUE_LIST instructions transition back to being "DBG_VALUE $noreg". There should be no regression in variable location coverage: instead, WebAssembly does not yet benefit from the additional locations now being recovered.

[aheejin]

Improve comments and CHECK line

[dschuff]

In D102589#2763870, @jmorse wrote:

Until recently, variable locations that could not be described became "DBG_VALUE $noreg" instructions. With some recent patches from @StephenTozer , some of those locations can now be described with DBG_VALUE_LIST instructions. If I understand this patch correctly, for WebAssembly those DBG_VALUE_LIST instructions transition back to being "DBG_VALUE $noreg". There should be no regression in variable location coverage: instead, WebAssembly does not yet benefit from the additional locations now being recovered.

Great, thanks for the clarification.

[aheejin]

@jmorse Thanks for your help!

llvm/lib/Target/WebAssembly/WebAssemblyDebugValueManager.cpp

 #include "WebAssembly.h"
 #include "WebAssemblyMachineFunctionInfo.h"
 #include "llvm/CodeGen/MachineInstr.h"
 
 using namespace llvm;
 
 WebAssemblyDebugValueManager::WebAssemblyDebugValueManager(
     MachineInstr *Instr) {
+  const auto *MF = Instr->getParent()->getParent();
+  const auto *TII = MF->getSubtarget<WebAssemblySubtarget>().getInstrInfo();
+
   // This code differs from MachineInstr::collectDebugValues in that it scans
   // the whole BB, not just contiguous DBG_VALUEs.
   if (!Instr->getOperand(0).isReg())
     return;
   CurrentReg = Instr->getOperand(0).getReg();
 
+  SmallVector<MachineInstr *, 2> DbgValueLists;
   MachineBasicBlock::iterator DI = *Instr;
   ++DI;
   for (MachineBasicBlock::iterator DE = Instr->getParent()->end(); DI != DE;
        ++DI) {
     if (DI->isDebugValue() &&
         DI->hasDebugOperandForReg(Instr->getOperand(0).getReg()))
-      DbgValues.push_back(&*DI);
+      DI->getOpcode() == TargetOpcode::DBG_VALUE
+          ? DbgValues.push_back(&*DI)
+          : DbgValueLists.push_back(&*DI);
+  }
+
+  // This class currently cannot handle DBG_VALUE_LISTs correctly. So this
+  // converts DBG_VALUE_LISTs to "DBG_VALUE $noreg", which will appear as
+  // "optimized out". This can invalidate existing iterators pointing to
+  // instructions within this BB from the caller.
+  // See https://bugs.llvm.org/show_bug.cgi?id=50361
+  // TODO Correctly handle DBG_VALUE_LISTs
+  for (auto *DVL : DbgValueLists) {
+    BuildMI(*DVL->getParent(), DVL, DVL->getDebugLoc(),
+            TII->get(TargetOpcode::DBG_VALUE), false, Register(),
+            DVL->getOperand(0).getMetadata(), DVL->getOperand(1).getMetadata());
+    DVL->eraseFromParent();
   }
 }
 
 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 = MBB.begin(), E = MBB.end(); I != E;) {
       }
 
       // Insert local.sets for any defs that aren't stackified yet.
       for (auto &Def : MI.defs()) {
         Register OldReg = Def.getReg();
         if (!MFI.isVRegStackified(OldReg)) {
           const TargetRegisterClass *RC = MRI.getRegClass(OldReg);
           Register NewReg = MRI.createVirtualRegister(RC);
-          auto InsertPt = std::next(MI.getIterator());
           if (UseEmpty[Register::virtReg2Index(OldReg)]) {
             unsigned Opc = getDropOpcode(RC);
-            MachineInstr *Drop =
-                BuildMI(MBB, InsertPt, MI.getDebugLoc(), TII->get(Opc))
+            MachineInstr *Drop = BuildMI(MBB, std::next(MI.getIterator()),
+                                         MI.getDebugLoc(), TII->get(Opc))
                                      .addReg(NewReg);
             // After the drop instruction, this reg operand will not be used
             Drop->getOperand(0).setIsKill();
             if (MFI.isFrameBaseVirtual() && OldReg == MFI.getFrameBaseVreg())
               MFI.clearFrameBaseVreg();
           } else {
             unsigned LocalId = getLocalId(Reg2Local, MFI, CurLocal, OldReg);
             unsigned Opc = getLocalSetOpcode(RC);
 
             WebAssemblyDebugValueManager(&MI).replaceWithLocal(LocalId);
 
-            BuildMI(MBB, InsertPt, MI.getDebugLoc(), TII->get(Opc))
+            BuildMI(MBB, std::next(MI.getIterator()), MI.getDebugLoc(),
+                    TII->get(Opc))
                 .addImm(LocalId)
                 .addReg(NewReg);
           }
           // This register operand of the original instruction is now being used
           // by the inserted drop or local.set instruction, so make it not dead
           // yet.
           Def.setReg(NewReg);
           Def.setIsDead(false);

llvm/test/CodeGen/WebAssembly/reg-stackify-dbg.mir

+# RUN: llc -mtriple=wasm32-unknown-unknown -run-pass wasm-reg-stackify %s -o - | FileCheck %s
+
+--- |
+  target datalayout = "e-m:e-p:32:32-i64:64-n32:64-S128"
+  target triple = "wasm32-unknown-unknown"
+
+  define void @dbg_value_list_test() {
+    ret void
+  }
+
+  !llvm.module.flags = !{!0}
+  !llvm.dbg.cu = !{!1}
+
+  !0 = !{i32 2, !"Debug Info Version", i32 3}
+  !1 = distinct !DICompileUnit(language: DW_LANG_C99, file: !2, producer: "clang version 3.9.0 (trunk 266005) (llvm/trunk 266105)", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !3)
+  !2 = !DIFile(filename: "test.c", directory: "/")
+  !3 = !{}
+  !4 = distinct !DISubprogram(name: "dbg_value_list_test", scope: !2, file: !2, line: 10, type: !5, isLocal: false, isDefinition: true, scopeLine: 11, flags: DIFlagPrototyped, isOptimized: true, unit: !1, retainedNodes: !3)
+  !5 = !DISubroutineType(types: !3)
+  !6 = !DILocalVariable(name: "var", scope: !4, file: !2, line: 15, type: !7)
+  !7 = !DIBasicType(name: "int", size: 32, align: 32, encoding: DW_ATE_signed)
+  !8 = !DILocation(line: 15, column: 6, scope: !4)
+...
+
+# WebAssemblyDebugValueManager currently does not handle DBG_VALUE_LIST
+# instructions correctly and instead effectively nullifying them by turning them
+# into "DBG_VALUE $noreg". See https://bugs.llvm.org/show_bug.cgi?id=50361.
+# (Otherwise DBG_VALUE_LIST instructions can be exponentially and possibly
+# incorrectly copied.)
+# This tests if DBG_VALUE_LIST is nullified as intended.
+
+# CHECK-LABEL: name: dbg_value_list_test
+name: dbg_value_list_test
+liveins:
+  - { reg: '$arguments' }
+body: |
+  bb.0:
+    ; CHECK: DBG_VALUE $noreg, $noreg
+    %0:i32 = ARGUMENT_i32 0, implicit $arguments
+    %1:i32 = ARGUMENT_i32 1, implicit $arguments
+    %2:i32 = ARGUMENT_i32 2, implicit $arguments
+    %3:i32 = LOAD_I32_A32 2, 0, %0:i32, implicit-def dead $arguments
+    %4:i32 = LT_U_I32 %3:i32, %1:i32, implicit-def dead $arguments
+    %5:i32 = GE_U_I32 %4:i32, %2:i32, implicit-def dead $arguments
+    %6:i32 = OR_I32 %5:i32, %4:i32, implicit-def dead $arguments
+    ; This should become "DBG_VALUE $noreg" and should not be copied when %4 is
+    ; tee'd
+    ; CHECK-NOT: DBG_VALUE_LIST
+    DBG_VALUE_LIST !6, !DIExpression(), %4:i32, debug-location !8
+    RETURN %6:i32, implicit-def dead $arguments
+...