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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
