[PATCH] D58042: [LiveDebugValues] Emit parameter's entry value

[Djordje Todorovic via Phabricator via llvm-commits]

djtodoro marked 4 inline comments as done.
djtodoro added inline comments.


================
Comment at: include/llvm/IR/DebugInfoMetadata.h:2529
+  enum { NoDeref = false, WithDeref = true, WithStackValue = true,
+         WithEntryValue = true };
 
----------------
aprantl wrote:
> The more flags we add these bool parameters get less and less safe, since the order isn't typesafe. We should probably think about also using an `unsigned Flag` parameter that is `WithDeref | WithEntryValue`.
I agree with this.


================
Comment at: include/llvm/IR/DebugInfoMetadata.h:2600
+    return getNumElements() > 0 &&
+           getElement(0) == dwarf::DW_OP_GNU_entry_value;
+  }
----------------
aprantl wrote:
> In LLVM IR I'd prefer using the DWARF 5 DW_OP_entry_value (Or a new LLVM one if we change the semantics). We can still emit the GNU variant, if requested.
> 
> According to the DWARF spec an entry value can appear anywhere in the dwarf expression, right? If we only accept one as the first element, we also need a verifier that enforces this.
>In LLVM IR I'd prefer using the DWARF 5 DW_OP_entry_value (Or a new LLVM one if we change the semantics). We can still emit the GNU variant, if requested.
We will make it.

>According to the DWARF spec an entry value can appear anywhere in the dwarf expression, right?
 Yes, but for now we support just basic entry values expressions. 

>If we only accept one as the first element, we also need a verifier that enforces this.
Sure.



================
Comment at: lib/CodeGen/AsmPrinter/DwarfExpression.cpp:333
+
+  emitOp(dwarf::DW_OP_GNU_entry_value);
+  emitUnsigned(Op->getArg(0));
----------------
aprantl wrote:
> This should be DW_OP_entry_value if DWARF 5 is requested (or a perhaps GDB tuning)
Sure.


================
Comment at: lib/CodeGen/LiveDebugValues.cpp:450
 
+void LiveDebugValues::emitEntryValues(
+    MachineInstr &MI, OpenRangesSet &OpenRanges, VarLocMap &VarLocIDs,
----------------
aprantl wrote:
> Would it be possible to split this part into a separate patch?
IIUC, splitting introduction and production of entry_values makes sense.


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

https://reviews.llvm.org/D58042