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

Mon Jul 24 19:58:29 PDT 2023

dblaikie accepted this revision.
dblaikie added a comment.
This revision is now accepted and ready to land.

In D152095#4480908 <https://reviews.llvm.org/D152095#4480908>, @DianQK wrote:

> In D152095#4474741 <https://reviews.llvm.org/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.

================
Comment at: llvm/lib/IR/Verifier.cpp:1415-1416
+    // 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());
----------------
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"?

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152095/new/

https://reviews.llvm.org/D152095