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

[Amjad Aboud via llvm-commits]

aaboud added a comment.

Hi David,
Thanks for the comments, I tried to answer them.
But, I feels that you are not following the change I am trying to make in this patch.
I described it in the summary, please let me know if the summary is not clear enough.

Thanks


================
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.
----------------
dblaikie wrote:
> Why did this need to sink into createScopeChildrenDIE?
Because, we want to make imported entities defined in the function top level scope behave as those defined in a lexical scope.
"createScopeChildrenDIE" is the only function that is called for both lexical scope and function.
"constructScopeDIE" is not called for function, only for lexical block (or inlined function - not relevant).

================
Comment at: lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:549
@@ +548,3 @@
+      Children.push_back(
+      constructImportedEntityDIE(cast<DIImportedEntity>(E.second)));
+  }
----------------
dblaikie wrote:
> check formatting (run clang-format over the patch?)
Will do.
Though, I think I already did and this was the result, but I will double check.

================
Comment at: lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:553
@@ -553,1 +552,3 @@
+  if (HasNoneScopeChild)
+    *HasNoneScopeChild = Children.size() != 0;
 
----------------
dblaikie wrote:
> 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.
I will use the "empty()" method.

Why I changed that to an indicator instead of a counter?
1. Because this is the right behavior.
2. The reason it was a counter, is that we had to add the imported entities outside this function, so we could not calculate the indicator at this function, now we can.
3. The indicator is needed for next patch, and I really do not want to change to indicator in the next patch to prevent force unrelated (but needed) changes there.

Is there any reason why you would prefer a counter that will never be used as a counter?

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

================
Comment at: test/DebugInfo/imported_entities.ll:42
@@ +41,3 @@
+; CHECK-NOT: NULL
+; CHECK: DW_TAG_imported_module
+; CHECK-NOT: DW_TAG
----------------
dblaikie wrote:
> 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))
I am not quite following.
These imported entities are defined inside the abstract function under the abstract function scope.
According to my plan, this is where they will keep being defined.
So, how do you see the final solution looks like?
Where do you want to define imported entities with respect to inline and abstract functions/lexical scopes?


Repository:
  rL LLVM

http://reviews.llvm.org/D12913