[PATCH] D129748: [Modules] Disable preferred_name attribute in C++20 Modules

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D129748#3653897 <https://reviews.llvm.org/D129748#3653897>, @ChuanqiXu wrote:

> In D129748#3651771 <https://reviews.llvm.org/D129748#3651771>, @erichkeane wrote:
>
>> I guess I don't have a good idea why this attribute would cause ODR issues?  It would seem that if it appeared in 2 different TUs that we could just 'pick' whichever we wanted, right?
>
> If the compiler finds it appeared in 2 different TUs with different definition (although the judgement is wrong), the compiler would emit an error. So it would block the uses of C++20 Modules with `preferred_name`.
>
>> The description in the bug report of the problem isn't clear to me what the actual issue is.
>
> Sorry. My bad. Let me try to clarify it. When we write the attribute `preferred_name(foo)` in ASTWriter, the compiler would try to write the type for the argument `foo`. Then when the compiler write the type for `foo`, the compiler find the type for `foo` is a TypeDef type. So the compiler would write the corresponding type `foo_templ<char>`. The key point here is that the AST for `foo_templ<char>` is complete now. Since the AST for `foo_templ<char>` is constructed in Sema.
>
> But problem comes when we read it. When we read the attribute `preferred_name(foo)`, we would read the type for the argument `foo` and then we would try to read the type `foo_templ<char>` later. However, the key problem here is that when we read `foo_templ<char>`, its AST is not constructed yet! So we get a different type with the writer writes. So here is the ODR violation.
>
> The problem is fundamental and I've spent 2 weeks on it. But I don't find any fixes for it. Then I found that, once I disabled `preferred_name`, we could go much further. So I am wondering if it would be an option to skip `preferred_name` if C++ modules is enabled. The idea may not be good in general. But I feel it might be an option in this specific case given it is hard to solve and `preferred_name` is primarily for printers.

Hmm... interesting.  I wish I had a better understanding of the ASTWriter to help with this, but given:

1- Getting modules compiled is important
2- this attribute is 'ignore-able', and is for diagnostics only
3- the ASTWriter/ASTReader seems to be messing this up.

I think there _IS_ perhaps an acceptability to ignoring this attribute when it would cause a problem.  However, I think doing it as you're doing it now is wrong.  IF we are going to solve the symptom here, I think we should use a much more precise cut, and make either ASTReader not emit the attribute, or ASTWriter just 'ignore' it when reading.  WDYT?  @aaron.ballman as well...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129748