[PATCH] D60716: [DwarfDebug] Dump call site debug info into DWARF

Thu Jul 25 18:55:50 PDT 2019

vsk added a comment.

This has been sitting on my review queue for too long, I'm really sorry for the delay here. I've still only gotten 2/3 of the way through this patch but will make time to finish the rest soon. Thanks for working on this!

I have a correctness concern in 'shouldInterpret'. My other concerns are about documentation/naming.

================
Comment at: include/llvm/CodeGen/TargetInstrInfo.h:1684

+  /// Produce RHS description of loading instruction \p MI by giving
+  /// expression \p Expr over \p Op. If we do not manage to find such
----------------
Could you clarify what RHS mean, here?

================
Comment at: lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:970
+    DIE &CallSiteDIE, SmallVector<DbgCallSiteParam, 4> &Params) {
+  for (auto &Param : Params) {
+    unsigned Register = Param.getRegister();
----------------
const auto& Param?

================
Comment at: lib/CodeGen/AsmPrinter/DwarfCompileUnit.h:230

+  dwarf::Tag getDwarf5OrGNUCallSiteTag(dwarf::Tag Tag);
+  dwarf::Attribute getDwarf5OrGNUCallSiteAttr(dwarf::Attribute Attr);
----------------
Please document these methods and, if it's not too much trouble, mark them const.

================
Comment at: lib/CodeGen/AsmPrinter/DwarfCompileUnit.h:240
+  /// after the call instruction. \p CallReg is a register location
+  /// for an indirect call.
+  DIE &constructCallSiteEntryDIE(DIE &ScopeDIE, const DISubprogram *CalleeSP,
----------------
As the descriptions of the parameters here is fairly complex, could you split the discussion of each parameter into a separate paragraph?

Also, is the parenthetical "(in the case of non-gdb tuning)" in the PCOffset description redundant?

================
Comment at: lib/CodeGen/AsmPrinter/DwarfCompileUnit.h:240
+  /// after the call instruction. \p CallReg is a register location
+  /// for an indirect call.
+  DIE &constructCallSiteEntryDIE(DIE &ScopeDIE, const DISubprogram *CalleeSP,
----------------
vsk wrote:
> As the descriptions of the parameters here is fairly complex, could you split the discussion of each parameter into a separate paragraph?
> 
> Also, is the parenthetical "(in the case of non-gdb tuning)" in the PCOffset description redundant?
Does CallReg=0 for direct calls?

================
Comment at: lib/CodeGen/AsmPrinter/DwarfCompileUnit.h:244
+                                 const MCExpr *PCOffset, unsigned CallReg);
+  void constructCallSiteParmEntryDIEs(DIE &CallSiteDIE,
+                                      SmallVector<DbgCallSiteParam, 4> &Params);
----------------
Please document constructCallSiteParmEntryDIEs. Specifically, it would help to have a \ref to the function that creates the param array, and to have a note about whether or not the order of the parameters matters.

================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:580
+  for (auto ArgReg : CallFwdRegsInfo->second)
+    ArgsRegsForProcess.insert(ArgReg.Reg);
+
----------------
Can we assert that this insertion into ArgsRegsForProcess occurs? I'm assuming that it's a bug if more than one argument is forward in the same register.

Super nit-pick, but, I don't think having 'ForProcess' in the name is very clear. Something like 'ForwardedRegWorklist' would be more in keeping with the naming in the rest of the compiler.

================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:585
+  // if a call is within entry MBB.
+  DenseMap<unsigned, unsigned> RegsForEntryValues;
+  bool ShouldTryEmitEntryVals = MBB->getIterator() == MF->begin();
----------------
Could you add a comment explaining what RegsForEntryValues is before explaining ShouldTryEmitEntryVals?

================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:589
+  // Return true if it is an instruction over a parameter's forwarding
+  // register that clobbers it.
+  auto shouldInterpret = [&](const MachineInstr &MI) -> unsigned {
----------------
The comment for 'shouldInterpret' should be updated to reflect the fact that it returns a register, not a bool. Also, consider using a more specific name, like 'getForwardingRegDefinedByMI'.

================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:597
+      if (MO.isReg() && MO.isDef() && MO.getReg() &&
+          TRI->isPhysicalRegister(MO.getReg())) {
+        for (auto FwdReg : ArgsRegsForProcess)
----------------
Is the first call to MO.getReg() just checking for undef? I wonder why TRI->isPhysicalRegister() does not make it redundant.

================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:599
+        for (auto FwdReg : ArgsRegsForProcess)
+          if (TRI->regsOverlap(FwdReg, MO.getReg()))
+            return FwdReg;
----------------
Can more than one Def be attached to an MI? 'MachineInstr::getNumDefs()' suggests it is possible. If so, then it seems like there is a bug here when more than one forwarding reg is clobbered by MI?

================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:607
+
+  auto finishCallSiteParam = [&](DbgValueLoc &DbgLocVal, unsigned &Reg) {
+    unsigned FwdReg = Reg;
----------------
Why are DbgLocVal and Reg passed in by reference?

================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:610
+    if (ShouldTryEmitEntryVals && RegsForEntryValues.count(Reg))
+      FwdReg = RegsForEntryValues[Reg];
+    DbgCallSiteParam CSParm(FwdReg, DbgLocVal);
----------------
Consider using RegsForEntryValues.find() to avoid doing two lookups.

================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:617
+  // Search for a loading value in forwaring registers.
+  while (I != MBB->rend()) {
+    // If the next instruction is a call we can not
----------------
Consider using a for loop here. It will guarantee iterator increment and make it easier to continue early.

================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:627
+
+    if (unsigned Reg = shouldInterpret(*I)) {
+      ArgsRegsForProcess.erase(Reg);
----------------
To simplify this, I suggest continuing early when Reg is undef.

================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:630
+      const MachineOperand *Op;
+      DIExpression *Expr;
+      if (auto ParamValue = TII->describeLoadedValue(*I)) {
----------------
These declarations for `Op` and `Expr` don't seem necessary.

================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:644
+          if (TRI->isCalleeSavedPhysReg(RegLoc, *MF) || IsSPorFP) {
+            DbgValueLoc DbgLocVal(Expr, MachineLocation(RegLoc, IsSPorFP));
+            finishCallSiteParam(DbgLocVal, Reg);
----------------
Please use /*IsIndirect=*/IsSPorFP when calling the MachineLocation constructor.

================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:647
+          } else if (ShouldTryEmitEntryVals) {
+            ArgsRegsForProcess.insert(RegLoc);
+            RegsForEntryValues[RegLoc] = Reg;
----------------
I wonder whether two passes through the block are needed, as ArgsRegsForProcess may not be complete when it is referenced in `shouldInterpret`.

================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:660
+    DIExpression *EntryExpr = DIExpression::get(MF->getFunction().getContext(),
+                                                {dwarf::DW_OP_entry_value, 1});
+    for (auto RegEntry : ArgsRegsForProcess) {
----------------
Not sure what '1' is for in '{dwarf::DW_OP_entry_value, 1}', perhaps a comment would help?

================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:662
+    for (auto RegEntry : ArgsRegsForProcess) {
+      unsigned FwdReg = RegsForEntryValues.count(RegEntry)
+                            ? RegsForEntryValues[RegEntry]
----------------
RegsForEntryValues.find to avoid two lookups?

================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.h:261
+  unsigned Register;
+  DbgValueLoc Value;
+public:
----------------
Consider adding some detail, like:
```
unsigned Register; ///< Parameter register at the callee entry point.
DbgValueLoc; ///< Corresponding location for the parameter value at the call site.
```

================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.h:264
+  DbgCallSiteParam(unsigned Reg, DbgValueLoc Val)
+      : Register(Reg), Value(Val) {}
+
----------------
Assert that Reg isn't undef here?

================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.h:267
+  unsigned getRegister() { return Register; }
+  DbgValueLoc getValue() { return Value; }
+};
----------------
Please mark the DbgCallSiteParam methods const.

================
Comment at: test/DebugInfo/MIR/X86/dbgcall-site-interpretation.mir:29
+# CHECK-NEXT:     DW_AT_GNU_call_site_value   (DW_OP_lit4)
+# CHECK-NOT: DW_TAG_GNU_call_site_parameter
+#
----------------
Consider using 'FileCheck -implicit-check-not=DW_TAG_GNU_call_site_parameter %s' to catch all of them.

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

https://reviews.llvm.org/D60716