Fixed debug info emitting for imported entity defined directly in a function scope.

[aaboud]

Summary

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.
However, if the imported entity was defined in a function scope (top level), it is emitted only in the concrete function, even if it has no code!

This patch solve the issue by handling imported entity defined in a function scope similar to the one defined in a lexical block scope.

[aaboud]

Hi,
Can I get approval or feedback on this patch?
Thanks

[aprantl]

lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp

336

HasNoneScopeChild is uninitialized here.

538

This should probably be a reference like Children.

552

This looks like a use of an undef variable?

[aaboud]

Thanks for your comments.
Answered below.

lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp

336

I guess it won't hurt to initialize it to "false", though the "createScopeChildrenDIE" will always initialize it.

538

It is a pointer and not a reference because not all callers need this information, so they can simply not send a place holder for this return info.

Notice that I followed the previous implementation, just changed the counter with an indicator, as the counter meant to be used as indication for if the lexical scope has some children that are not other lexical scopes.

552

Not exactly.
This checks that the pointer is not "nullptr".
HasNoneScopeChild is a pointer with a nullptr default value.
As not all callers interested in this information.

[aprantl]

Ah yes. Sorry for the noise!

[dblaikie]

Just glanced at the tests, haven't looked at the code yet.

(not sure if the test filecheck lines should be further simplified by dropping some of the attributes - the basics of line/file location on imported entities is tested elsewhere, perhaps this test should just focus on where the imported entities are placed in the DIE tree)

test/DebugInfo/imported_entities.ll

13

Why do you need SROA here?

20

I usually write these tests by using attribute((always_inline)) then even at -O0 clang will run LLVM's AlwaysInliner and inline this, rather than relying on the normal inliner choosing to inline the function.

27

Is the parameter necessary? If you need to leave breadcrumbs to ensure that foo is inlined and not optimized away, you can add a call to an external function in the inlined function:

void f1();
__attribute__((alwaysinline)) void f2() {
  using ...;
  f1();
}
void f3() {
  f2();
}

[aaboud]

test/DebugInfo/imported_entities.ll

13

It makes the LLVM IR simpler by removing the "lifetime" intrinsic that is added by the "-inline" pass.
Other than that it is not needed.

Anyway, once I use the "always_inline" attribute you suggested, there is no need for "-inline" nor "-soa" passes.

20

Turned to be a better way to write the test.
I just needed to keep the static, so clang can get rid of the inlined function once it inline it.

[aaboud]

Following David comments, I managed to reduced the new test and make it much simpler.

[aaboud]

Changed the "static" keyword with "inline" keyword in the LIT test.

David, do I have an approval to commit? Or you would like to take another look on the code?

Thanks

[dblaikie]

"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?)

lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp

336

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)

340

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...

343

Why did this need to sink into createScopeChildrenDIE?

549

check formatting (run clang-format over the patch?)

553

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.

test/DebugInfo/imported_entities.ll

36

This is the abstract definition, yes?

43

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))

[aaboud]

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

lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp

343

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).

549

Will do.
Though, I think I already did and this was the result, but I will double check.

553

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?

test/DebugInfo/imported_entities.ll

36

Yes.

43

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?

[aaboud]

Applied David comments.

David, do you have other comments or last minute thoughts?

[aaboud]

David,
Any more comments?

[aaboud]

Can I commit this patch?
Is there any more comments?

Thanks,
Amjad

[aaboud]

Updated code to top of trunk.
This commit is still needed as first patch for complete implementation of "RFC: Supporting all entities declared in lexical scope in LLVM debug info"
http://lists.llvm.org/pipermail/llvm-dev/2015-December/093313.html

lib/CodeGen/AsmPrinter/DwarfCompileUnit.h

public:
   DIE *constructVariableDIE(DbgVariable &DV, bool Abstract = false);
 
   DIE *constructVariableDIE(DbgVariable &DV, const LexicalScope &Scope,
                             DIE *&ObjectPointer);
 
   /// A helper function to create children of a Scope DIE.
   DIE *createScopeChildrenDIE(LexicalScope *Scope,
                               SmallVectorImpl<DIE *> &Children,
-                              unsigned *ChildScopeCount = nullptr);
+                              bool *HasNoneScopeChild = nullptr);
 
   /// \brief Construct a DIE for this subprogram scope.
   void constructSubprogramScopeDIE(LexicalScope *Scope);
 
   DIE *createAndAddScopeChildren(LexicalScope *Scope, DIE &ScopeDIE);
 
   void constructAbstractSubprogramScopeDIE(LexicalScope *Scope);
 

lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp

if (!ScopeDIE)
       return;
     // We create children when the scope DIE is not null.
     createScopeChildrenDIE(Scope, Children);
   } else {
     // Early exit when we know the scope DIE is going to be null.
     if (DD->isLexicalScopeDIENull(Scope))
       return;
 
-    unsigned ChildScopeCount;
+    bool HasNoneScopeChild;
 
     // We create children here when we know the scope DIE is not going to be
     // 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);
 
     // If there are only other scopes as children, put them directly in the
     // parent instead, as this scope would serve no purpose.
-    if (Children.size() == ChildScopeCount) {
+    if (!HasNoneScopeChild) {
       FinalChildren.insert(FinalChildren.end(),
                            std::make_move_iterator(Children.begin()),
                            std::make_move_iterator(Children.end()));
       return;
     }
     ScopeDIE = constructLexicalScopeDIE(Scope);
     assert(ScopeDIE && "Scope DIE should not be null.");
   }
DIE *DwarfCompileUnit::constructVariableDIE(DbgVariable &DV,
   auto Var = constructVariableDIE(DV, Scope.isAbstractScope());
   if (DV.isObjectPointer())
     ObjectPointer = Var;
   return Var;
 }
 
 DIE *DwarfCompileUnit::createScopeChildrenDIE(LexicalScope *Scope,
                                               SmallVectorImpl<DIE *> &Children,
-                                              unsigned *ChildScopeCount) {
+                                              bool *HasNoneScopeChild) {
   DIE *ObjectPointer = nullptr;
+  auto *DS = Scope->getScopeNode();
 
   for (DbgVariable *DV : DU->getScopeVariables().lookup(Scope))
     Children.push_back(constructVariableDIE(*DV, *Scope, ObjectPointer));
 
-  unsigned ChildCountWithoutScopes = Children.size();
+  // Skip imported directives in gmlt-like data.
+  if (!includeMinimalInlineScopes()) {
+    for (const auto &E : DD->findImportedEntitiesForScope(DS))
+      Children.push_back(
+          constructImportedEntityDIE(cast<DIImportedEntity>(E.second)));
+  }
+
+  if (HasNoneScopeChild)
+    *HasNoneScopeChild = !Children.empty();
 
   for (LexicalScope *LS : Scope->getChildren())
     constructScopeDIE(LS, Children);
 
-  if (ChildScopeCount)
-    *ChildScopeCount = Children.size() - ChildCountWithoutScopes;
-
   return ObjectPointer;
 }
 
 void DwarfCompileUnit::constructSubprogramScopeDIE(LexicalScope *Scope) {
   assert(Scope && Scope->getScopeNode());
   assert(!Scope->getInlinedAt());
   assert(!Scope->isAbstractScope());
   auto *Sub = cast<DISubprogram>(Scope->getScopeNode());

lib/CodeGen/AsmPrinter/DwarfDebug.cpp

DwarfDebug::constructDwarfCompileUnit(const DICompileUnit *DIUnit) {
 
   CUMap.insert(std::make_pair(DIUnit, &NewCU));
   CUDieMap.insert(std::make_pair(&Die, &NewCU));
   return NewCU;
 }
 
 void DwarfDebug::constructAndAddImportedEntityDIE(DwarfCompileUnit &TheCU,
                                                   const DIImportedEntity *N) {
-  if (DIE *D = TheCU.getOrCreateContextDIE(N->getScope()))
+  auto S = N->getScope();
+  if (S && isa<DILexicalBlockBase>(S) || isa<DISubprogram>(S))
+    // Delay handling imported entity defined in function/lexical block scope.
+    // It will be handled when processing the function/lexical block.
+    return;
+  if (DIE *D = TheCU.getOrCreateContextDIE(S))
     D->addChild(TheCU.constructImportedEntityDIE(N));
 }
 
 // Emit all Dwarf sections that should come prior to the content. Create
 // global DIEs and emit initial debug info sections. This is invoked by
 // the target AsmPrinter.
 void DwarfDebug::beginModule() {
   if (DisableDebugInfoPrinting)

test/DebugInfo/imported_entities.ll

+; RUN: %llc_dwarf -O0 -filetype=obj -dwarf-linkage-names=Enable < %s | llvm-dwarfdump -debug-dump=info - | FileCheck %s
+
+;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
+;; Test case where imported entities are defined in inlined function scope.
+;; The test checks the following:
+;; 1. Inlined function "f2" is emitted only as abstract debug info entry and
+;;    inlined debug info entry.
+;; 2. Both entries has two imported DIE children:
+;;    DW_TAG_imported_module - "NameSpace"
+;;    DW_TAG_imported_declaration - "TypeDef"
+;;
+;; Note: The optimal solution would be to emit the imported entries only in the
+;;       abstratc lexical scope, while debugger be able to correlate between it
+;;       and the inline lexical scope through DW_AT_abstract_origin attribute.
+;;       However, this can be implemented only after fixing debugger (gdb/lldb).
+;;
+;; This test was generated by running following command:
+;; clang -cc1 -O0 -g -emit-llvm test.cpp -o -
+;; Where test.cpp
+;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
+;;namespace NameSpace {
+;;  typedef int TypeDef;
+;;}
+;;
+;;void f1();
+;;inline void __attribute__((always_inline)) f2() {
+;;  using namespace NameSpace;
+;;  using NameSpace::TypeDef;
+;;  f1();
+;;}
+;;void f3() {
+;;  f2();
+;;}
+;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
+
+; CHECK: debug_info contents
+
+;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
+;; This part checks for imported entity entries in abstract function entry.
+;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
+; CHECK: [[F2:.*]]: DW_TAG_subprogram
+; CHECK-NOT: DW_TAG
+; CHECK: DW_AT_name{{.*}}= "f2"
+; CHECK-NOT: DW_TAG
+; CHECK: DW_AT_inline
+
+; CHECK-NOT: NULL
+; CHECK: DW_TAG_imported_module
+; CHECK-NOT: DW_TAG
+; CHECK: DW_AT_import{{.*}}=> {[[NameSpace:0x[0-9a-f]*]]})
+
+; CHECK-NOT: NULL
+; CHECK: DW_TAG_imported_declaration
+; CHECK-NOT: DW_TAG
+; CHECK: DW_AT_import{{.*}}=> {[[TypeDef:0x[0-9a-f]*]]})
+
+; CHECK: NULL
+
+;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
+;; This part checks for declaration entries, which are imported elsewhere.
+;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
+; CHECK: [[NameSpace]]:{{ *}}DW_TAG_namespace
+; CHECK-NEXT: DW_AT_name{{.*}} = "NameSpace"
+
+; CHECK-NOT: NULL
+; CHECK: [[TypeDef]]:{{.*}}DW_TAG_typedef
+; CHECK-NOT: DW_TAG
+; CHECK: DW_AT_name{{.*}}= "TypeDef"
+
+; CHECK: NULL
+
+;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
+;; This part checks for imported entity entries in inlined function entry.
+;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
+; CHECK: DW_TAG_subprogram
+; CHECK-NOT: DW_TAG
+; CHECK: DW_AT_name{{.*}}= "f3"
+
+; CHECK-NOT: NULL
+; CHECK: DW_TAG_inlined_subroutine
+; CHECK-NEXT: DW_AT_abstract_origin{{.*}}{[[F2]]}
+
+; CHECK-NOT: NULL
+; CHECK: DW_TAG_imported_module
+; CHECK-NOT: DW_TAG
+; CHECK: DW_AT_import{{.*}}=> {[[NameSpace:0x[0-9a-f]*]]})
+
+; CHECK-NOT: NULL
+; CHECK: DW_TAG_imported_declaration
+; CHECK-NOT: DW_TAG
+; CHECK: DW_AT_import{{.*}}=> {[[TypeDef:0x[0-9a-f]*]]})
+
+; CHECK: NULL
+; CHECK-NOT: DW_TAG_subprogram
+
+; Function Attrs: nounwind
+define void @"\01?f3@@YAXXZ"() #0 {
+entry:
+  call void @"\01?f1@@YAXXZ"() #2, !dbg !17
+  ret void, !dbg !19
+}
+
+declare void @"\01?f1@@YAXXZ"() #1
+
+attributes #0 = { nounwind "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="false" "no-infs-fp-math"="false" "no-nans-fp-math"="false" "no-realign-stack" "stack-protector-buffer-size"="8" "unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #1 = { "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="false" "no-infs-fp-math"="false" "no-nans-fp-math"="false" "no-realign-stack" "stack-protector-buffer-size"="8" "unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #2 = { nounwind }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!14, !15}
+!llvm.ident = !{!16}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !1, producer: "clang version 3.8.0 (trunk 246757)", isOptimized: false, runtimeVersion: 0, emissionKind: 1, enums: !2, subprograms: !3, imports: !8)
+!1 = !DIFile(filename: "test.cpp", directory: "/")
+!2 = !{}
+!3 = !{!4, !7}
+!4 = distinct !DISubprogram(name: "f3", linkageName: "\01?f3@@YAXXZ", scope: !1, file: !1, line: 10, type: !5, isLocal: false, isDefinition: true, scopeLine: 10, flags: DIFlagPrototyped, isOptimized: false, function: void ()* @"\01?f3@@YAXXZ", variables: !2)
+!5 = !DISubroutineType(types: !6)
+!6 = !{null}
+!7 = distinct !DISubprogram(name: "f2", linkageName: "\01?f2@@YAXXZ", scope: !1, file: !1, line: 5, type: !5, isLocal: false, isDefinition: true, scopeLine: 5, flags: DIFlagPrototyped, isOptimized: false, variables: !2)
+!8 = !{!9, !11}
+!9 = !DIImportedEntity(tag: DW_TAG_imported_module, scope: !7, entity: !10, line: 6)
+!10 = !DINamespace(name: "NameSpace", scope: null, file: !1, line: 1)
+!11 = !DIImportedEntity(tag: DW_TAG_imported_declaration, scope: !7, entity: !12, line: 7)
+!12 = !DIDerivedType(tag: DW_TAG_typedef, name: "TypeDef", scope: !10, file: !1, line: 2, baseType: !13)
+!13 = !DIBasicType(name: "int", size: 32, align: 32, encoding: DW_ATE_signed)
+!14 = !{i32 2, !"Dwarf Version", i32 4}
+!15 = !{i32 2, !"Debug Info Version", i32 3}
+!16 = !{!"clang version 3.8.0 (trunk 246757)"}
+!17 = !DILocation(line: 8, scope: !7, inlinedAt: !18)
+!18 = distinct !DILocation(line: 11, scope: !4)
+!19 = !DILocation(line: 12, scope: !4)

test/DebugInfo/namespace.ll

 ; CHECK-NEXT: DW_AT_name{{.*}}= "var_fwd"
 ; CHECK-NOT: NULL
 ; CHECK: [[FOO:0x[0-9a-f]*]]:{{ *}}DW_TAG_structure_type
 ; CHECK-NEXT: DW_AT_name{{.*}}= "foo"
 ; CHECK-NEXT: DW_AT_declaration
 ; CHECK-NOT: NULL
 ; CHECK: [[BAR:0x[0-9a-f]*]]:{{ *}}DW_TAG_structure_type
 ; CHECK-NEXT: DW_AT_name{{.*}}= "bar"
+; CHECK: DW_TAG_subprogram
+; CHECK-NOT: DW_TAG
+; CHECK: DW_AT_MIPS_linkage_name
+; CHECK-NOT: DW_TAG
+; CHECK: DW_AT_name{{.*}}= "f1"
 ; CHECK: [[FUNC1:.*]]: DW_TAG_subprogram
 ; CHECK-NOT: DW_TAG
 ; CHECK: DW_AT_MIPS_linkage_name
 ; CHECK-NOT: DW_TAG
 ; CHECK: DW_AT_name{{.*}}= "f1"
 ; CHECK: [[BAZ:0x[0-9a-f]*]]:{{.*}}DW_TAG_typedef
 ; CHECK-NOT: DW_TAG
 ; CHECK: DW_AT_name{{.*}}= "baz"
 ; CHECK: [[VAR_DECL:0x[0-9a-f]*]]:{{.*}}DW_TAG_variable
 ; CHECK-NOT: DW_TAG
 ; CHECK: DW_AT_name{{.*}}= "var_decl"
 ; CHECK-NOT: DW_TAG
 ; CHECK: DW_AT_declaration
 ; CHECK: [[FUNC_DECL:0x[0-9a-f]*]]:{{.*}}DW_TAG_subprogram
 ; CHECK-NOT: DW_TAG
 ; CHECK: DW_AT_name{{.*}}= "func_decl"
 ; CHECK-NOT: DW_TAG
 ; CHECK: DW_AT_declaration
 ; CHECK: [[FUNC_FWD:0x[0-9a-f]*]]:{{.*}}DW_TAG_subprogram
 ; CHECK-NOT: DW_TAG
 ; CHECK: DW_AT_name{{.*}}= "func_fwd"
 ; CHECK-NOT: DW_AT_declaration
-; CHECK: DW_TAG_subprogram
-; CHECK-NOT: DW_TAG
-; CHECK: DW_AT_MIPS_linkage_name
-; CHECK-NOT: DW_TAG
-; CHECK: DW_AT_name{{.*}}= "f1"
 ; CHECK: NULL
 
 ; CHECK-NOT: NULL
 ; CHECK: DW_TAG_imported_module
 ; This is a bug, it should be in F2 but it inherits the file from its
 ; enclosing scope
 ; CHECK-NEXT: DW_AT_decl_file{{.*}}([[F1]])
 ; CHECK-NEXT: DW_AT_decl_line{{.*}}(15)
 ; CHECK-NEXT: DW_AT_import{{.*}}=> {[[NS2]]})
 ; CHECK: NULL
 ; CHECK-NOT: NULL
 
 ; CHECK: DW_TAG_imported_module
 ; Same bug as above, this should be F2, not F1
 ; CHECK-NEXT: DW_AT_decl_file{{.*}}([[F1]])
 ; CHECK-NEXT: DW_AT_decl_line{{.*}}(18)
 ; CHECK-NEXT: DW_AT_import{{.*}}=> {[[NS1]]})
 ; CHECK-NOT: NULL
 
 ; CHECK: DW_TAG_subprogram
+; CHECK: DW_AT_name{{.*}}= "__cxx_global_var_init"
+; CHECK-NOT: DW_TAG
+
+; CHECK: DW_TAG_subprogram
 ; CHECK-NOT: DW_TAG
 ; CHECK: DW_AT_MIPS_linkage_name
 ; CHECK-NOT: DW_TAG
 ; CHECK: DW_AT_name{{.*}}= "func"
 ; CHECK-NOT: NULL
 ; CHECK: DW_TAG_imported_module
 ; CHECK-NEXT: DW_AT_decl_file{{.*}}([[F2]])
 ; CHECK-NEXT: DW_AT_decl_line{{.*}}(26)