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

[Amjad Aboud]

REPOSITORY
  rL LLVM

================
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.
----------------
friss wrote:
> 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.
I see no reason to add them to the minimal flag.
We did not skip emitting local variable due to this flag, did we?
Imported Entities should respect the flag, but you will need to explain why other should!

================
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(),
----------------
friss wrote:
> This could just query .empty() on the containers.
I wish it have.
the iterator_range container has no "empty()" method.

================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:459
@@ +458,3 @@
+    for (auto *GV : CUNode->getGlobalVariables()) {
+      if (CU.getOrCreateContextDIE(GV->getScope()))
+        CU.getOrCreateGlobalVariableDIE(GV);
----------------
friss wrote:
> 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.
If this function would be able to create a lexical block DIE, I would not need to implement this patch from the first place, right?
Also, if you check line 490. when we construct the Imported Entities, it does the same check. Actually, I copied this from their.

================
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.
 
----------------
friss wrote:
> Not related to your patch, but this look like an orphaned comment.
OK, will remove.

================
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;
 
----------------
friss wrote:
> 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)
I do not mind merging them, just need some good ideas on how to do that.

================
Comment at: lib/CodeGen/AsmPrinter/DwarfUnit.cpp:744-745
@@ -743,1 +743,4 @@
 
+  if (DIE *TyDIE = getDIE(Ty))
+    return TyDIE;
+
----------------
friss wrote:
> Why is that new shortcut needed?
Yes, I thought it is new, it is not. I just moved it from below.
1. I see no good reason to create the context if we already have the DIE itself.
2. For Type DIEs with lexical block context, the function ContextDIE from below will be nullptr at this point.

http://reviews.llvm.org/D9758

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