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

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added a comment.

This tentatively LsGTM, modulo some comments. Most of the discussion has been about the context-sensitivity of how the expression stack has evolved, which is awkward but pre-existing. Just to check with @scott.linder, you're alright with the patch going in, with improving expression handling a problem for another day?



================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:987-988
+      // The second operand is only an offset if it's an immediate.
+      if (MI->isIndirectDebugValue())
+        Offset = StackOffset::getFixed(MI->getDebugOffset().getImm());
+      if (Offset)
----------------
This over-writes Offset from getFrameIndexReference if the DBG_VALUE is indirect -- which I don't believe is a behaviour in the old code.

AFAIUI the "debug offset" should nowadays only be used as a flag indicating indirectness, not the actual numerical offset. There's at least one assertion out the that the offset is zero. (Obviously none of this is ideal).


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DebugLocEntry.h:37
 
 /// A single location or constant.
+class DbgValueLocEntry {
----------------
It'd be good to elaborate on this -- a single machine location within a variable location description, one of possibly many?


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:758-759
+      } else if (Entry.isInt()) {
+        // If there is an expression, emit raw unsigned bytes.
+        DwarfExpr.addUnsignedConstant(Entry.getInt());
+      } else if (Entry.isConstantFP()) {
----------------
To check my understanding: for non-variadic variables emission of integer operands might be signed or unsigned, depending on the type of the variable, we can see that earlier in this function and down in DwarfDebug::emitDebugLocValue. For a variadic expression, however, it's expected that the value gets "compiled in" unsigned, hence we add as unsigned here, yes?


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp:359-362
+  auto NextOp = ExprCursor.peek();
+  if (SubRegisterSizeInBits && NextOp &&
+      (NextOp->getOp() != dwarf::DW_OP_LLVM_fragment))
+    maskSubRegister();
----------------
What about if the next opcode is stack_value, do we need to mask in that scenario?


================
Comment at: llvm/test/DebugInfo/X86/dbg_value_list_clobbers.mir:1-2
+# RUN: llc %s --start-after=livedebugvalues -filetype=obj -o - \
+# RUN:     | llvm-dwarfdump - -name locala -o -
+#
----------------
Does not call FileCheck


================
Comment at: llvm/test/DebugInfo/X86/dbg_value_list_emission.mir:5
+#
+# Test that we produce good DWARF from DBG_VALUE_LIST instructions.
+# Comments and test directives inline.
----------------
Mega-nit: "good" suggests there's a subjective difference, could I suggest "correct"


================
Comment at: llvm/test/DebugInfo/X86/dbg_value_list_emission.mir:94
+    RETQ debug-location !15
+...
----------------
Could I request a test that a DBG_VALUE_LIST with $noreg somewhere in it does not lead to a location-list entry -- I've been bitten by $noregs not terminating things in the past.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83495