[Verifier] definition subprograms cannot be nested within DICompositeType when enabling ODR.

[DianQK]

Summary

Resolve https://github.com/llvm/llvm-project/issues/61932. We should add the validation.

LLVM can't handle IR where subprogram definitions are nested within DICompositeType when doing LTO builds, because there's no good way to cross the CU boundary to insert a nested DISubprogram definition in one CU into a type defined in another CU.

The test cases cross-cu-inlining-2.ll and cross-cu-inlining-ranges.ll can be deleted. In the cross-cu-inlining-2.ll, the low pc and high pc of the CU are also incorrect.

[dblaikie]

Oh, sorry - I think I might've miscommunicated. My thinking was that this should be part of LLVM's IR verifier - not the DWARF binary verifier. This limitation is LLVM's, not DWARF's. (DWARF can describe member definitions inside type definitions, but LLVM can't handle IR that describes things that way when doing LTO builds because there's no good way to cross the CU boundary while preserving the context of where a given member was defined)

[DianQK]

This limitation is LLVM's, not DWARF's. (DWARF can describe member definitions inside type definitions, but LLVM can't handle IR that describes things that way when doing LTO builds because there's no good way to cross the CU boundary while preserving the context of where a given member was defined)

I did consider whether to put this verification in IR or DWARF, I just noticed that this type of verification is written in llvm-dwarfdump. Thank you for explaining, it's more appropriate to put it in IR. Could you explain the issues related to subroutines?

[dblaikie]

Could you explain the issues related to subroutines?

So the issue is that member functions might be defined in multiple CUs (and that might be important that they be defined in the correct CU - if, eg, that member function definition calls into a file-static function that's emitted into the suitable CU, so name lookup should find the CU-specific context like that) - but we can't simultaneously respect those two requirements in an LTO build, without duplicating the type into multiple CUs. Whereas the out of line member function definition can be in one CU, with the type definition (containing a member function declaration) can be in another CU and a cross-CU reference can be used to reference the member function declaration from the member funciton definition.

We /could/ do this by emitting a type /declaration/ into the CU with the other member function definition in an LTO build, but that seems a bit ugly, then requiring debuggers to stitch together these various descriptions of the type.

Also, I'm not sure what this metadata would do to DWARF type units (the type description would no longer be canonical, and we'd end up trying to emit a definition into the type in the type unit, which wouldn't be right).

[DianQK]

What I would like to ask is about the checks related to inlined subroutines in the issue. How are they different from the checks for definition subprograms? Based on your previous explanation and my current understanding, it seems that checking DISubprogram should be sufficient.

In the current context, I think I can understand that "subroutine" and "subprogram" are the same term/semantics.

[DianQK]

Rebase.

[dblaikie]

In D152095#4413788, @DianQK wrote:

What I would like to ask is about the checks related to inlined subroutines in the issue. How are they different from the checks for definition subprograms? Based on your previous explanation and my current understanding, it seems that checking DISubprogram should be sufficient.

In the current context, I think I can understand that "subroutine" and "subprogram" are the same term/semantics.

Ah, sorry - yeah, I might've used "subroutine" and "subprogram" somewhat casually/interchangeably in my descriptions. It's weird those things are named differently in the DWARF spec anyway, but...

The basic idea is that if a member function is defined inside the type, we can't properly reflect that if the type is in another CU - so for cross-CU/LTO purposes, all member functions need to be indirected through a declaration in the type and otherwise scoped to something that can appear outside the type definition - like to the root or to a namespace, that can be duplicated/produced in whatever CU is required for the definition of the function.

[dblaikie]

This has LLVM changes in it? What's the purpose of those changes?

llvm/test/DebugInfo/Generic/cross-cu-inlining-ranges.ll

50 ↗

(On Diff #533598)

This looks a bit confused - it's claiming to be a definition, but it's not referenced by any llvm Function - and then it's referencing a declaration? Perhaps these functions should not have the definition flag?

@aprantl @JDevlieghere what's the Swift IR look like today? Does it put subprogram definitions within composite types? That's not ideal/bit likely to break under LTO builds & would you be amenable to changing that to indirect through a definition that's outside the composite type to a declaration in the composite type?

[DianQK]

The modifications made this time still involve manually adding declaration references. I just wanted to demonstrate that these test cases work after removing the previous code and adding the declaration. I can remove these two test cases in the next commit or remove them in this current commit.

llvm/test/DebugInfo/Generic/cross-cu-inlining-ranges.ll

50 ↗

(On Diff #533598)

I deleted this piece of code earlier in order to create the smallest possible test cases.

In the https://swift.godbolt.org/z/7Yoqa7qnx, the init method does exist.
From the provided link, the swiftc still puts subprogram definitions within composite types.

[dblaikie]

In D152095#4452275, @DianQK wrote:

The modifications made this time still involve manually adding declaration references. I just wanted to demonstrate that these test cases work after removing the previous code and adding the declaration. I can remove these two test cases in the next commit or remove them in this current commit.

Not sure I follow - I guess regardless they probably shouldn't be in this change.

I'd expect this change to add the verifier check, and add an example IR that doesn't pass verification/demonstrates the new verifier check correctly catches the intended case(s).

@aprantl @JDevlieghere Sounds like this verifier check might fail for Swift debug info IR? You folks have any thoughts on that/how to address it/if anything needs to be done before we enable this check? (and how Swift is working with LTO if it generates IR like this... ?)

[DianQK]

In D152095#4457017, @dblaikie wrote:

Not sure I follow - I guess regardless they probably shouldn't be in this change.

I want to remove these two IRs from this change. Since these violate the current convention, these don't make sense anymore.

I'd expect this change to add the verifier check, and add an example IR that doesn't pass verification/demonstrates the new verifier check correctly catches the intended case(s).

This should be llvm/test/Verifier/disubprogram-declaration-within-struct.ll?

[DianQK]

I tried adding a patch to Swift based on the Rust implementation. I don't know if the condition for determining whether a function is a method is reasonable, but everything seems to be working fine.
Thanks very much for @cuviper's implementation.

[dblaikie]

In D152095#4459678, @DianQK wrote:

In D152095#4457017, @dblaikie wrote:

Not sure I follow - I guess regardless they probably shouldn't be in this change.

I want to remove these two IRs from this change. Since these violate the current convention, these don't make sense anymore.

I'm still not quite following why this patch has changes to llvm/lib/CodeGen/AsmPrinter? Is the point that those changes can be removed once this invariant is introduced? If so, those changes should probably be in a follow-up after this one, rather than at the same time.

I'd expect this change to add the verifier check, and add an example IR that doesn't pass verification/demonstrates the new verifier check correctly catches the intended case(s).

This should be llvm/test/Verifier/disubprogram-declaration-within-struct.ll?

Yep, that looks OK. Though I don't understand the phrasing "when enabling ODR" - what does that refer to? (is there some feature to "enable/disable ODR" that's relevant here?)

[DianQK]

Remove changes that are not directly related to this patch.
Improve the description and handling of the ODR.

[DianQK]

In D152095#4474741, @dblaikie wrote:

I'm still not quite following why this patch has changes to llvm/lib/CodeGen/AsmPrinter? Is the point that those changes can be removed once this invariant is introduced? If so, those changes should probably be in a follow-up after this one, rather than at the same time.

I have removed these changes. I could push these changes directly after the current patch. If a review is still required, please let me know.

I'd expect this change to add the verifier check, and add an example IR that doesn't pass verification/demonstrates the new verifier check correctly catches the intended case(s).

This should be llvm/test/Verifier/disubprogram-declaration-within-struct.ll?

Yep, that looks OK. Though I don't understand the phrasing "when enabling ODR" - what does that refer to? (is there some feature to "enable/disable ODR" that's relevant here?)

We can disable ODR with disableDebugTypeODRUniquing, or with the command line parameter --disable-debug-info-type-map.
When ODR is disabled or DICompositeType without an identifier, llvm-link will create two DICompositeType in different CU. In this case, LTO works fine.

The explanation I found from ODR in LangRef is the One Definition Rule.

[dblaikie]

In D152095#4480908, @DianQK wrote:

In D152095#4474741, @dblaikie wrote:

I'm still not quite following why this patch has changes to llvm/lib/CodeGen/AsmPrinter? Is the point that those changes can be removed once this invariant is introduced? If so, those changes should probably be in a follow-up after this one, rather than at the same time.

I have removed these changes. I could push these changes directly after the current patch. If a review is still required, please let me know.

OK, sounds good! Yeah, if those revert previous patches that are no longer required after this invariant is introduced, no need for further review - if you can mark them as reverts of the patches that introduced them (even if the revert doesn't apply perfectly cleanly/requires some cleanup to apply) it'll help keep track of everything - be good to mention the original commits D*** numbers in this review and/or mention this review in the reverts, etc, so we can piece the whole story together.

I'd expect this change to add the verifier check, and add an example IR that doesn't pass verification/demonstrates the new verifier check correctly catches the intended case(s).

This should be llvm/test/Verifier/disubprogram-declaration-within-struct.ll?

Yep, that looks OK. Though I don't understand the phrasing "when enabling ODR" - what does that refer to? (is there some feature to "enable/disable ODR" that's relevant here?)

We can disable ODR with disableDebugTypeODRUniquing, or with the command line parameter --disable-debug-info-type-map.
When ODR is disabled or DICompositeType without an identifier, llvm-link will create two DICompositeType in different CU. In this case, LTO works fine.

Ah, good to know.

SGTM.

llvm/lib/IR/Verifier.cpp

1415–1416

Perhaps "there's no good way to cross the CU boundary to insert a nested DISubprogram definition in one CU into a type defined in another CU"?

[DianQK]

Update code comments.

[DianQK]

Thank you for your help and review.
Subsequent commits are https://github.com/llvm/llvm-project/commit/30f2170a78e7c67a9d870ba9615d0c4c472cedbf and https://github.com/llvm/llvm-project/commit/f2d307c4ede0c55b28bd366511907bda29703994.
I did not remove the code changes to D135114, which should make sense.

[dblaikie]

Awesome - thanks!

llvm/lib/IR/Verifier.cpp

CheckDI(!hasConflictingReferenceFlags(N.getFlags()),
           "invalid reference flags", &N);
 
   auto *Unit = N.getRawUnit();
   if (N.isDefinition()) {
     // Subprogram definitions (not part of the type hierarchy).
     CheckDI(N.isDistinct(), "subprogram definitions must be distinct", &N);
     CheckDI(Unit, "subprogram definitions must have a compile unit", &N);
     CheckDI(isa<DICompileUnit>(Unit), "invalid unit type", &N, Unit);
+    // When more than one module is imported into the same context, such as
+    // during an LTO build, there's no good way to cross the CU boundary for the
+    // DISubprogram that is nested within the ODR type.
+    auto *CT = dyn_cast_or_null<DICompositeType>(N.getRawScope());
+    if (CT && CT->getRawIdentifier() &&
+        M.getContext().isODRUniquingDebugTypes())
+      CheckDI(N.getDeclaration(),
+              "definition subprograms cannot be nested within DICompositeType "
+              "when enabling ODR",
+              &N);
     if (N.getFile())
       verifySourceDebugInfo(*N.getUnit(), *N.getFile());
   } else {
     // Subprogram declarations (part of the type hierarchy).
     CheckDI(!Unit, "subprogram declarations must not have a compile unit", &N);
     CheckDI(!N.getRawDeclaration(),
             "subprogram declaration must not have a declaration field");
   }

llvm/test/Verifier/disubprogram-declaration-within-struct-2.ll

+; RUN: opt -passes=verify -S < %s 2>&1 | FileCheck %s
+; RUN: opt -passes=verify -S --disable-debug-info-type-map < %s | FileCheck --check-prefix=CHECK-NOT-ODR %s
+
+; Same content as the disubprogram-declaration-within-struct.ll file, except the `identifier` for `DICompositeType` is removed.
+
+; ModuleID = 'foo.7e668e4a-cgu.0'
+source_filename = "foo.7e668e4a-cgu.0"
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+; foo::Foo::bar
+; Function Attrs: nonlazybind uwtable
+define void @_ZN3foo3Foo3bar17h32d65c44145019c8E() unnamed_addr #0 !dbg !6 {
+start:
+  ret void, !dbg !14
+}
+
+attributes #0 = { nonlazybind uwtable "probe-stack"="__rust_probestack" "target-cpu"="x86-64" }
+
+!llvm.module.flags = !{!0, !1, !2, !3}
+!llvm.dbg.cu = !{!4}
+
+!0 = !{i32 7, !"PIC Level", i32 2}
+!1 = !{i32 2, !"RtLibUseGOT", i32 1}
+!2 = !{i32 2, !"Dwarf Version", i32 4}
+!3 = !{i32 2, !"Debug Info Version", i32 3}
+!4 = distinct !DICompileUnit(language: DW_LANG_Rust, file: !5, producer: "clang LLVM (rustc version 1.69.0 (84c898d65 2023-04-16))", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, splitDebugInlining: false)
+!5 = !DIFile(filename: "foo.rs/@/foo.7e668e4a-cgu.0", directory: "/tmp")
+!6 = distinct !DISubprogram(name: "bar", linkageName: "_ZN3foo3Foo3bar17h32d65c44145019c8E", scope: !8, file: !7, line: 4, type: !12, scopeLine: 4, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !4, templateParams: !11, retainedNodes: !11)
+!7 = !DIFile(filename: "foo.rs", directory: "/tmp", checksumkind: CSK_MD5, checksum: "427d9b572596c2f310b6185a1781f222")
+!8 = !DICompositeType(tag: DW_TAG_structure_type, name: "Foo", scope: !10, file: !9, align: 8, elements: !11)
+!9 = !DIFile(filename: "<unknown>", directory: "")
+!10 = !DINamespace(name: "foo", scope: null)
+!11 = !{}
+!12 = !DISubroutineType(types: !13)
+!13 = !{null}
+!14 = !DILocation(line: 6, column: 6, scope: !6)
+
+; CHECK-NOT: definition subprograms cannot be nested within DICompositeType when enabling ODR
+; CHECK-NOT: warning: ignoring invalid debug info
+
+; CHECK-NOT-ODR-NOT: definition subprograms cannot be nested within DICompositeType when enabling ODR
+; CHECK-NOT-ODR-NOT: warning: ignoring invalid debug info

llvm/test/Verifier/disubprogram-declaration-within-struct.ll

+; RUN: opt -passes=verify -disable-output < %s 2>&1 | FileCheck %s
+; RUN: opt -passes=verify -S --disable-debug-info-type-map < %s | FileCheck --check-prefix=CHECK-NOT-ODR %s
+
+; The Rust source:
+; pub struct Foo;
+; impl Foo {
+;   pub fn bar() {}
+; }
+
+; ModuleID = 'foo.7e668e4a-cgu.0'
+source_filename = "foo.7e668e4a-cgu.0"
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+; foo::Foo::bar
+; Function Attrs: nonlazybind uwtable
+define void @_ZN3foo3Foo3bar17h32d65c44145019c8E() unnamed_addr #0 !dbg !6 {
+start:
+  ret void, !dbg !14
+}
+
+attributes #0 = { nonlazybind uwtable "probe-stack"="__rust_probestack" "target-cpu"="x86-64" }
+
+!llvm.module.flags = !{!0, !1, !2, !3}
+!llvm.dbg.cu = !{!4}
+
+!0 = !{i32 7, !"PIC Level", i32 2}
+!1 = !{i32 2, !"RtLibUseGOT", i32 1}
+!2 = !{i32 2, !"Dwarf Version", i32 4}
+!3 = !{i32 2, !"Debug Info Version", i32 3}
+!4 = distinct !DICompileUnit(language: DW_LANG_Rust, file: !5, producer: "clang LLVM (rustc version 1.69.0 (84c898d65 2023-04-16))", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, splitDebugInlining: false)
+!5 = !DIFile(filename: "foo.rs/@/foo.7e668e4a-cgu.0", directory: "/tmp")
+!6 = distinct !DISubprogram(name: "bar", linkageName: "_ZN3foo3Foo3bar17h32d65c44145019c8E", scope: !8, file: !7, line: 4, type: !12, scopeLine: 4, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !4, templateParams: !11, retainedNodes: !11)
+!7 = !DIFile(filename: "foo.rs", directory: "/tmp", checksumkind: CSK_MD5, checksum: "427d9b572596c2f310b6185a1781f222")
+!8 = !DICompositeType(tag: DW_TAG_structure_type, name: "Foo", scope: !10, file: !9, align: 8, elements: !11, identifier: "a8fd67db4e906c9c4aceea39cb8b0f61")
+!9 = !DIFile(filename: "<unknown>", directory: "")
+!10 = !DINamespace(name: "foo", scope: null)
+!11 = !{}
+!12 = !DISubroutineType(types: !13)
+!13 = !{null}
+!14 = !DILocation(line: 6, column: 6, scope: !6)
+
+; CHECK: definition subprograms cannot be nested within DICompositeType when enabling ODR
+; CHECK: warning: ignoring invalid debug info
+
+; CHECK-NOT-ODR-NOT: definition subprograms cannot be nested within DICompositeType when enabling ODR
+; CHECK-NOT-ODR-NOT: warning: ignoring invalid debug info