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

[Frederic Riss]

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.
----------------
aaboud wrote:
> 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!
Actually we do not emit locals in that case. If you lookup the function includeMinimalInlineScopes() you'll see that it is true in line-tables-only mode in which we do not emit locals.

================
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(),
----------------
aaboud wrote:
> friss wrote:
> > This could just query .empty() on the containers.
> I wish it have.
> the iterator_range container has no "empty()" method.
righto, I thought this returned a container and I didn't come back to it when reading the function definitions below.


================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:459
@@ +458,3 @@
+    for (auto *GV : CUNode->getGlobalVariables()) {
+      if (CU.getOrCreateContextDIE(GV->getScope()))
+        CU.getOrCreateGlobalVariableDIE(GV);
----------------
aaboud wrote:
> 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.
Sorry, you didn't get my point. You are no just calling function that looks up the scope DIE. As its name implies, it can create things. I'm asking if the side effects of that function are safe to apply here. And if they ever change to return a scope where they don't return any today would it break the logic in getOrCreateGlobalVariable()? Looking at the code there, it seems safe on both fronts because that function is called anyway as one the the very first things (and this also applies to the getOrCreateType() call below).

================
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;
 
----------------
aaboud wrote:
> 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.
Well, I'd try to put everything in the same map (renaming it beforehand to something like 'LocalDeclMap'. Then in constructScopeDIE, discriminate between the different types using dyn_cast (and handle everything in the same loop under the includeMinimalInlineScopes() condition). Is there anything I'm missing that makes this impractical?

================
Comment at: lib/CodeGen/AsmPrinter/DwarfUnit.cpp:744-745
@@ -743,1 +743,4 @@
 
+  if (DIE *TyDIE = getDIE(Ty))
+    return TyDIE;
+
----------------
aaboud wrote:
> 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.
> 
The comment a few lines below ("Construct the context before querying for the existence of the DIE in case such construction creates the DIE") makes it seem like this is deliberate. Anyway if it's not needed for the patch it shouldn't be here (if you think it is a good idea, it can be the subject of another patch).

http://reviews.llvm.org/D9758

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