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

Mon Jul 29 07:37:16 PDT 2019

djtodoro marked 36 inline comments as done.
djtodoro added a comment.

@vsk Thanks a lot for such nice review!

================
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
----------------
vsk wrote:
> Could you clarify what RHS mean, here?
Sure.

================
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:
> 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?
>As the descriptions of the parameters here is fairly complex, could you split the discussion of each parameter into a separate paragraph?
Sure.

>Also, is the parenthetical "(in the case of non-gdb tuning)" in the PCOffset description redundant?
The comment is updated in order to avoid the ambiguity. GDB uses PC info even for tail calls.

>Does CallReg=0 for direct calls?
Yes.

================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:580
+  for (auto ArgReg : CallFwdRegsInfo->second)
+    ArgsRegsForProcess.insert(ArgReg.Reg);
+
----------------
vsk wrote:
> 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.
This only adds all the regs into the working list by iterating through the `CallSiteInfo`.

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

Thanks!!

================
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();
----------------
vsk wrote:
> Could you add a comment explaining what RegsForEntryValues is before explaining ShouldTryEmitEntryVals?
Sure.

================
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 {
----------------
vsk wrote:
> 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'.
Thanks for this!

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

================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:599
+        for (auto FwdReg : ArgsRegsForProcess)
+          if (TRI->regsOverlap(FwdReg, MO.getReg()))
+            return FwdReg;
----------------
vsk wrote:
> 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?
Hmm...Yes, we have not consider such situation. Thanks!

================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:607
+
+  auto finishCallSiteParam = [&](DbgValueLoc &DbgLocVal, unsigned &Reg) {
+    unsigned FwdReg = Reg;
----------------
vsk wrote:
> Why are DbgLocVal and Reg passed in by reference?
We can pass them as values, and let the compiler to optimize it for us.

================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:647
+          } else if (ShouldTryEmitEntryVals) {
+            ArgsRegsForProcess.insert(RegLoc);
+            RegsForEntryValues[RegLoc] = Reg;
----------------
vsk wrote:
> I wonder whether two passes through the block are needed, as ArgsRegsForProcess may not be complete when it is referenced in `shouldInterpret`.
The working list is complete at the moment. But, we are inserting new registers in order to try generating call site parameters values as entry values (after the loop ends). We are doing that only for the entry MBB (the simplest case), but since the GCC generates the call site values expression as entry values even for the MBBs other then the entry one, we have left the `TODO` comment for that.

================
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) {
----------------
vsk wrote:
> Not sure what '1' is for in '{dwarf::DW_OP_entry_value, 1}', perhaps a comment would help?
Sure.

================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.h:261
+  unsigned Register;
+  DbgValueLoc Value;
+public:
----------------
vsk wrote:
> 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.
> ```
Thanks!

================
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
+#
----------------
vsk wrote:
> Consider using 'FileCheck -implicit-check-not=DW_TAG_GNU_call_site_parameter %s' to catch all of them.
We just wanted to emphasizes that we did not describe a call site parameter. Since all of this is controlled by an option, the implicit-check-not will require not to use the option on the same test case. But we put an error alerting that the `CallSiteInfo` (the `MF` attribute `callSites`) is provided but not used, so it will be difficult to use the implicit-check-not here.

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

https://reviews.llvm.org/D60716