[PATCH] D12913: Fixed debug info emitting for imported entity defined directly in a function scope.

[David Blaikie via llvm-commits]

dblaikie added a comment.

"Imported entity defined in a lexical block is handled correctly by emitting a debug info entry in each lexical block instance (abstract, inline and concrete), if it has code."

Is that the right behavior? (maybe an intermediate step towards consistency, with the end goal being to put the imported entities only in the abstract origin, if available, otherwise in the concrete function?)


================
Comment at: lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:336
@@ -335,3 +335,3 @@
 
-    unsigned ChildScopeCount;
+    bool HasNoneScopeChild;
 
----------------
I'm OK leaving it uninitialized if that's the intent - it means tools like MSan and the like can catch cases where the intent is violated (if we initialize it, then fail to set it where we intend, then use it - these tools can't help us because they can't distinguish the intentional from the unintentional)

================
Comment at: lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:340
@@ -339,11 +339,3 @@
     // null and the children will be added to the scope DIE.
-    createScopeChildrenDIE(Scope, Children, &ChildScopeCount);
-
-    // Skip imported directives in gmlt-like data.
-    if (!includeMinimalInlineScopes()) {
-      // There is no need to emit empty lexical block DIE.
-      for (const auto &E : DD->findImportedEntitiesForScope(DS))
-        Children.push_back(
-            constructImportedEntityDIE(cast<DIImportedEntity>(E.second)));
-    }
+    createScopeChildrenDIE(Scope, Children, &HasNoneScopeChild);
 
----------------
What's the purpose of the "HasNoneScopeChild" parameter compared to just testing if Children is non-empty after the call?

I take it it's to detect whether Children has /immediate/ children, or only more scopes? (hmm, I don't remember that out parameter "ChildScopeCount" going in - not sure when that happened, maybe I did that for all I know/recall... it's been a while since I've played with this code)

Anyway, I think I follow/seems reasonable enough... 

================
Comment at: lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:343
@@ -342,3 @@
-    // Skip imported directives in gmlt-like data.
-    if (!includeMinimalInlineScopes()) {
-      // There is no need to emit empty lexical block DIE.
----------------
Why did this need to sink into createScopeChildrenDIE?

================
Comment at: lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:549
@@ +548,3 @@
+      Children.push_back(
+      constructImportedEntityDIE(cast<DIImportedEntity>(E.second)));
+  }
----------------
check formatting (run clang-format over the patch?)

================
Comment at: lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:553
@@ -553,1 +552,3 @@
+  if (HasNoneScopeChild)
+    *HasNoneScopeChild = Children.size() != 0;
 
----------------
prefer !empty over size != 0

Also could you explain why the change from from using a size difference (size - child count without scopes) to an absolute non-empty test? I'm not quite following.

================
Comment at: test/DebugInfo/imported_entities.ll:35
@@ +34,3 @@
+;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
+; CHECK: [[F2:.*]]: DW_TAG_subprogram
+; CHECK-NOT: DW_TAG
----------------
This is the abstract definition, yes?

================
Comment at: test/DebugInfo/imported_entities.ll:42
@@ +41,3 @@
+; CHECK-NOT: NULL
+; CHECK: DW_TAG_imported_module
+; CHECK-NOT: DW_TAG
----------------
These are imported entities without any scope parenting? & are probably useless at the moment, I imagine? (but not a regression caused by your change, just the current state of affairs - maybe a comment in the test case describing what they should be in the end (either removed, or adding scope information/removing the imported entities from the actual inlined and concrete instances, etc))


Repository:
  rL LLVM

http://reviews.llvm.org/D12913