[PATCH] D83495: [DebugInfo] Add DWARF emission for DBG_VALUE_LIST

[Scott Linder via Phabricator via llvm-commits]

scott.linder added inline comments.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:952-973
+    case MachineOperand::MO_Register:
+    case MachineOperand::MO_FrameIndex: {
+      Register Reg;
+      if (Op.isReg())
+        Reg = Op.getReg();
+      else {
+        const TargetFrameLowering *TFI =
----------------
If you are changing this anyway, I'd vote to just split up these two cases, they don't actually seem related and the existing code reads worse to me.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DebugHandlerBase.cpp:38
   DbgVariableLocation Location;
-  if (!Instruction.isDebugValue())
+  // Variables calculated from multiple locations can't be represented here.
+  if (!Instruction.isNonListDebugValue())
----------------
Would it make sense to still try applying the heuristic if `size(Instruction.debug_values()) == 1 && Instruction.getDebugOperand(0).isReg() && DIExpr->expr_op_begin()->getOp == dwarf::DW_OP_LLVM_arg`?

I think this only really makes a difference when we eliminate the old version, but I assume we will still want this thing to work for the cases it can?


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DebugLocEntry.h:121
+           is_contained(Locs, dwarf::DW_OP_stack_value));
+    for (DbgValueLocEntry &Entry : ValueLocEntries) {
+      assert(!Entry.isConstantFP() && !Entry.isConstantInt() &&
----------------
I think you can wrap this constructor body in `#ifndef NDEBUG`


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DebugLocEntry.h:135
+    if (!IsVariadic) {
+      assert(ValueLocEntries.size() == 1);
+    } else {
----------------
Same here, the whole constructor should be `#ifndef NDEBUG`


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:2469
+    } else if (Entry.isConstantFP()) {
+	  if (AP.getDwarfVersion() >= 4 && AP.getDwarfDebug()->tuneForGDB()) {
+		  DwarfExpr.addConstantFP(Entry.getConstantFP()->getValueAPF(), AP);
----------------
Was changing this condition intentional? If so can it be in a separate patch?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83495/new/

https://reviews.llvm.org/D83495