[PATCH] D20147: [WIP] DebugInfo: New metadata representation for global variables.

[Adrian Prantl via llvm-commits]

aprantl added a comment.

Thanks for putting so much effort into this, this is starting to look really great!


================
Comment at: include/llvm/IR/DIBuilder.h:503
@@ +502,3 @@
+      return DIExpression::get(
+          VMContext, {dwarf::DW_OP_constu, Val, dwarf::DW_OP_stack_value});
+    }
----------------
If we haven't already, we will need to add explicit support for DW_OP_stack_value to DwarfExpression to ensure that we don't emit when generating DWARF 2. (And yes, without it the expression will be ambiguous).

================
Comment at: include/llvm/IR/DebugInfoMetadata.h:1827
@@ +1826,3 @@
+/// This is (almost) a DWARF expression that modifies the location of a
+/// variable or (or the location of a single piece of a variable).
+///
----------------
... or //is// the (constant) variable.

================
Comment at: include/llvm/IR/DebugInfoMetadata.h:1830
@@ +1829,3 @@
+/// FIXME: Instead of DW_OP_plus taking an argument, this should use DW_OP_const
+/// and have DW_OP_plus consume the topmost elements on the stack.
+///
----------------
Oh yes :-)

================
Comment at: include/llvm/IR/DebugInfoMetadata.h:1867
@@ +1866,3 @@
+
+  /// \brief Return whether this is a piece of an aggregate variable.
+  bool isBitPiece() const;
----------------
All the \brief's are redundant unless you want to include more than the first sentence in the brief description. Makes the code a bit more readable IMHO.
(I know this is just copied, but we might as well fix it up now).

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:2607
@@ +2606,3 @@
+      GlobalVariable *Attach = nullptr;
+      if (auto *CMD = dyn_cast_or_null<ConstantAsMetadata>(Expr)) {
+        if (auto *GV = dyn_cast<GlobalVariable>(CMD->getValue())) {
----------------
I'm not sure if Expr is guaranteed to be materialized at this point. You may need to store a list of DIExpressions and fix them up later.
Duncan?

================
Comment at: lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:126
@@ +125,3 @@
+
+  // For compatibility with DWARF 4,
+  // DW_AT_location(DW_OP_constu, X, DW_OP_stack_value) becomes
----------------
Ah, great! This should probably say "DWARF 3 and earlier",


http://reviews.llvm.org/D20147