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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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).
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.
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.
There's a cute diagram of the interaction: https://github.com/apple/llvm-project/blob/1fda14a45e23c41ac661c20a248a7fa4b102230d/lldb/source/Symbol/SwiftASTContext.cpp#L3020
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]:
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.
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.
Actually, since the DISubprograms from the retained types don't affect the final DWARF, it's not a problem at all. :)
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?) |
clang/test/Modules/ModuleDebugInfo.m | ||
---|---|---|
46–47 | Yes, it was only accessible via that subprogram we removed. |
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? |
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! |
@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, ...)
Did this type go missing with this change? (ie: was this type /only/ accessible via a subprogram in the retained types list?)