This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by DianQK on Jun 4 2023, 7:17 AM.

Details

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.

Diff Detail

Event Timeline

DianQK created this revision.Jun 4 2023, 7:17 AM
Herald added a project: Restricted Project. · View Herald Transcript
DianQK requested review of this revision.Jun 4 2023, 7:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 4 2023, 7:17 AM
DianQK edited the summary of this revision. (Show Details)Jun 4 2023, 7:20 AM
DianQK added reviewers: dblaikie, ellis, aprantl.
DianQK added a project: debug-info.

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 added a comment.Jun 6 2023, 6:01 PM

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?

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 updated this revision to Diff 530498.Jun 12 2023, 7:15 AM

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 retitled this revision from DWARFVerifier: Verifies that definition subprograms cannot be nested within structures such as classes and structs to [Verifier] definition subprograms cannot be nested within DICompositeType when enabling ODR..Jun 12 2023, 7:15 AM
DianQK edited the summary of this revision. (Show Details)
DianQK edited the summary of this revision. (Show Details)
DianQK updated this revision to Diff 533598.Jun 22 2023, 7:29 AM
DianQK edited the summary of this revision. (Show Details)

Rebase.

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.

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?

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.

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

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 added a subscriber: cuviper.Jul 1 2023, 5:58 AM

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.

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 updated this revision to Diff 538139.Jul 7 2023, 7:32 AM

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

DianQK added a comment.Jul 7 2023, 7:46 AM

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 accepted this revision.Jul 24 2023, 7:58 PM

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

This revision is now accepted and ready to land.Jul 24 2023, 7:58 PM
DianQK updated this revision to Diff 543957.Jul 25 2023, 6:59 AM

Update code comments.

DianQK marked an inline comment as done.EditedJul 25 2023, 7:22 AM

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.

DianQK edited the summary of this revision. (Show Details)Jul 25 2023, 3:05 PM
This revision was landed with ongoing or failed builds.Jul 25 2023, 3:08 PM
This revision was automatically updated to reflect the committed changes.

Awesome - thanks!