[PATCH] D115353: Verifier: accept enums as scopes

[David Blaikie via Phabricator via llvm-commits]

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

In D115353#3183991 <https://reviews.llvm.org/D115353#3183991>, @ychen wrote:

> In D115353#3183911 <https://reviews.llvm.org/D115353#3183911>, @dblaikie wrote:
>
>> In D115353#3183820 <https://reviews.llvm.org/D115353#3183820>, @ychen wrote:
>>
>>> In D115353#3183713 <https://reviews.llvm.org/D115353#3183713>, @dblaikie wrote:
>>>
>>>> In D115353#3179986 <https://reviews.llvm.org/D115353#3179986>, @ychen wrote:
>>>>
>>>>> Oops, apologies for breaking Rust. I'm no debuginfo expert, just wondering if we could keep this check for C/C++?
>>>>
>>>> Reckon it's worth the special case, though? I don't feel super strongly either way - I guess we already do have the "CurrentSourceLang" there and use it for some fortran special case, so we could test it here too. But I'm not sure how valuable it is?
>>>>
>>>> The original bug/issue should be caught during IR merging instead - and since the goal is to support the case of member functions in enums, it's not like the LLVM codegen can't handle this case (when it's C++ or any other language).
>>>>
>>>> (I assume the verifier check was added originally because members in enums broke LLVM in some way? But that bug has been/is being fixed so Rust keeps working? Or did it never break LLVM but instead just produce "weird" DWARF?)
>>>
>>> It didn't break LLVM, just the debug IR does not make sense for C++. I was suggesting the "CurrentSourceLang" method so there is no need to think about if the issue may happen in any other way than IR merging. IMHO, it is valuable than losing a few lines of code. Since it is cheap enough to keep and maintain, I'd prefer to keep the check.
>>
>> I guess the counterargument might be that there's probably lots of invariants for what constitutes valid/reasonable DWARF for C++ - but we don't comprehensively try to check for all those things, why would we check for this thing in particular?
>>
>> Generally the IR verifier is more about: "If the IR is verified it won't crash LLVM/will produce valid DWARF"
>
> Yeah, agreed that this is a rare special casing for C++, if it is preferred to be avoided, I think it makes sense to remove the check. Feel free to accept the patch assuming it looks good to you. Thanks for chiming in :-).

Fair - I appreciate the discussion, but think I'll lean towards not including these checks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115353