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

[Vedant Kumar via Phabricator via llvm-commits]

vsk added inline comments.


================
Comment at: lib/CodeGen/AsmPrinter/DwarfCompileUnit.h:244
+  /// non-zero for non-tail calls (in the case of non-gdb tuning, since for
+  /// GDB + DWARF 5 tuninig we still generate PC info for tail calls) or be the
   /// function-local offset to PC value after the call instruction.
----------------
nit, tuning


================
Comment at: lib/CodeGen/AsmPrinter/DwarfCompileUnit.h:254
+  /// Note: The order of parameters does not metter, since debuggers recognize
+  ///       call site parameters by its DW_AT_location attribute.
+  void constructCallSiteParmEntryDIEs(DIE &CallSiteDIE,
----------------
nit, matter, and its -> the


================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:580
+  for (auto ArgReg : CallFwdRegsInfo->second)
+    ArgsRegsForProcess.insert(ArgReg.Reg);
+
----------------
djtodoro wrote:
> 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!!
I see. I had in mind:

```
bool InsertedReg = ForwardedRegWorklist.insert(ArgReg.Reg).second;
assert(InsertedReg && "Single register used to forward two arguments?");
```

If this is already checked elsewhere, there's no need for a duplicate assert.


================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:609
+          if (TRI->regsOverlap(FwdReg, MO.getReg()))
+            Defs.push_back(FwdReg);
+      }
----------------
After FwdReg from MO is pushed into Defs, shouldn't this loop should break? IIUC no other registers should be equal-to/alias FwdReg.


================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:631
+    // forwarding registers or we finished the interpretation of all parameters.
+    if (I->isCall())
+      return;
----------------
Should the basic block scan stop at branches in addition to calls to reduce compile time (MI::isCall() || MI::isBranch())?


================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:632
+    if (I->isCall())
+      return;
+
----------------
If the scan reaches a call or a branch, shouldn't it break instead of returning? That way, the ShouldTryEmitEntryVals case can be finished. Perhaps there should be a test?


================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:683
+        if (EntryValReg != RegsForEntryValues.end())
+          FwdReg = EntryValReg->second;
+
----------------
nit, extra indentation here?


================
Comment at: lib/CodeGen/AsmPrinter/DwarfExpression.cpp:479
+        emitUnsigned(Op->getArg(0));
+        emitSigned(Op->getArg(1));
+      } else {
----------------
I don't understand the lowering of DW_OP_regx within addMachineRegExpression. Why should the lowering be different within/without parameter values? It seems like the DIExpression itself should be formed with DW_OP_bregx, if/when needed.


================
Comment at: lib/CodeGen/TargetInstrInfo.cpp:1146
+      return None;
+    unsigned CastedOffset = static_cast<unsigned>(Offset);
+    if (Offset > 0)
----------------
I do not think that casting the offset to unsigned is safe due to loss of precision. Also, note that the expression generated for the Offset=0 case is inefficient. Is there some way to reuse the code in DIExpression::appendOffset for this?


================
Comment at: lib/IR/DebugInfoMetadata.cpp:854
+    if (Op >= dwarf::DW_OP_reg0 && Op <= dwarf::DW_OP_reg31)
+      continue;
+
----------------
djtodoro wrote:
> aprantl wrote:
> > I think this must also check that we are inside an entry value.
> > DW_OP_reg is used only in situations when we want to describe an expression over two registers (example: Loading a parameter's value with LEA instruction. Please see X86InstrInfo::describeLoadedValue()). We can discuss about potential alternatives for that.
> > 
> > DW_OP_entry_values uses advantages from @entry values (read from DW_TAG_call_site_parameter info) when representing normal values. In debugger we would have a new feature for printing of new kind of value (i.e. print val at entry). GDB already has that support.
> > Without the part for producing DW_OP_entry_values (in LiveDebugValues) we would see just those val at entry and there won't be cases of improving normal values of a parameters.
> > For now, we generate DW_OP_entry_values with a simple register location, but the register is read from DBG_VALUE operand (We don't need DW_OP_reg in DIExpression for that). DIExpression has only entry value DWARF operand and the size of following expression (for now it is always 1, because the rest of expression will be only register from DBG_VALUE).
> > 
> > The conclusion is that there are two separate parts here:
> > 
> > Introduction debug info for call site parameters (DW_TAG_call_site_parameter) that is used for printing @entry value within debugger
> > Introduction of new kind of DWARF operand (DW_OP_entry_value) that can use benefits of @entry values when representing normal values of function parameters
> 
> According to this comment this new `DW_OP_reg` operand will only be used in the situation when an instruction, that loads a value into parameter forwarding register, is dealing with multiple registers (please see test case `test/DebugInfo/MIR/X86/dbgcall-site-lea-interpretation.mir`). The `DIExpression` that contains the `DW_OP_reg` will be used only for printing the `DW_AT_GNU_call_site_value` and if we remove this part the only thing that will happen is that we will have less number of call site parameters generated (because without the `DW_AT_GNU_call_site_value` a  `DW_TAG_GNU_call_site_parameter` is useless and if don't manage to describe loaded value we are avoiding the production of call site parameters debug info).
> 
>  
@aprantl would you mind re-stating your concern? The dwarf standard says that DW_OP_reg<n> describes an object in a register. That seems like it would be valid outside of a DW_OP_entry_value block. Is it not?


================
Comment at: lib/Target/X86/X86InstrInfo.cpp:7452
+      Elements.push_back(dwarf::DW_OP_plus);
+    }
+
----------------
We should be able to factor out the logic in DIExpression::{append,prepend}Offset to simplify this.


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

https://reviews.llvm.org/D60716