[PATCH] D56587: Introduce DW_OP_LLVM_convert

[Paul Robinson via Phabricator via llvm-commits]

probinson added inline comments.


================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:946
 
+  for (const auto &P : CUMap) {
+    auto &CU = *P.second;
----------------
aprantl wrote:
> markus wrote:
> > If there were multiple Dwarf CUs in the same LLVM Module this would not work right. We need to emit base types for each DwarfCompileUnit but only those types that are used by DwarfExpressions in that unit. So appears to make sense to put the `MarkusNodes` inside the CU.
> Would creating a separate CU just for our basic types help in any way?
A separate CU for basic types would not be usable by DW_OP_convert? because it uses CU-relative offsets to find them.


================
Comment at: lib/CodeGen/AsmPrinter/DwarfExpression.cpp:29
+  "generate-typed-dwarf5-expr", cl::Hidden,
+  cl::desc("Generate typed DWARF5 expressions regardless of DWARF version targeted."),
+  cl::init(false));
----------------
aprantl wrote:
> Why is this needed? Shouldn't we key off the debugger tuning flag?
The debugger-tuning design is that we use it only in the DwarfDebug ctor to set other control flags, which can all be influenced independently.  Tuning and its associated flags are not per-CU, although DWARF version is.


================
Comment at: lib/CodeGen/AsmPrinter/DwarfExpression.cpp:399
+      if (DwarfVersion >= 5 || GenerateTypedDwarfExpr) {
+        emitOp(dwarf::DW_OP_convert);
+        // XXX: Simply emit the index into the raw byte stream as ULEB128,
----------------
Pre-v5 needs to emit the GNU op, not the standard op.


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

https://reviews.llvm.org/D56587