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

[Adrian Prantl via llvm-commits]

aprantl added inline comments.

================
Comment at: include/llvm/IR/DIBuilder.h:444
@@ -442,3 +443,3 @@
                                            bool isLocalToUnit,
-                                           llvm::Constant *Val,
+                                           DIExpression *Expr,
                                            MDNode *Decl = nullptr);
----------------
Maybe this should default to nullptr?

================
Comment at: include/llvm/IR/GlobalVariable.h:32
@@ -32,1 +31,3 @@
+class DIGlobalVariable;
+class Module;
 template <typename ValueSubClass> class SymbolTableListTraits;
----------------
Why Module?

================
Comment at: lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:179
@@ -188,20 +178,3 @@
     if (DD->useAllLinkageNames())
       addLinkageName(*VariableDIE, GV->getLinkageName());
   }
----------------
Can you remind me why this is no longer necessary?

================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:470
@@ -475,1 +469,3 @@
+  unsigned NumDebugCUs = std::distance(M->debug_compile_units_begin(),
+                                       M->debug_compile_units_end());
   // Tell MMI whether we have debug info.
----------------
Nice! This should probably be a separate NFC commit.

================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:491
@@ +490,3 @@
+    for (auto *GV : CUNode->getGlobalVariables()) {
+      const GlobalVariable *Global = nullptr;
+      auto I = GVMap.find(GV);
----------------
```
if (const GlobalVariable *Global = GVMap.lookup(GV))
  CU.getOrCreateGlobalVariableDIE(GV, Global);
```
?

================
Comment at: lib/CodeGen/GlobalMerge.cpp:460
@@ -457,1 +459,3 @@
 
+      MergedGV->copyMetadata(Globals[k], MergedLayout->getElementOffset(idx));
+
----------------
Maybe add a short comment about what's happening here?

================
Comment at: lib/IR/DebugInfoMetadata.cpp:566
@@ -565,1 +565,3 @@
+    case dwarf::DW_OP_stack_value:
+      // Piece and stack value expressions must be at the end.
       return I->get() + I->getSize() == E->get();
----------------
Technically correct, but DW_OP_piece must be even farther at the end ;-)

In the DWARF 5 draft Figure D.61, there is an expression example that uses both to synthesize a struct from the constants {1, 2}:

        DW_OP_lit1 DW_OP_stack_value DW_OP_piece(4)
        DW_OP_lit2 DW_OP_stack_value DW_OP_piece(4)


http://reviews.llvm.org/D20147