[Lldb-commits] [PATCH] D80345: [DwarfExpression] Support entry values for indirect parameters

[Vedant Kumar via Phabricator via lldb-commits]

vsk marked 11 inline comments as done.
vsk added inline comments.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:1288
   DwarfExpr.addFragmentOffset(DIExpr);
-  if (Location.isIndirect())
+  if (Location.isIndirect() && !DIExpr->isEntryValue())
     DwarfExpr.setMemoryLocationKind();
----------------
aprantl wrote:
> Can you add a comment? it's not necessarily obvious why an indirect entry value is not a memory location. Is it because an entry value is its own kind?
This has more to do with pre-existing constraints in the dwarf emitter, more than anything else. `DwarfExpression::addReg` cannot be used with Memory location kinds, but we need that method to write out DW_OP_entry_value(DW_OP_regN).

I guess there's no such restriction for `addBReg`, but I'm not sure why we'd pick that representation (see Djordje's earlier comment).


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:790
+    // FIXME: This produces unusable descriptions when the register contains
+    // a pointer to a temporary copy of a struct passed by value.
     DIExpression *EntryExpr = DIExpression::get(
----------------
aprantl wrote:
> What does "unusable" mean? Incorrect? Invalid?
Should be "incorrect", fixed. It's "unusable" in the sense that the debugger will error out trying to evaluate the expression: it will think, "struct Foo /is/ the pointer in this register", instead of, "struct Foo is /pointed to/ by this register".


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:2412
     }
 
     const TargetRegisterInfo &TRI = *AP.MF->getSubtarget().getRegisterInfo();
----------------
aprantl wrote:
> factoring out this snippet into a template and reusing it in DwarfCompileUnit.cpp is probably not going to make this more readable, is it?
I tried doing this a few different ways :). I couldn't find a clean solution. In the end I opted for adding 'DwarfExpression::adjustLocationKind' to share just some of the logic. Lmk what you think.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfExpression.h:146
 
   /// The flags of location description being produced.
+  enum {
----------------
aprantl wrote:
> This comment is weird :-)
> 
> `Additional flags that may be combined with any location description kind.`?
Fixed, that's much better.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfExpression.h:149
+    EntryValue = 1 << 0,
+    IndirectEntryValue = 1 << 1,
+    CallSiteParamValue = 1 << 2
----------------
aprantl wrote:
> Would it make more sense call this `Indirect` (w/o EntryValue) and treat it as a separate bit, so you can still test for EntryValue independently?
Yes, that does seem better. It's more extensible this way, and anyway, most of the logic for treating direct/indirect entry values should be the same.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfExpression.h:308
   /// Lock this down to become an entry value location.
-  void setEntryValueFlag() { LocationFlags |= EntryValue; }
+  void setEntryValueFlagFromLoc(MachineLocation Loc) {
+    LocationFlags |= Loc.isIndirect() ? IndirectEntryValue : EntryValue;
----------------
aprantl wrote:
> Is the dependency on MachineLocation really necessary, or could this just take a bool / enum IsIndirect?
It aids encapsulation to not require clients to unpack the relevant bit of MachineLocation, I feel. I've changed this to accept a reference, to at least get rid of the header dependency. This seems acceptable to me, as DwarfExpression clients work with MachineLocation anyway.


================
Comment at: llvm/test/DebugInfo/MIR/AArch64/dbgcall-site-indirect-param.mir:18
+# CHECK-NEXT: [0x0000000000000000, 0x0000000000000010): DW_OP_breg0 W0+0
+# CHECK-NEXT: [0x0000000000000010, 0x000000000000001c): DW_OP_entry_value(DW_OP_reg0 W0))
+# CHECK-NEXT: DW_AT_name    ("f")
----------------
djtodoro wrote:
> I know that the final effect is the same, but should this be `DW_OP_entry_value 2 DW_OP_bregN 0` instead of `DW_OP_entry_value 1 DW_OP_regN`?
I'm not sure, the DW_OP_regN description is simply the one that has already been wired up. Is there a reason to prefer 'DW_OP_bregN 0'?


================
Comment at: llvm/test/DebugInfo/MIR/AArch64/dbgcall-site-indirect-param.mir:71
+  !24 = !DILocation(line: 5, column: 24, scope: !13)
+  !25 = !DILocation(line: 6, column: 23, scope: !13)
+  !31 = !DILocation(line: 6, column: 20, scope: !13)
----------------
djtodoro wrote:
> Nit: !25-36! could be attached to !24
That's a fair point, but I feel this is too onerous of a reduction to do by hand. As the MIR is already fairly reduced, I would prefer not to do this.


================
Comment at: llvm/test/DebugInfo/MIR/AArch64/dbgcall-site-indirect-param.mir:79
+---
+name:            bar
+alignment:       4
----------------
djtodoro wrote:
> I believe we do need all the attributes.
Fixed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80345