[PATCH] D130331: [C++20] [Modules] Disable preferred_name when writing a C++20 Module interface

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added a comment.

I guess this is probably ok as a short term fix for the Clang 15 release. It still makes me nervous though.



================
Comment at: clang/lib/Serialization/ASTWriter.cpp:4353
+  // https://github.com/llvm/llvm-project/issues/56490 for example.
+  if (!A || (isa<PreferredNameAttr>(A) && Writer->isWritingNamedModules()))
     return Record.push_back(0);
----------------
ChuanqiXu wrote:
> tahonermann wrote:
> > ChuanqiXu wrote:
> > > The `Writer->isWritingNamedModules()` part is necessary. Otherwise we would break the https://github.com/llvm/llvm-project/blob/main/clang/test/PCH/decl-attrs.cpp test. The reason why the bug is not found by the user of PCH or clang modules is that a header generally would be guarded by `#ifndef ... #define` fashion. And if we remove the guard, the compiler would emit an error for duplicated definition. So the problem only lives in C++20 Named Modules.
> > Correct me if I'm mistaken, but I think this issue only occurs because, in the test, both modules have the problematic declarations in the global module fragment; thus creating duplicate definitions that have to be merged which then exposes the ODR mismatch.
> > 
> > I'm suspicious that this actually fixes all possible scenarios. For example:
> >   //--- X1.cpp
> >   #include "foo.h"
> >   import A;
> > 
> >   //--- X2.cpp
> >   import A;
> >   #include "foo.h"
> > 
> > I would expect the proposed change to cause an ODR issue in these scenarios since the definition from the module still needs to be merged in non-modular TUs, but now the imported module will lack the attribute present in the non-modular TUs.
> > Correct me if I'm mistaken, but I think this issue only occurs because, in the test, both modules have the problematic declarations in the global module fragment; thus creating duplicate definitions that have to be merged which then exposes the ODR mismatch.
> 
> I am not sure if I followed. If you are referring to why this problem only exists in C++20 Named Modules, I think you are correct. Other modules (Clang modules, C++20 Header units) don't have global modules.
> 
> > I'm suspicious that this actually fixes all possible scenarios. For example:
> 
> I've added the two examples below. I understand this is confusing at the first sight. There are two cases.
> (1) For `X1.cpp`, we do ODR checks in ASTReaders by calling `ASTContext::isSameEntity.` And  `ASTContext::isSameEntity` wouldn't check for attributes.  (Another defect?)
> (2) For `X2.cpp`, we do ODR checks in Sema. And it would emit a warning as the tests shows.
> 
> So as a conclusion, the current implementation works 'not bad' currently. But I agree that it might bad in the future. Especially WG21 are going to disallow the compilers to ignore the semantics of attributes.
> 
> I am not sure if I followed. If you are referring to why this problem only exists in C++20 Named Modules, I think you are correct.

I was just trying to summarize the root cause again; to make sure I understand when and why the problem occurs.

> (1) For X1.cpp, we do ODR checks in ASTReaders by calling ASTContext::isSameEntity. And ASTContext::isSameEntity wouldn't check for attributes. (Another defect?)

Yeah, that strikes me as likely being another defect; In your added test cases, I suspect we should be doing ODR checks for both `Use1.cpp` and `Use2.cpp` (which would then produce a consistent ODR error instead of the asymmetric warning that is currently issued).


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

https://reviews.llvm.org/D130331