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

[Stephen Tozer via Phabricator via llvm-commits]

StephenTozer added inline comments.


================
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)
----------------
jmorse wrote:
> 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).
> This over-writes Offset from getFrameIndexReference if the DBG_VALUE is indirect -- which I don't believe is a behaviour in the old code.

That's because the old code is somewhat misleading; `Offset` is declared at a single point above as positive iff `MI` is an indirect debug value; otherwise it is 0, and gets incremented if the location operand is not a register. These two conditions are mutually exclusive, so ultimately it's just being optionally assigned one of two values; I've just made that more explicit here, but the behaviour should be unchanged.

> 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).

That assertion does exist in some places, but I'm not sure if it's everywhere. I could be wrong, but I believe the assertions that the offset is zero are all at points prior to the stack layout being finalized; at the time we emit DWARF, I //think// we may still have non-zero offsets for some DBG_VALUEs.


================
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()) {
----------------
jmorse wrote:
> 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?
This is really just a "dumb" carry-over of the logic above; when there is a non-trivial expression for a non-variadic debug value with an integer location in this function, it looks as though we just add the unsigned bytes and call it a day. I think the intent is that the signedness is ultimately interpreted according to the variable type? But in all existing cases I think the approach is flawed; unless you have an empty DIExpression, there's no guarantee that the signedness of the variable matches the signedness of the location operand.


================
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();
----------------
jmorse wrote:
> What about if the next opcode is stack_value, do we need to mask in that scenario?
I think so? To be honest, it looks to me like the only time we can use a subregister and don't apply a mask is when describing either a Register location (where we cannot use subreg masking, we use DW_OP_piece instead), or when describing a simple memory location, i.e. a single DW_OP_breg with 0 offset.

Although the latter case seems like a valid argument for "we don't //always// need to mask the subregister", I suspect that it's actually the case that we just never use a subregister for those locations. The full size of a register for a given architecture will also generally be the size of a memory address, so it may just be assumed that we don't need to check for subreg masking in that case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83495