[DebugInfo][CallSites] Remove decl subprograms from 'retainedTypes:'

[djtodoro]

Summary

After the D70350, the retainedTypes: isn't being used for the purpose of call site debug info for extern calls, so it is safe to delete it from IR representation.
We are also adding a test to ensure the subprogram isn't stored within the retainedTypes: from corresponding DICompileUnit.

[djtodoro]

Still have test failing:

Clang :: Modules/DebugInfoTransitiveImport.m
Clang :: Modules/ModuleDebugInfo.cpp
Clang :: Modules/ModuleDebugInfo.m

I haven't looked into the failures, but I guess something Objective-C++ related should be handled with some additional work here.

[dblaikie]

In D80369#2048932, @djtodoro wrote:

Still have test failing:

Clang :: Modules/DebugInfoTransitiveImport.m
Clang :: Modules/ModuleDebugInfo.cpp
Clang :: Modules/ModuleDebugInfo.m

I haven't looked into the failures, but I guess something Objective-C++ related should be handled with some additional work here.

These look like over-constrained/accidental tests. At least the ModuleDEbugInfo.m test - the type being tested for won't be emitted, because the type itself isn't in the retained types list nor is it reachable from any other metadata that is going to be emitted.

I think it'd probably be good to clean these tests up - and leave it to @aprantl to look into if this has exposed other bugs - but the bugs are already there, and this IR/debug info metadata isn't working around the bugs, since the IR is going unused anyway.

[djtodoro]

-Remove the decls only in the case of call-site debug info

[djtodoro]

In D80369#2050022, @dblaikie wrote:

In D80369#2048932, @djtodoro wrote:

Still have test failing:

Clang :: Modules/DebugInfoTransitiveImport.m
Clang :: Modules/ModuleDebugInfo.cpp
Clang :: Modules/ModuleDebugInfo.m

I haven't looked into the failures, but I guess something Objective-C++ related should be handled with some additional work here.

These look like over-constrained/accidental tests. At least the ModuleDEbugInfo.m test - the type being tested for won't be emitted, because the type itself isn't in the retained types list nor is it reachable from any other metadata that is going to be emitted.

I think it'd probably be good to clean these tests up - and leave it to @aprantl to look into if this has exposed other bugs - but the bugs are already there, and this IR/debug info metadata isn't working around the bugs, since the IR is going unused anyway.

Looks like ObjC expects the declarations within retained types (based on some tests failures), so I left it as is (I think @aprantl can help us with that).
Therefore, this patch will remove the declarations only in the case of call site debug info for now.

[dblaikie]

In D80369#2050962, @djtodoro wrote:

In D80369#2050022, @dblaikie wrote:

In D80369#2048932, @djtodoro wrote:

Still have test failing:

Clang :: Modules/DebugInfoTransitiveImport.m
Clang :: Modules/ModuleDebugInfo.cpp
Clang :: Modules/ModuleDebugInfo.m

I haven't looked into the failures, but I guess something Objective-C++ related should be handled with some additional work here.

These look like over-constrained/accidental tests. At least the ModuleDEbugInfo.m test - the type being tested for won't be emitted, because the type itself isn't in the retained types list nor is it reachable from any other metadata that is going to be emitted.

I think it'd probably be good to clean these tests up - and leave it to @aprantl to look into if this has exposed other bugs - but the bugs are already there, and this IR/debug info metadata isn't working around the bugs, since the IR is going unused anyway.

Looks like ObjC expects the declarations within retained types (based on some tests failures), so I left it as is (I think @aprantl can help us with that).
Therefore, this patch will remove the declarations only in the case of call site debug info for now.

Yeah, that's what I was trying to say - I think the tests expect that, but I don't see any code path that uses that debug info metadata - I think the test is overconstrained/accidentally testing for this behavior.

I think the right thing to do is to change these tests to test the new behavior (the behavior of the first version of this patch) - and if some use arises/someone can demonstrate a codepath that's using this data, it can be added back in.

@aprantl can you check here? I've attached two IR files for the ModuleDebugInfo.m test, before/after ('x' is before), stripped of the metadata numbers to make diffing easier - but I think you can still follow what's connected to what with reasonable guesswork. The point is these two function declarations end up in the retainedTypes list, and since the function declarations are emitted, so are the types used in their parameters, etc - but those types aren't reachable from anywhere else in the debug info metadata, so they won't be emitted into the final object file so far as I can see (because nothing looks at the function declarations in retainedTypes - only types).

x.ll152 KBDownload

y.ll152 KBDownload

[aprantl]

In D80369#2051278, @dblaikie wrote:

@aprantl can you check here? I've attached two IR files for the ModuleDebugInfo.m test, before/after ('x' is before), stripped of the metadata numbers to make diffing easier - but I think you can still follow what's connected to what with reasonable guesswork. The point is these two function declarations end up in the retainedTypes list, and since the function declarations are emitted, so are the types used in their parameters, etc - but those types aren't reachable from anywhere else in the debug info metadata, so they won't be emitted into the final object file so far as I can see (because nothing looks at the function declarations in retainedTypes - only types).

x.ll152 KBDownload

y.ll152 KBDownload

These two files are for the C++ test, not the Objective-C one. Is the Objective-C case similar?
If there is no explicit CHECK line for the types anchored by the function calls, removing them is fine. However, for ObjC there is explicit code in clang/lib/CodeGen/ObjectFilePCHContainerOperations.cpp DebugTypeVisitor::VisitFunctionDecl() to emit the Objective-C methods. I do want to keep this functionality; it's needed for debugging Swift code with ObjC interoperability. I would be fine with manually adding everything created by this particular visitor to the retained types if that makes the separation easier.

[dblaikie]

In D80369#2051681, @aprantl wrote:

In D80369#2051278, @dblaikie wrote:

@aprantl can you check here? I've attached two IR files for the ModuleDebugInfo.m test, before/after ('x' is before), stripped of the metadata numbers to make diffing easier - but I think you can still follow what's connected to what with reasonable guesswork. The point is these two function declarations end up in the retainedTypes list, and since the function declarations are emitted, so are the types used in their parameters, etc - but those types aren't reachable from anywhere else in the debug info metadata, so they won't be emitted into the final object file so far as I can see (because nothing looks at the function declarations in retainedTypes - only types).

x.ll152 KBDownload

y.ll152 KBDownload

These two files are for the C++ test, not the Objective-C one. Is the Objective-C case similar?

Haven't looked.

If there is no explicit CHECK line for the types anchored by the function calls, removing them is fine. However, for ObjC there is explicit code in clang/lib/CodeGen/ObjectFilePCHContainerOperations.cpp DebugTypeVisitor::VisitFunctionDecl() to emit the Objective-C methods. I do want to keep this functionality; it's needed for debugging Swift code with ObjC interoperability.

How does this data get used for Swift code and ObjC interoperability? At the moment I see no use of this IR metadata in LLVM. Does ObjC/Swift interop use the DI IR over in the Swift compiler? Got a link to the code there?

I would be fine with manually adding everything created by this particular visitor to the retained types if that makes the separation easier.

[aprantl]

How does this data get used for Swift code and ObjC interoperability? At the moment I see no use of this IR metadata in LLVM. Does ObjC/Swift interop use the DI IR over in the Swift compiler? Got a link to the code there?

Are you saying the retained types are not emitted in DWARF?
[Assuming the types do show up in the DWARF sections of the the fat .pcm files —] Swift has a component called ClangImporter that reads Clang ASTs and generates Swift ASTs to provide Swift <-> (Objective-)C interoperability. In the debugger, we don't want to have to depend on the availability of C headers (and the associated brittleness of include paths) just to realize mixed Swift-Objective-C types. Since LLDB already knows how to create Clang ASTs from DWARF, we read the types from DWARF build a Clang AST hand that over to the ClangImporter in LLDB's embedded Swift compiler and thus realize Clang-imported Swift types.

[aprantl]

There's a cute diagram of the interaction: https://github.com/apple/llvm-project/blob/1fda14a45e23c41ac661c20a248a7fa4b102230d/lldb/source/Symbol/SwiftASTContext.cpp#L3020

[dblaikie]

In D80369#2056094, @aprantl wrote:

How does this data get used for Swift code and ObjC interoperability? At the moment I see no use of this IR metadata in LLVM. Does ObjC/Swift interop use the DI IR over in the Swift compiler? Got a link to the code there?

Are you saying the retained types are not emitted in DWARF?

Sort of. I'm saying /these/ types (the ones that these tests are testing for) aren't retained types. The declaration subprograms are in the retained types list - but when retained types are emitted, only types in the retained types list are used, the subprograms (and the types they only indirectly reference) are ignored.

Ah, nope, I'm wrong - I /thought/ the test was correctly flagging these types as missing - that the only reason the types were there was because their subprogram was in the retained types list.

That's incorrect - the types are here, they're just reordered because. At least for the C++ test, this change makes it pass:

diff --git clang/test/Modules/ModuleDebugInfo.cpp clang/test/Modules/ModuleDebugInfo.cpp
index 26369c89605..b1ffe27ec22 100644
--- clang/test/Modules/ModuleDebugInfo.cpp
+++ clang/test/Modules/ModuleDebugInfo.cpp
@@ -51,15 +51,6 @@
 // CHECK-SAME:             )
 // CHECK: !DIEnumerator(name: "e5", value: 5, isUnsigned: true)
 
-// CHECK: !DIDerivedType(tag: DW_TAG_typedef, name: "B",
-// no mangled name here yet.
-
-// This type is anchored by a function parameter.
-// CHECK: !DICompositeType(tag: DW_TAG_class_type, name: "A<void>"
-// CHECK-SAME:             elements:
-// CHECK-SAME:             templateParams:
-// CHECK-SAME:             identifier: "_ZTSN8DebugCXX1AIJvEEE")
-
 // CHECK: !DICompositeType(tag: DW_TAG_structure_type, name: "Struct"
 // CHECK-SAME:             identifier: "_ZTSN8DebugCXX6StructE")
 
@@ -85,6 +76,12 @@
 // CHECK-SAME:             templateParams:
 // CHECK-SAME:             identifier: "_ZTSN8DebugCXX8TemplateIlNS_6traitsIlEEEE")
 
+// This type is anchored by a function parameter.
+// CHECK: !DICompositeType(tag: DW_TAG_class_type, name: "A<void>"
+// CHECK-SAME:             elements:
+// CHECK-SAME:             templateParams:
+// CHECK-SAME:             identifier: "_ZTSN8DebugCXX1AIJvEEE")
+
 // CHECK: !DIDerivedType(tag: DW_TAG_typedef, name: "FloatInstantiation"
 // no mangled name here yet.
 
@@ -93,6 +90,9 @@
 // CHECK-SAME:             flags: DIFlagFwdDecl
 // CHECK-SAME:             identifier: "_ZTSN8DebugCXX8TemplateIfNS_6traitsIfEEEE")
 
+// CHECK: !DIDerivedType(tag: DW_TAG_typedef, name: "B",
+// no mangled name here yet.
+
 // CHECK: !DICompositeType(tag: DW_TAG_structure_type, name: "Virtual"
 // CHECK-SAME:             elements:
 // CHECK-SAME:             identifier: "_ZTS7Virtual")

[djtodoro]

@dblaikie wrote:

... At least for the C++ test, this change makes it pass:

diff --git clang/test/Modules/ModuleDebugInfo.cpp clang/test/Modules/ModuleDebugInfo.cpp
index 26369c89605..b1ffe27ec22 100644
--- clang/test/Modules/ModuleDebugInfo.cpp
+++ clang/test/Modules/ModuleDebugInfo.cpp
@@ -51,15 +51,6 @@
 // CHECK-SAME:             )
 // CHECK: !DIEnumerator(name: "e5", value: 5, isUnsigned: true)
 
-// CHECK: !DIDerivedType(tag: DW_TAG_typedef, name: "B",
-// no mangled name here yet.
-
-// This type is anchored by a function parameter.
-// CHECK: !DICompositeType(tag: DW_TAG_class_type, name: "A<void>"
-// CHECK-SAME:             elements:
-// CHECK-SAME:             templateParams:
-// CHECK-SAME:             identifier: "_ZTSN8DebugCXX1AIJvEEE")
-
 // CHECK: !DICompositeType(tag: DW_TAG_structure_type, name: "Struct"
 // CHECK-SAME:             identifier: "_ZTSN8DebugCXX6StructE")
 
@@ -85,6 +76,12 @@
 // CHECK-SAME:             templateParams:
 // CHECK-SAME:             identifier: "_ZTSN8DebugCXX8TemplateIlNS_6traitsIlEEEE")
 
+// This type is anchored by a function parameter.
+// CHECK: !DICompositeType(tag: DW_TAG_class_type, name: "A<void>"
+// CHECK-SAME:             elements:
+// CHECK-SAME:             templateParams:
+// CHECK-SAME:             identifier: "_ZTSN8DebugCXX1AIJvEEE")
+
 // CHECK: !DIDerivedType(tag: DW_TAG_typedef, name: "FloatInstantiation"
 // no mangled name here yet.
 
@@ -93,6 +90,9 @@
 // CHECK-SAME:             flags: DIFlagFwdDecl
 // CHECK-SAME:             identifier: "_ZTSN8DebugCXX8TemplateIfNS_6traitsIfEEEE")
 
+// CHECK: !DIDerivedType(tag: DW_TAG_typedef, name: "B",
+// no mangled name here yet.
+
 // CHECK: !DICompositeType(tag: DW_TAG_structure_type, name: "Virtual"
 // CHECK-SAME:             elements:
 // CHECK-SAME:             identifier: "_ZTS7Virtual")

I've also faced this scenario, but the problem was with the Modules/ModuleDebugInfo.m (Objective-C) test, since after applying previous version of the patch [0] (plus refacotred Modules/DebugInfoTransitiveImport.m & Modules/ModuleDebugInfo.cpp) there wasn't any DISubprogram at all, and that was the reason I made this version of the patch.

[0]:

debuginfo-remove-decls-from-ret-types.patch4 KBDownload

[dblaikie]

In D80369#2057077, @djtodoro wrote:

@dblaikie wrote:

... At least for the C++ test, this change makes it pass:

diff --git clang/test/Modules/ModuleDebugInfo.cpp clang/test/Modules/ModuleDebugInfo.cpp
index 26369c89605..b1ffe27ec22 100644
--- clang/test/Modules/ModuleDebugInfo.cpp
+++ clang/test/Modules/ModuleDebugInfo.cpp
@@ -51,15 +51,6 @@
 // CHECK-SAME:             )
 // CHECK: !DIEnumerator(name: "e5", value: 5, isUnsigned: true)
 
-// CHECK: !DIDerivedType(tag: DW_TAG_typedef, name: "B",
-// no mangled name here yet.
-
-// This type is anchored by a function parameter.
-// CHECK: !DICompositeType(tag: DW_TAG_class_type, name: "A<void>"
-// CHECK-SAME:             elements:
-// CHECK-SAME:             templateParams:
-// CHECK-SAME:             identifier: "_ZTSN8DebugCXX1AIJvEEE")
-
 // CHECK: !DICompositeType(tag: DW_TAG_structure_type, name: "Struct"
 // CHECK-SAME:             identifier: "_ZTSN8DebugCXX6StructE")
 
@@ -85,6 +76,12 @@
 // CHECK-SAME:             templateParams:
 // CHECK-SAME:             identifier: "_ZTSN8DebugCXX8TemplateIlNS_6traitsIlEEEE")
 
+// This type is anchored by a function parameter.
+// CHECK: !DICompositeType(tag: DW_TAG_class_type, name: "A<void>"
+// CHECK-SAME:             elements:
+// CHECK-SAME:             templateParams:
+// CHECK-SAME:             identifier: "_ZTSN8DebugCXX1AIJvEEE")
+
 // CHECK: !DIDerivedType(tag: DW_TAG_typedef, name: "FloatInstantiation"
 // no mangled name here yet.
 
@@ -93,6 +90,9 @@
 // CHECK-SAME:             flags: DIFlagFwdDecl
 // CHECK-SAME:             identifier: "_ZTSN8DebugCXX8TemplateIfNS_6traitsIfEEEE")
 
+// CHECK: !DIDerivedType(tag: DW_TAG_typedef, name: "B",
+// no mangled name here yet.
+
 // CHECK: !DICompositeType(tag: DW_TAG_structure_type, name: "Virtual"
 // CHECK-SAME:             elements:
 // CHECK-SAME:             identifier: "_ZTS7Virtual")

I've also faced this scenario, but the problem was with the Modules/ModuleDebugInfo.m (Objective-C) test, since after applying previous version of the patch [0] (plus refacotred Modules/DebugInfoTransitiveImport.m & Modules/ModuleDebugInfo.cpp) there wasn't any DISubprogram at all, and that was the reason I made this version of the patch.

Not sure I follow - why was it a problem that there was no DISubprogram at all?

[aprantl]

One way to make the tests more robust is capture the output in a %t.ll file and do repeated RUN: cat %t.ll | FileCheck --check-prefix=TYPE1 lines. Anyway, oif the output is just reshuffled, this should be all fine.

[aprantl]

The declaration subprograms are in the retained types list - but when retained types are emitted, only types in the retained types list are used, the subprograms (and the types they only indirectly reference) are ignored.

So adding the subprogram nodes to retained types has no effect? That's good to know. I think we can remove the code then, as long as the existing tests aren't affected.

[dblaikie]

In D80369#2057874, @aprantl wrote:

The declaration subprograms are in the retained types list - but when retained types are emitted, only types in the retained types list are used, the subprograms (and the types they only indirectly reference) are ignored.

So adding the subprogram nodes to retained types has no effect?

So far as I can tell. The only use (other than serializing and deserializing) of retained types seems to be here: https://github.com/llvm-mirror/llvm/blob/master/lib/CodeGen/AsmPrinter/DwarfDebug.cpp#L882 which filter only the types in the retained types list. I'd say, even, that the retained types list should probably be only types (verifier check?) unless/until we have a use case otherwise. (& if anyone wants to merge the enums list with the retained types list, that'd be a nice cleanup too).

That's good to know. I think we can remove the code then, as long as the existing tests aren't affected.

[djtodoro]

In D80369#2057866, @dblaikie wrote:

Not sure I follow - why was it a problem that there was no DISubprogram at all?

Actually, since the DISubprograms from the retained types don't affect the final DWARF, it's not a problem at all. :)

[djtodoro]

-Tests clean up

[dblaikie]

clang/test/Modules/ModuleDebugInfo.m

46–47 ↗

(On Diff #266776)

Did this type go missing with this change? (ie: was this type /only/ accessible via a subprogram in the retained types list?)

[djtodoro]

clang/test/Modules/ModuleDebugInfo.m

46–47 ↗

(On Diff #266776)

Yes, it was only accessible via that subprogram we removed.

[dblaikie]

clang/test/Modules/ModuleDebugInfo.m

46–47 ↗

(On Diff #266776)

@aprantl - you might want to check whether you really wanted this type & if so, how it should be handled. To the best of my knowledge/understanding, this type never would've made it through to the DWARF output even prior to this change to remove DISubprograms from retainedType - so this doesn't represent (again, to the best of my understanding) a regression, but exposes a potential problem for your use case, I guess?

[aprantl]

clang/test/Modules/ModuleDebugInfo.m

46–47 ↗

(On Diff #266776)

Correct. I will cycle back to this, when I find a use-case that depends on this and potentially add the ability to thread this through to DWARF in that case. Let's not hold up this patch because of that though!

[aprantl]

@vsk: this is going to be relevant if we ever decide to produce DWARF for the extra attributes currently read from the .apinotes (swift names, availability, nullabilkity, ...)

[djtodoro]

Thanks for the reviews!

clang/lib/CodeGen/CGDebugInfo.cpp

void CGDebugInfo::EmitFunctionDecl(GlobalDecl GD, SourceLocation Loc,
   llvm::DISubprogram *SP = DBuilder.createFunction(
       FDContext, Name, LinkageName, Unit, LineNo,
       getOrCreateFunctionType(D, FnType, Unit), ScopeLine, Flags, SPFlags,
       TParamsArray.get(), getFunctionDeclaration(D));
 
   if (IsDeclForCallSite)
     Fn->setSubprogram(SP);
 
-  DBuilder.retainType(SP);
+  DBuilder.finalizeSubprogram(SP);
 }
 
 void CGDebugInfo::EmitFuncDeclForCallSite(llvm::CallBase *CallOrInvoke,
                                           QualType CalleeType,
                                           const FunctionDecl *CalleeDecl) {
   if (!CallOrInvoke)
     return;
   auto *Func = CallOrInvoke->getCalledFunction();

clang/test/CodeGen/debug-info-extern-call.c

 // When entry values are emitted, expect a subprogram for extern decls so that
 // the dwarf generator can describe call site parameters at extern call sites.
 //
+// Initial implementation relied on the 'retainedTypes:' from the corresponding
+// DICompileUnit, so we also ensure that we do not store the extern declaration
+// subprogram into the 'retainedTypes:'.
+//
 // RUN: %clang -g -O2 -target x86_64-none-linux-gnu -S -emit-llvm %s -o - \
 // RUN:   | FileCheck %s -check-prefix=DECLS-FOR-EXTERN
 
 // Similarly, when the debugger tuning is gdb, expect a subprogram for extern
 // decls so that the dwarf generator can describe information needed for tail
 // call frame reconstrution.
 //
 // RUN: %clang -g -O2 -target x86_64-none-linux-gnu -ggdb -S -emit-llvm %s -o - \
 // RUN:   | FileCheck %s -check-prefix=DECLS-FOR-EXTERN
 //
 // Do not emit a subprogram for extern decls when entry values are disabled and
 // the tuning is not set to gdb.
 //
 // RUN: %clang -g -O2 -target x86_64-none-linux-gnu -gsce -S -emit-llvm %s -o - \
 // RUN:   | FileCheck %s -check-prefix=NO-DECLS-FOR-EXTERN
 
+// DECLS-FOR-EXTERN-NOT: !DICompileUnit({{.*}}retainedTypes: ![[RETTYPES:[0-9]+]]
+// DECLS-FOR-EXTERN-NOT: ![[RETTYPES]] = !{
 // DECLS-FOR-EXTERN: !DISubprogram(name: "fn1"
 // DECLS-FOR-EXTERN-NOT: !DISubprogram(name: "memcmp"
 // DECLS-FOR-EXTERN-NOT: !DISubprogram(name: "__some_reserved_name"
 
 // NO-DECLS-FOR-EXTERN-NOT: !DISubprogram(name: "fn1"
 // NO-DECLS-FOR-EXTERN-NOT: !DISubprogram(name: "memcmp"
 // NO-DECLS-FOR-EXTERN-NOT: !DISubprogram(name: "__some_reserved_name"