Page MenuHomePhabricator

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

Authored by djtodoro on Thu, May 21, 2:18 AM.

Details

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.

Diff Detail

Event Timeline

djtodoro created this revision.Thu, May 21, 2:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptThu, May 21, 2:18 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
djtodoro updated this revision to Diff 265481.Thu, May 21, 5:50 AM
djtodoro retitled this revision from [DebugInfo] Remove decl subprograms from 'retainedTypes:' to WIP: [DebugInfo] Remove decl subprograms from 'retainedTypes:'.

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.

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 updated this revision to Diff 265722.Fri, May 22, 5:58 AM
djtodoro retitled this revision from WIP: [DebugInfo] Remove decl subprograms from 'retainedTypes:' to [DebugInfo][CallSites] Remove decl subprograms from 'retainedTypes:'.

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

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.

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

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

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.

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

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.

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.

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

@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]:

@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?

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.

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.

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.

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 updated this revision to Diff 266776.Thu, May 28, 1:59 AM

-Tests clean up

dblaikie added inline comments.Thu, May 28, 12:30 PM
clang/test/Modules/ModuleDebugInfo.m
46–47

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

djtodoro marked an inline comment as done.Fri, May 29, 12:43 AM
djtodoro added inline comments.
clang/test/Modules/ModuleDebugInfo.m
46–47

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

dblaikie added inline comments.Fri, May 29, 9:39 AM
clang/test/Modules/ModuleDebugInfo.m
46–47

@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 added inline comments.Fri, May 29, 2:05 PM
clang/test/Modules/ModuleDebugInfo.m
46–47

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 accepted this revision.Fri, May 29, 2:06 PM
This revision is now accepted and ready to land.Fri, May 29, 2:06 PM

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

Thanks for the reviews!

This revision was automatically updated to reflect the committed changes.