[DebugInfo] Place local ODR-uniqued types in decl DISubprograms #142166
Conversation
There are two flavours of DISubprogram: declarations, which are unique'd and abstractly describe the function in question. There are also definition DISubprograms which correspond to real instances, some of which are inlined, duplicated across translation units in LTO, or otherwise can have multiple instances. Given that LLVM sometimes force-uniques types by their ODR-name, see the enableDebugTypeODRUniquing feature, we shouldn't place types that might be unique'd into duplicated contexts like definition DISubprograms. Instead, place them into the declaration. This slightly bends the existing approach where only functions that have a separate declaratrion to their definition get a declaration-DISubprogram. A single function in a translation unit might now get a declaration where it didn't before, if it contains an ODR-unique'd type declaration. This seems reasonable given that the LLVM idea of a declaration doesn't have to exactly match source-language ideas. The added cpp test checks that such ORD-unique'd types are detected and placed in the declaration DISubprogram, creating one if necessary. The IR test ensures that nothing changes after a round-trip with enableDebugTypeODRUniquing turned on.
dzhidzhoev
requested review from
jmorse,
adrian-prantl,
SLTozer,
OCHyams,
dwblaikie,
pogo59 and
CarlosAlbertoEnciso
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-codegen @llvm/pr-subscribers-debuginfo Author: Vladislav Dzhidzhoev (dzhidzhoev) ChangesThis commit was split from #119001 commit chain after the discussion with @jmorse. It can be merged before the fixed version of #75385, which will help to track down issues if they pop up after the merge. The original commit message follows: There are two flavours of DISubprogram: declarations, which are unique'd and abstractly describe the function in question. There are also definition DISubprograms which correspond to real instances, some of which are inlined, duplicated across translation units in LTO, or otherwise can have multiple instances. Given that LLVM sometimes force-uniques types by their ODR-name, see the enableDebugTypeODRUniquing feature, we shouldn't place types that might be unique'd into duplicated contexts like definition DISubprograms. Instead, place them into the declaration. This slightly bends the existing approach where only functions that have a separate declaratrion to their definition get a declaration-DISubprogram. A single function in a translation unit might now get a declaration where it didn't before, if it contains an ODR-unique'd type declaration. This seems reasonable given that the LLVM idea of a declaration doesn't have to exactly match source-language ideas. The added cpp test checks that such ORD-unique'd types are detected and placed in the declaration DISubprogram, creating one if necessary. The IR test ensures that nothing changes after a round-trip with enableDebugTypeODRUniquing turned on. Full diff: https://github.com/llvm/llvm-project/pull/142166.diff 4 Files Affected:
diff --git a/clang/lib/CodeGen/CGDebugInfo.cpp b/clang/lib/CodeGen/CGDebugInfo.cpp
index 5772c07154144..5d82717de2910 100644
--- a/clang/lib/CodeGen/CGDebugInfo.cpp
+++ b/clang/lib/CodeGen/CGDebugInfo.cpp
@@ -1353,6 +1353,7 @@ CGDebugInfo::getOrCreateRecordFwdDecl(const RecordType *Ty,
// Don't include a linkage name in line tables only.
if (CGM.getCodeGenOpts().hasReducedDebugInfo())
Identifier = getTypeIdentifier(Ty, CGM, TheCU);
+ Ctx = PickCompositeTypeScope(Ctx, Identifier);
llvm::DICompositeType *RetTy = DBuilder.createReplaceableCompositeType(
getTagForRecord(RD), RDName, Ctx, DefUnit, Line, 0, Size, Align, Flags,
Identifier);
@@ -3718,6 +3719,7 @@ llvm::DIType *CGDebugInfo::CreateEnumType(const EnumType *Ty) {
// FwdDecl with the second and then replace the second with
// complete type.
llvm::DIScope *EDContext = getDeclContextDescriptor(ED);
+ EDContext = PickCompositeTypeScope(EDContext, Identifier);
llvm::DIFile *DefUnit = getOrCreateFile(ED->getLocation());
llvm::TempDIScope TmpContext(DBuilder.createReplaceableCompositeType(
llvm::dwarf::DW_TAG_enumeration_type, "", TheCU, DefUnit, 0));
@@ -3765,7 +3767,7 @@ llvm::DIType *CGDebugInfo::CreateTypeDefinition(const EnumType *Ty) {
llvm::DIFile *DefUnit = getOrCreateFile(ED->getLocation());
unsigned Line = getLineNumber(ED->getLocation());
- llvm::DIScope *EnumContext = getDeclContextDescriptor(ED);
+ llvm::DIScope *EnumContext = PickCompositeTypeScope(getDeclContextDescriptor(ED), Identifier);
llvm::DIType *ClassTy = getOrCreateType(ED->getIntegerType(), DefUnit);
return DBuilder.createEnumerationType(
EnumContext, ED->getName(), DefUnit, Line, Size, Align, EltArray, ClassTy,
@@ -4097,6 +4099,57 @@ CGDebugInfo::getOrCreateLimitedType(const RecordType *Ty) {
return Res;
}
+llvm::DIScope *CGDebugInfo::PickCompositeTypeScope(llvm::DIScope *S,
+ StringRef Identifier) {
+ using llvm::DISubprogram;
+
+ // Only adjust the scope for composite types placed into functions.
+ if (!isa<DISubprogram>(S))
+ return S;
+
+ // We must adjust the scope if the ODR-name of the type is set.
+ if (Identifier.empty())
+ return S;
+
+ // This type has an ODR-name, and might be de-duplicated during LTO. It needs
+ // to be placed in the unique declaration of the function, not a (potentially
+ // duplicated) definition.
+ DISubprogram *SP = cast<DISubprogram>(S);
+ if (DISubprogram *Decl = SP->getDeclaration())
+ return Decl;
+
+ // There is no declaration -- we must produce one and retrofit it to the
+ // existing definition. Assume that we can just harvest the existing
+ // information, clear the definition flag and set as decl.
+ DISubprogram::DISPFlags SPFlags = SP->getSPFlags();
+ SPFlags &= ~DISubprogram::SPFlagDefinition;
+
+ llvm::DINode::DIFlags Flags = SP->getFlags();
+ Flags &= ~llvm::DINode::FlagAllCallsDescribed;
+
+#ifdef EXPENSIVE_CHECKS
+ // If we're looking to be really rigorous and avoid a hard-to-debug mishap,
+ // make sure that there aren't any function definitions in the scope chain.
+ llvm::DIScope *ToCheck = SP->getScope();
+ do {
+ // We should terminate at a DIFile rather than a DICompileUnit -- we're
+ // not fully unique across LTO otherwise.
+ assert(!isa<llvm::DICompileUnit>(ToCheck));
+ if (auto *DISP = dyn_cast<DISubprogram>(ToCheck))
+ assert(!(DISP->getSPFlags() & DISubprogram::SPFlagDefinition));
+ ToCheck = ToCheck->getScope();
+ } while (ToCheck);
+#endif
+
+ DISubprogram *DeclSP = DBuilder.createFunction(
+ SP->getScope(), SP->getName(), SP->getLinkageName(), SP->getFile(),
+ SP->getLine(), SP->getType(), SP->getScopeLine(), Flags, SPFlags,
+ SP->getTemplateParams(), nullptr, nullptr, SP->getAnnotations());
+
+ SP->replaceDeclaration(DeclSP);
+ return DeclSP;
+}
+
// TODO: Currently used for context chains when limiting debug info.
llvm::DICompositeType *CGDebugInfo::CreateLimitedType(const RecordType *Ty) {
RecordDecl *RD = Ty->getDecl();
@@ -4134,6 +4187,7 @@ llvm::DICompositeType *CGDebugInfo::CreateLimitedType(const RecordType *Ty) {
auto Align = getTypeAlignIfRequired(Ty, CGM.getContext());
SmallString<256> Identifier = getTypeIdentifier(Ty, CGM, TheCU);
+ RDContext = PickCompositeTypeScope(RDContext, Identifier);
// Explicitly record the calling convention and export symbols for C++
// records.
diff --git a/clang/lib/CodeGen/CGDebugInfo.h b/clang/lib/CodeGen/CGDebugInfo.h
index 855881744237c..fc51cea5bc1f6 100644
--- a/clang/lib/CodeGen/CGDebugInfo.h
+++ b/clang/lib/CodeGen/CGDebugInfo.h
@@ -501,6 +501,12 @@ class CGDebugInfo {
void EmitFunctionDecl(GlobalDecl GD, SourceLocation Loc,
QualType FnType, llvm::Function *Fn = nullptr);
+ /// Select the appropriate scope for a composite type, redirecting certain
+ /// types into declaration DISubprograms rather than definition DISubprograms.
+ /// This avoids certain types that LLVM can unique based on their name being
+ /// put in a distinct-storage context.
+ llvm::DIScope *PickCompositeTypeScope(llvm::DIScope *S, StringRef Identifier);
+
/// Emit debug info for an extern function being called.
/// This is needed for call site debug info.
void EmitFuncDeclForCallSite(llvm::CallBase *CallOrInvoke,
diff --git a/clang/test/CodeGenCXX/debug-info-local-types.cpp b/clang/test/CodeGenCXX/debug-info-local-types.cpp
new file mode 100644
index 0000000000000..5f3a0efa7526d
--- /dev/null
+++ b/clang/test/CodeGenCXX/debug-info-local-types.cpp
@@ -0,0 +1,79 @@
+// RUN: %clang_cc1 -triple %itanium_abi_triple %s -o - -O0 -emit-llvm \
+// RUN: -disable-llvm-passes -debug-info-kind=limited | FileCheck %s
+//
+// Test that types declared inside functions, that receive an "identifier"
+// field used for ODR-uniquing, are placed inside the declaration DISubprogram
+// for the function rather than the definition DISubprogram. This avoids
+// later problems with distinct types in distinct DISubprograms being
+// inadvertantly unique'd; see github PR 75385.
+//
+// NB: The types below are marked distinct, but other portions of LLVM
+// force-unique them at a later date, see the enableDebugTypeODRUniquing
+// feature. Clang doesn't enable that itself; instead this test ensures a safe
+// representation of the types is produced.
+//
+// The check-lines below are not strictly in order of hierachy, so here's a
+// diagram of what's desired:
+//
+// DIFile
+// |
+// Decl-DISubprogram "foo"
+// / \
+// / \
+// Def-DISubprogram "foo" DICompositeType "bar"
+// |
+// |
+// Decl-DISubprogram "get_a"
+// / |
+// / |
+// Def-DISubprogram "get_a" DICompositeType "baz"
+// |
+// |
+// {Def,Decl}-DISubprogram "get_b"
+
+// CHECK: ![[FILENUM:[0-9]+]] = !DIFile(filename: "{{.*}}debug-info-local-types.cpp",
+
+// CHECK: ![[BARSTRUCT:[0-9]+]] = distinct !DICompositeType(tag: DW_TAG_class_type, name: "bar", scope: ![[FOOFUNC:[0-9]+]], file: ![[FILENUM]],
+// CHECK-SAME: identifier: "_ZTSZ3foovE3bar")
+
+// CHECK: ![[FOOFUNC]] = !DISubprogram(name: "foo", linkageName: "_Z3foov", scope: ![[FILENUM]], file: ![[FILENUM]],
+//// Test to ensure that this is _not_ a definition, therefore a decl.
+// CHECK-SAME: spFlags: 0)
+
+// CHECK: ![[GETADECL:[0-9]+]] = !DISubprogram(name: "get_a", scope: ![[BARSTRUCT]], file: ![[FILENUM]],
+//// Test to ensure that this is _not_ a definition, therefore a decl.
+// CHECK-SAME: spFlags: 0)
+
+// CHECK: ![[BAZSTRUCT:[0-9]+]] = distinct !DICompositeType(tag: DW_TAG_class_type, name: "baz", scope: ![[GETADECL]], file: ![[FILENUM]],
+// CHECK-SAME: identifier: "_ZTSZZ3foovEN3bar5get_aEvE3baz")
+// CHECK: distinct !DISubprogram(name: "get_b",
+// CHECK-SAME: scope: ![[BAZSTRUCT]], file: ![[FILENUM]],
+
+inline int foo() {
+ class bar {
+ private:
+ int a = 0;
+ public:
+ int get_a() {
+ class baz {
+ private:
+ int b = 0;
+ public:
+ int get_b() {
+ return b;
+ }
+ };
+
+ static baz xyzzy;
+ return a + xyzzy.get_b();
+ }
+ };
+
+ static bar baz;
+ return baz.get_a();
+}
+
+int a() {
+ return foo();
+}
+
diff --git a/llvm/test/DebugInfo/local-odr-types-hierarchy.ll b/llvm/test/DebugInfo/local-odr-types-hierarchy.ll
new file mode 100644
index 0000000000000..3c37816b68650
--- /dev/null
+++ b/llvm/test/DebugInfo/local-odr-types-hierarchy.ll
@@ -0,0 +1,124 @@
+; RUN: opt %s -o - -S | FileCheck %s
+
+; Paired with clang/test/CodeGenCXX/debug-info-local-types.cpp, this round-trips
+; debug-info metadata for types that are ODR-uniqued, to ensure that the type
+; hierachy does not change. See the enableDebugTypeODRUniquing feature: types
+; that have the "identifier" field set will be unique'd based on their name,
+; even if the "distinct" flag is set. Clang doesn't enable that itself, but opt
+; does, therefore we pass the metadata through opt to check it doesn't change
+; the type hiearchy.
+;
+; The check-lines below are not strictly in order of hierachy, so here's a
+; diagram of what's desired:
+;
+; DIFile
+; |
+; Decl-DISubprogram "foo"
+; / \
+; / \
+; Def-DISubprogram "foo" DICompositeType "bar"
+; |
+; |
+; Decl-DISubprogram "get_a"
+; / |
+; / |
+; Def-DISubprogram "get_a" DICompositeType "baz"
+; |
+; |
+; {Def,Decl}-DISubprogram "get_b"
+;
+; The declaration DISubprograms are unique'd, and the DICompositeTypes should
+; be in those scopes rather than the definition DISubprograms.
+
+; CHECK: ![[FILENUM:[0-9]+]] = !DIFile(filename: "{{.*}}debug-info-local-types.cpp",
+
+; CHECK: ![[BARSTRUCT:[0-9]+]] = distinct !DICompositeType(tag: DW_TAG_class_type, name: "bar", scope: ![[FOOFUNC:[0-9]+]], file: ![[FILENUM]],
+; CHECK-SAME: identifier: "_ZTSZ3foovE3bar")
+
+; CHECK: ![[FOOFUNC]] = !DISubprogram(name: "foo", linkageName: "_Z3foov", scope: ![[FILENUM]], file: ![[FILENUM]],
+;; Test to ensure that this is _not_ a definition, therefore a decl.
+; CHECK-SAME: spFlags: 0)
+
+; CHECK: ![[GETADECL:[0-9]+]] = !DISubprogram(name: "get_a", scope: ![[BARSTRUCT]], file: ![[FILENUM]],
+;; Test to ensure that this is _not_ a definition, therefore a decl.
+; CHECK-SAME: spFlags: 0)
+
+; CHECK: ![[BAZSTRUCT:[0-9]+]] = distinct !DICompositeType(tag: DW_TAG_class_type, name: "baz", scope: ![[GETADECL]], file: ![[FILENUM]],
+; CHECK-SAME: identifier: "_ZTSZZ3foovEN3bar5get_aEvE3baz")
+; CHECK: distinct !DISubprogram(name: "get_b",
+; CHECK-SAME: scope: ![[BAZSTRUCT]], file: ![[FILENUM]],
+
+%class.bar = type { i32 }
+%class.baz = type { i32 }
+
+$_Z3foov = comdat any
+
+$_ZZ3foovEN3bar5get_aEv = comdat any
+
+$_ZZZ3foovEN3bar5get_aEvEN3baz5get_bEv = comdat any
+
+$_ZZ3foovE3baz = comdat any
+
+$_ZZZ3foovEN3bar5get_aEvE5xyzzy = comdat any
+
+@_ZZ3foovE3baz = linkonce_odr global %class.bar zeroinitializer, comdat, align 4, !dbg !0
+@_ZZZ3foovEN3bar5get_aEvE5xyzzy = linkonce_odr global %class.baz zeroinitializer, comdat, align 4, !dbg !10
+
+define dso_local noundef i32 @_Z1av() !dbg !32 {
+entry:
+ unreachable
+}
+
+define linkonce_odr noundef i32 @_Z3foov() comdat !dbg !2 {
+entry:
+ unreachable
+}
+
+define linkonce_odr noundef i32 @_ZZ3foovEN3bar5get_aEv(ptr noundef nonnull align 4 dereferenceable(4) %this) comdat align 2 !dbg !12 {
+entry:
+ unreachable
+}
+
+define linkonce_odr noundef i32 @_ZZZ3foovEN3bar5get_aEvEN3baz5get_bEv(ptr noundef nonnull align 4 dereferenceable(4) %this) comdat align 2 !dbg !33 {
+entry:
+ unreachable
+}
+
+!llvm.dbg.cu = !{!7}
+!llvm.module.flags = !{!29, !30}
+!llvm.ident = !{!31}
+
+!0 = !DIGlobalVariableExpression(var: !1, expr: !DIExpression())
+!1 = distinct !DIGlobalVariable(name: "baz", scope: !2, file: !3, line: 71, type: !13, isLocal: false, isDefinition: true)
+!2 = distinct !DISubprogram(name: "foo", linkageName: "_Z3foov", scope: !3, file: !3, line: 51, type: !4, scopeLine: 51, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !7, declaration: !14)
+!3 = !DIFile(filename: "debug-info-local-types.cpp", directory: ".")
+!4 = !DISubroutineType(types: !5)
+!5 = !{!6}
+!6 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!7 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus_14, file: !8, producer: "clang", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, globals: !9, splitDebugInlining: false, nameTableKind: None)
+!8 = !DIFile(filename: "<stdin>", directory: ".")
+!9 = !{!0, !10}
+!10 = !DIGlobalVariableExpression(var: !11, expr: !DIExpression())
+!11 = distinct !DIGlobalVariable(name: "xyzzy", scope: !12, file: !3, line: 66, type: !22, isLocal: false, isDefinition: true)
+!12 = distinct !DISubprogram(name: "get_a", linkageName: "_ZZ3foovEN3bar5get_aEv", scope: !13, file: !3, line: 56, type: !18, scopeLine: 56, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !7, declaration: !17, retainedNodes: !21)
+!13 = distinct !DICompositeType(tag: DW_TAG_class_type, name: "bar", scope: !14, file: !3, line: 52, size: 32, flags: DIFlagTypePassByValue | DIFlagNonTrivial, elements: !15, identifier: "_ZTSZ3foovE3bar")
+!14 = !DISubprogram(name: "foo", linkageName: "_Z3foov", scope: !3, file: !3, line: 51, type: !4, scopeLine: 51, flags: DIFlagPrototyped, spFlags: 0)
+!15 = !{!16, !17}
+!16 = !DIDerivedType(tag: DW_TAG_member, name: "a", scope: !13, file: !3, line: 54, baseType: !6, size: 32)
+!17 = !DISubprogram(name: "get_a", scope: !13, file: !3, line: 56, type: !18, scopeLine: 56, flags: DIFlagPublic | DIFlagPrototyped, spFlags: 0)
+!18 = !DISubroutineType(types: !19)
+!19 = !{!6, !20}
+!20 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !13, size: 64, flags: DIFlagArtificial | DIFlagObjectPointer)
+!21 = !{}
+!22 = distinct !DICompositeType(tag: DW_TAG_class_type, name: "baz", scope: !17, file: !3, line: 57, size: 32, flags: DIFlagTypePassByValue | DIFlagNonTrivial, elements: !23, identifier: "_ZTSZZ3foovEN3bar5get_aEvE3baz")
+!23 = !{!24, !25}
+!24 = !DIDerivedType(tag: DW_TAG_member, name: "b", scope: !22, file: !3, line: 59, baseType: !6, size: 32)
+!25 = !DISubprogram(name: "get_b", scope: !22, file: !3, line: 61, type: !26, scopeLine: 61, flags: DIFlagPublic | DIFlagPrototyped, spFlags: 0)
+!26 = !DISubroutineType(types: !27)
+!27 = !{!6, !28}
+!28 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !22, size: 64, flags: DIFlagArtificial | DIFlagObjectPointer)
+!29 = !{i32 2, !"Debug Info Version", i32 3}
+!30 = !{i32 1, !"wchar_size", i32 4}
+!31 = !{!"clang"}
+!32 = distinct !DISubprogram(name: "a", linkageName: "_Z1av", scope: !3, file: !3, line: 75, type: !4, scopeLine: 75, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !7)
+!33 = distinct !DISubprogram(name: "get_b", linkageName: "_ZZZ3foovEN3bar5get_aEvEN3baz5get_bEv", scope: !22, file: !3, line: 61, type: !26, scopeLine: 61, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !7, declaration: !25, retainedNodes: !21)
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
Gentle ping |
|
I know this scoping local non-instance specific entities work has/continues to be a long, arduous, journey... but I'm not sure this particular step is right. I /think/ function-local types need to go in definitions - in /abstract/ definitions if they exist (not in the inlined instances or concrete out of line instances). Because function local entities may need to go inside scopes inside functions - and declarations don't have scopes, definitions do. Then those abstract scopes can be referenced from concrete/inlined scopes to attach the specific instruction ranges to them, over which the local name will be valid. Does this make sense? Is there some alternative? |
|
(Transparency, I wrote some of the code in the PR so dunno if I should review it,)
I think this highlights the difference in my thinking -- I've been treating "declaration" DISubprograms like they're actually "abstract" DISubprograms (they're very similar in my mind). As far as I'm aware we don't have any entities in LLVM-IR metadata to represent abstract DISubprograms, and from |
goes to see what the DWARF for inlining looks like so inlined instances refer to the same distinct DISubprogram as a function definition at the moment. How does this work for LTO? (we'd want all inlined instances to refer to a singular abstract definition) Yeah, looks like that's broken (the abstract origins are not shared - they're duplicated in every translation unit). We should fix that. We have mangled names on functions like we do for types, so we could use the same approach to deduplicate them via mangled name as a key? (for types this mangled name field was introduced specifically as a deduplication key (I think initially for type units, then for LTO merging, but maybe it was the other way around) - currently the linkage name for functions isn't a load bearing key, but it could become one, or perhaps needs to be a distinct key compared to the mangled name - in the case of internal functions/types they might have a mangled name, but not allow deduplication (for internal linkage types, we omit the mangled name/deduplication key)) If that's an infeasibly large project at this time and we want to take on some technical debt to address the ODR type in a non-uniqued entity issue, yeah, guess we could put them on the declaration - but it does feel like it's going to be awkward/painful so long as that sticks around & we'd want to fix the other thing (shared abstract origins in LTO) anyway, probably? |
But the notion of "abstract" subprogram pops up only during DWARF generation, doesn’t it?
It seems something similar is done for declarations of composite type member subprograms, according to https://llvm.org/docs/LangRef.html#disubprogramdeclaration :
|
I think it might be useful to think of it that we only have abstract subprograms - concrete and inlined subprograms exist as a consequence of the instructions/locations in the llvm functions that define them. (long time ago we had subprograms that referred to functions (which became problematic in LTO - with multiple subprograms referring to the same deduplicated function, or optimized out functions still having lingering subprograms), so we inverted it - functions refer to their subprogram - at which point I think we sort of created the lazily-referenced "abstract" subprogram by construction, it can be referred to by both/either the concrete function, or the inlined instructions - it seems to me it is an abstract/shared description across both of those cases)
Oh, I think that was added for call-site declarations which are deduplicated correctly/not emitted as unique, etc.... so they're probably uniqued with each other, but not with the unique definitions from other translation units. So maybe the solution is as simple as removing the "unique" from DISubprogram definitions, and the existing infrastructure will handle it? (maybe have to expand it to allow it to work for things with DISPFlagDefinition... ) (I'm a bit confused about some of that spec wording though - DIFwdDecl is /not/ set but also DISPFlagDefinition is not present... (a: why do we have both of those, they seem redundant with each other, b: both being not-present seems contradictory)) |
If I understand the idea behind #119001 right, the problem that caused the crashes after #75385 was the case of two distinct DISubporgrams having a local type that gets uniqued during LTO, and these parent subprograms both retain the reference to the same local type (It was described here #75385 (comment)). So I was thinking in the opposite direction: should we force unique distinct definition DISubporgrams containing ODR-uniqued types, as well as it's done for declaration subprograms contained by ODR-uniqued types? |
Yep, I think that's the original thing.
I'm suggesting more broadly: we should force-unique all DISubprograms (even if they don't contain odr-uniqued types), by making them not distinct anymore & generalizing the unquing infrastructure you quoted in langref. This should address a broader class of problems - including, for instance, LTO where a subprogram is inlined/out of line in multiple files. |
This commit was split from #119001 commit chain after the discussion with @jmorse. It can be merged before the fixed version of #75385, which will help to track down issues if they pop up after the merge.
The original commit message follows:
There are two flavours of DISubprogram: declarations, which are unique'd and abstractly describe the function in question. There are also definition DISubprograms which correspond to real instances, some of which are inlined, duplicated across translation units in LTO, or otherwise can have multiple instances.
Given that LLVM sometimes force-uniques types by their ODR-name, see the enableDebugTypeODRUniquing feature, we shouldn't place types that might be unique'd into duplicated contexts like definition DISubprograms. Instead, place them into the declaration.
This slightly bends the existing approach where only functions that have a separate declaratrion to their definition get a declaration-DISubprogram. A single function in a translation unit might now get a declaration where it didn't before, if it contains an ODR-unique'd type declaration. This seems reasonable given that the LLVM idea of a declaration doesn't have to exactly match source-language ideas.
The added cpp test checks that such ORD-unique'd types are detected and placed in the declaration DISubprogram, creating one if necessary. The IR test ensures that nothing changes after a round-trip with enableDebugTypeODRUniquing turned on.