ChuanqiXu added inline comments.
================ Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:2925-2926 for (unsigned I = 0, E = readInt(); I != E; ++I) - Attrs.push_back(readAttr()); + if (auto *Attr = readAttr()) + Attrs.push_back(Attr); } ---------------- tahonermann wrote: > Interesting, this looks like a pre-existing issue. It looks like > `readBTFTypeTagAttr()` in > `clang/include/clang/Serialization/ASTRecordReader.h` has an assumption that > `readAttr()` always returns non-null. I wonder if that should be modified to > use `cast_or_null` rather than `cast`. I don't see any need to address that > issue (if it actually exists) in this review though. Yeah, it surprises me too. ================ 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: > 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. 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
