[PATCH] Fixed debug info generation for function static variables, typedef, and records

[Frederic Riss]

REPOSITORY
  rL LLVM

================
Comment at: lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:101
@@ -100,3 +100,3 @@
 DIE *DwarfCompileUnit::getOrCreateGlobalVariableDIE(
-    const DIGlobalVariable *GV) {
+  const DIGlobalVariable *GV, DIE *ContextDIE) {
   // Check for pre-existence.
----------------
A comment for the new argument would be nice (here or in the header).

================
Comment at: lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:341
@@ -339,3 +340,3 @@
     // Skip imported directives in gmlt-like data.
     if (!includeMinimalInlineScopes()) {
       // There is no need to emit empty lexical block DIE.
----------------
I think the local types and static vars should respect this flag like the imported entities does. If we go the way of unifying the 3 maps (se comments bellow), then everything can be handled inside that if.

================
Comment at: lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:354-355
@@ -350,1 +353,4 @@
+    if (Children.size() == ChildScopeCount &&
+        GVRangeForScope.begin() == GVRangeForScope.end() &&
+        RTRangeForScope.begin() == RTRangeForScope.end()) {
       FinalChildren.insert(FinalChildren.end(),
----------------
This could just query .empty() on the containers.

================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:459
@@ +458,3 @@
+    for (auto *GV : CUNode->getGlobalVariables()) {
+      if (CU.getOrCreateContextDIE(GV->getScope()))
+        CU.getOrCreateGlobalVariableDIE(GV);
----------------
Is that condition always safe to check if the scope is function-local? Just looking at the name of the function, it looks like we could change that one day to do something different for local scope (like create them right away like the name implies) and this would break the logic here.

================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.h:261-262
@@ -260,4 +260,4 @@
 
   // Holders for the various debug information flags that we might need to
   // have exposed. See accessor functions below for description.
 
----------------
Not related to your patch, but this look like an orphaned comment.

================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.h:265-271
@@ -265,4 +264,9 @@
   typedef SmallVector<std::pair<const MDNode *, const MDNode *>, 32>
-  ImportedEntityMap;
+  ImportedEntityMap, GlobalVariableMap, RetainedTypeMap;
+  // Holder for imported entities.
   ImportedEntityMap ScopesWithImportedEntities;
+  // Holder for global variables.
+  GlobalVariableMap ScopesWithGlovalVariables;
+  // Holder for retained types.
+  RetainedTypeMap ScopesWithRetainedTypes;
 
----------------
Couldn't we just merge the 3 maps. That are all used only in one and same place, and their handling is really similar. The fundamental difference between the 2 new ones and the existing one is that the DIEs created by getOrCreate{Type,Variable}DIE are automatically added to their parents, which doesn't happen for the imported entities. Maybe we should just unify all that? (I think David has written this part, so he might have an opinion on this)

================
Comment at: lib/CodeGen/AsmPrinter/DwarfUnit.cpp:744-745
@@ -743,1 +743,4 @@
 
+  if (DIE *TyDIE = getDIE(Ty))
+    return TyDIE;
+
----------------
Why is that new shortcut needed?

http://reviews.llvm.org/D9758

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/