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

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added inline comments.


================
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);
----------------
tahonermann wrote:
> tahonermann wrote:
> > 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).
> @ChuanqiXu and I chatted briefly during today's biweekly Clang Modules call. He was able to explain that the `Use2.cpp` test case also encounters the reported problem without his patch. That suffices to address my major concerns, so I'll approve.
> 
> I do hope we can continue to investigate and address the root cause of the problem though as I'm sure this issue will bite again.
>  For X1.cpp, we do ODR checks in ASTReaders by calling ASTContext::isSameEntity. And  ASTContext::isSameEntity wouldn't check for attributes. (Another defect?)

Oh, my bad writing. The `another defect` refers that we don't check attributes in ASTReaders. It may surprise some people. (But it may be expected due to some people think attributes as a hint instead of a semantical entity.)

> I do hope we can continue to investigate and address the root cause of the problem though as I'm sure this issue will bite again.

Yeah, I'll take an eye on this. Although there are many TODOs, I feel like we could fix the root problem in clang16.


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

https://reviews.llvm.org/D130331