[PATCH] D36907: [codeview] support more DW_OPs for more complete debug info

[Reid Kleckner via Phabricator via llvm-commits]

rnk added inline comments.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/VariableLocation.cpp:33-50
+    case dwarf::DW_OP_constu: {
+      int Value = Op->getArg(0);
+      ++Op;
+      if (Op != DIExpr->expr_op_end()) {
+        switch (Op->getOp()) {
+        case dwarf::DW_OP_minus:
+          Offset -= Value;
----------------
I was thinking about how to improve DIExpression over the weekend, and one of the ideas I came up with is that we should "canonicalize" them on construction, and we should add a high-level opcode like DW_OP_LLVM_fragment that expresses DW_OP_deref plus a signed offset. That would eliminate the need for a ton of DW_OP_const/minus/plus parsing and centralize the logic in the DIExpression opcode canonicalization phase. CodeGen and mid-level passes would be able to reason about chains of loads with offsets, or they would give up if the expression is too complicated.

This is an idle thought for a future patch, though...


================
Comment at: llvm/lib/CodeGen/AsmPrinter/VariableLocation.h:1
+//===- llvm/lib/CodeGen/AsmPrinter/VariableLocation.h -----------*- C++ -*-===//
+//
----------------
IMO we should call this DbgVariableLocation. I'd recommend moving it to DebugHandlerBase.h for now. That's the current home for common functionality between dwarf + CV emission.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/VariableLocation.h:13-14
+
+#include <llvm/ADT/Optional.h>
+#include <llvm/IR/DebugInfoMetadata.h>
+
----------------
These are not system headers, these should be quoted.


================
Comment at: llvm/test/DebugInfo/COFF/pieces.ll:182
 ; ASM:        .asciz  "o"
-; FIXME: We should have a .cv_def_range for 'o', but we don't yet.
-; ASM-NOT:    .cv_def_range
----------------
Cool. :)


https://reviews.llvm.org/D36907