[llvm-branch-commits] [clang] a0e1cc0 - [C++20] [Modules] Don't perform ODR checks in GMF
Tom Stellard via llvm-branch-commits
llvm-branch-commits at lists.llvm.org
Tue Feb 6 21:31:44 PST 2024
Author: Chuanqi Xu
Date: 2024-02-06T21:30:14-08:00
New Revision: a0e1cc0aefbc879cb2861003e487576e9a6b3c93
URL: https://github.com/llvm/llvm-project/commit/a0e1cc0aefbc879cb2861003e487576e9a6b3c93
DIFF: https://github.com/llvm/llvm-project/commit/a0e1cc0aefbc879cb2861003e487576e9a6b3c93.diff
LOG: [C++20] [Modules] Don't perform ODR checks in GMF
Close https://github.com/llvm/llvm-project/issues/79240.
See the linked issue for details. Given the frequency of issue reporting
about false positive ODR checks (I received private issue reports too),
I'd like to backport this to 18.x too.
Added:
Modified:
clang/docs/ReleaseNotes.rst
clang/include/clang/Serialization/ASTReader.h
clang/lib/Serialization/ASTReader.cpp
clang/lib/Serialization/ASTReaderDecl.cpp
clang/lib/Serialization/ASTWriter.cpp
clang/lib/Serialization/ASTWriterDecl.cpp
clang/test/Modules/concept.cppm
clang/test/Modules/no-eager-load.cppm
clang/test/Modules/polluted-operator.cppm
clang/test/Modules/pr76638.cppm
Removed:
################################################################################
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 0e4bda7d8f3505..ce94b3e44b955c 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -188,6 +188,11 @@ C++20 Feature Support
This feature is still experimental. Accordingly, ``__cpp_nontype_template_args`` was not updated.
However, its support can be tested with ``__has_extension(cxx_generalized_nttp)``.
+- Clang won't perform ODR checks for decls in the global module fragment any
+ more to ease the implementation and improve the user's using experience.
+ This follows the MSVC's behavior.
+ (`#79240 <https://github.com/llvm/llvm-project/issues/79240>`_).
+
C++23 Feature Support
^^^^^^^^^^^^^^^^^^^^^
- Implemented `P0847R7: Deducing this <https://wg21.link/P0847R7>`_. Some related core issues were also
diff --git a/clang/include/clang/Serialization/ASTReader.h b/clang/include/clang/Serialization/ASTReader.h
index dd1451bbf2d2c9..ba06ab0cd45097 100644
--- a/clang/include/clang/Serialization/ASTReader.h
+++ b/clang/include/clang/Serialization/ASTReader.h
@@ -2452,6 +2452,10 @@ class BitsUnpacker {
uint32_t CurrentBitsIndex = ~0;
};
+inline bool isFromExplicitGMF(const Decl *D) {
+ return D->getOwningModule() && D->getOwningModule()->isExplicitGlobalModule();
+}
+
} // namespace clang
#endif // LLVM_CLANG_SERIALIZATION_ASTREADER_H
diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp
index fecd94e875f671..e91c5fe08a043f 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -9743,6 +9743,9 @@ void ASTReader::finishPendingActions() {
if (!FD->isLateTemplateParsed() &&
!NonConstDefn->isLateTemplateParsed() &&
+ // We only perform ODR checks for decls not in the explicit
+ // global module fragment.
+ !isFromExplicitGMF(FD) &&
FD->getODRHash() != NonConstDefn->getODRHash()) {
if (!isa<CXXMethodDecl>(FD)) {
PendingFunctionOdrMergeFailures[FD].push_back(NonConstDefn);
diff --git a/clang/lib/Serialization/ASTReaderDecl.cpp b/clang/lib/Serialization/ASTReaderDecl.cpp
index a149d82153037f..7697f29b9054b4 100644
--- a/clang/lib/Serialization/ASTReaderDecl.cpp
+++ b/clang/lib/Serialization/ASTReaderDecl.cpp
@@ -804,8 +804,10 @@ void ASTDeclReader::VisitEnumDecl(EnumDecl *ED) {
ED->setScopedUsingClassTag(EnumDeclBits.getNextBit());
ED->setFixed(EnumDeclBits.getNextBit());
- ED->setHasODRHash(true);
- ED->ODRHash = Record.readInt();
+ if (!isFromExplicitGMF(ED)) {
+ ED->setHasODRHash(true);
+ ED->ODRHash = Record.readInt();
+ }
// If this is a definition subject to the ODR, and we already have a
// definition, merge this one into it.
@@ -827,7 +829,9 @@ void ASTDeclReader::VisitEnumDecl(EnumDecl *ED) {
Reader.MergedDeclContexts.insert(std::make_pair(ED, OldDef));
ED->demoteThisDefinitionToDeclaration();
Reader.mergeDefinitionVisibility(OldDef, ED);
- if (OldDef->getODRHash() != ED->getODRHash())
+ // We don't want to check the ODR hash value for declarations from global
+ // module fragment.
+ if (!isFromExplicitGMF(ED) && OldDef->getODRHash() != ED->getODRHash())
Reader.PendingEnumOdrMergeFailures[OldDef].push_back(ED);
} else {
OldDef = ED;
@@ -866,6 +870,9 @@ ASTDeclReader::VisitRecordDeclImpl(RecordDecl *RD) {
void ASTDeclReader::VisitRecordDecl(RecordDecl *RD) {
VisitRecordDeclImpl(RD);
+ // We should only reach here if we're in C/Objective-C. There is no
+ // global module fragment.
+ assert(!isFromExplicitGMF(RD));
RD->setODRHash(Record.readInt());
// Maintain the invariant of a redeclaration chain containing only
@@ -1094,8 +1101,10 @@ void ASTDeclReader::VisitFunctionDecl(FunctionDecl *FD) {
if (FD->isExplicitlyDefaulted())
FD->setDefaultLoc(readSourceLocation());
- FD->ODRHash = Record.readInt();
- FD->setHasODRHash(true);
+ if (!isFromExplicitGMF(FD)) {
+ FD->ODRHash = Record.readInt();
+ FD->setHasODRHash(true);
+ }
if (FD->isDefaulted()) {
if (unsigned NumLookups = Record.readInt()) {
@@ -1971,9 +1980,12 @@ void ASTDeclReader::ReadCXXDefinitionData(
#include "clang/AST/CXXRecordDeclDefinitionBits.def"
#undef FIELD
- // Note: the caller has deserialized the IsLambda bit already.
- Data.ODRHash = Record.readInt();
- Data.HasODRHash = true;
+ // We only perform ODR checks for decls not in GMF.
+ if (!isFromExplicitGMF(D)) {
+ // Note: the caller has deserialized the IsLambda bit already.
+ Data.ODRHash = Record.readInt();
+ Data.HasODRHash = true;
+ }
if (Record.readInt()) {
Reader.DefinitionSource[D] =
@@ -2134,6 +2146,10 @@ void ASTDeclReader::MergeDefinitionData(
}
}
+ // We don't want to check ODR for decls in the global module fragment.
+ if (isFromExplicitGMF(MergeDD.Definition))
+ return;
+
if (D->getODRHash() != MergeDD.ODRHash) {
DetectedOdrViolation = true;
}
@@ -3498,10 +3514,13 @@ ASTDeclReader::FindExistingResult ASTDeclReader::findExisting(NamedDecl *D) {
// If this declaration is from a merged context, make a note that we need to
// check that the canonical definition of that context contains the decl.
//
+ // Note that we don't perform ODR checks for decls from the global module
+ // fragment.
+ //
// FIXME: We should do something similar if we merge two definitions of the
// same template specialization into the same CXXRecordDecl.
auto MergedDCIt = Reader.MergedDeclContexts.find(D->getLexicalDeclContext());
- if (MergedDCIt != Reader.MergedDeclContexts.end() &&
+ if (MergedDCIt != Reader.MergedDeclContexts.end() && !isFromExplicitGMF(D) &&
MergedDCIt->second == D->getDeclContext())
Reader.PendingOdrMergeChecks.push_back(D);
diff --git a/clang/lib/Serialization/ASTWriter.cpp b/clang/lib/Serialization/ASTWriter.cpp
index 03bddfe0f5047d..b64a6abb90f824 100644
--- a/clang/lib/Serialization/ASTWriter.cpp
+++ b/clang/lib/Serialization/ASTWriter.cpp
@@ -6022,8 +6022,12 @@ void ASTRecordWriter::AddCXXDefinitionData(const CXXRecordDecl *D) {
Record->push_back(DefinitionBits);
- // getODRHash will compute the ODRHash if it has not been previously computed.
- Record->push_back(D->getODRHash());
+ // We only perform ODR checks for decls not in GMF.
+ if (!isFromExplicitGMF(D)) {
+ // getODRHash will compute the ODRHash if it has not been previously
+ // computed.
+ Record->push_back(D->getODRHash());
+ }
bool ModulesDebugInfo =
Writer->Context->getLangOpts().ModulesDebugInfo && !D->isDependentType();
diff --git a/clang/lib/Serialization/ASTWriterDecl.cpp b/clang/lib/Serialization/ASTWriterDecl.cpp
index bb1f51786d2813..86d40436a9bac1 100644
--- a/clang/lib/Serialization/ASTWriterDecl.cpp
+++ b/clang/lib/Serialization/ASTWriterDecl.cpp
@@ -493,7 +493,9 @@ void ASTDeclWriter::VisitEnumDecl(EnumDecl *D) {
EnumDeclBits.addBit(D->isFixed());
Record.push_back(EnumDeclBits);
- Record.push_back(D->getODRHash());
+ // We only perform ODR checks for decls not in GMF.
+ if (!isFromExplicitGMF(D))
+ Record.push_back(D->getODRHash());
if (MemberSpecializationInfo *MemberInfo = D->getMemberSpecializationInfo()) {
Record.AddDeclRef(MemberInfo->getInstantiatedFrom());
@@ -510,7 +512,7 @@ void ASTDeclWriter::VisitEnumDecl(EnumDecl *D) {
!D->isTopLevelDeclInObjCContainer() &&
!CXXRecordDecl::classofKind(D->getKind()) &&
!D->getIntegerTypeSourceInfo() && !D->getMemberSpecializationInfo() &&
- !needsAnonymousDeclarationNumber(D) &&
+ !needsAnonymousDeclarationNumber(D) && !isFromExplicitGMF(D) &&
D->getDeclName().getNameKind() == DeclarationName::Identifier)
AbbrevToUse = Writer.getDeclEnumAbbrev();
@@ -701,7 +703,9 @@ void ASTDeclWriter::VisitFunctionDecl(FunctionDecl *D) {
if (D->isExplicitlyDefaulted())
Record.AddSourceLocation(D->getDefaultLoc());
- Record.push_back(D->getODRHash());
+ // We only perform ODR checks for decls not in GMF.
+ if (!isFromExplicitGMF(D))
+ Record.push_back(D->getODRHash());
if (D->isDefaulted()) {
if (auto *FDI = D->getDefaultedFunctionInfo()) {
@@ -1506,7 +1510,8 @@ void ASTDeclWriter::VisitCXXMethodDecl(CXXMethodDecl *D) {
D->getFirstDecl() == D->getMostRecentDecl() && !D->isInvalidDecl() &&
!D->hasAttrs() && !D->isTopLevelDeclInObjCContainer() &&
D->getDeclName().getNameKind() == DeclarationName::Identifier &&
- !D->hasExtInfo() && !D->isExplicitlyDefaulted()) {
+ !isFromExplicitGMF(D) && !D->hasExtInfo() &&
+ !D->isExplicitlyDefaulted()) {
if (D->getTemplatedKind() == FunctionDecl::TK_NonTemplate ||
D->getTemplatedKind() == FunctionDecl::TK_FunctionTemplate ||
D->getTemplatedKind() == FunctionDecl::TK_MemberSpecialization ||
diff --git a/clang/test/Modules/concept.cppm b/clang/test/Modules/concept.cppm
index 0e85a46411a544..a343c9cd76a119 100644
--- a/clang/test/Modules/concept.cppm
+++ b/clang/test/Modules/concept.cppm
@@ -70,13 +70,6 @@ module;
export module B;
import A;
-#ifdef DIFFERENT
-// expected-error at foo.h:41 {{'__fn::operator()' from module 'A.<global>' is not present in definition of '__fn' provided earlier}}
-// expected-note@* 1+{{declaration of 'operator()' does not match}}
-#else
-// expected-no-diagnostics
-#endif
-
template <class T>
struct U {
auto operator+(U) { return 0; }
@@ -94,3 +87,10 @@ void foo() {
__fn{}(U<int>(), U<int>());
}
+
+#ifdef DIFFERENT
+// expected-error at B.cppm:* {{call to object of type '__fn' is ambiguous}}
+// expected-note@* 1+{{candidate function}}
+#else
+// expected-no-diagnostics
+#endif
diff --git a/clang/test/Modules/no-eager-load.cppm b/clang/test/Modules/no-eager-load.cppm
index 6632cc60c8eb84..8a2c7656bca2b4 100644
--- a/clang/test/Modules/no-eager-load.cppm
+++ b/clang/test/Modules/no-eager-load.cppm
@@ -9,19 +9,10 @@
// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify %t/d.cpp \
// RUN: -fprebuilt-module-path=%t
//
-// RUN: %clang_cc1 -std=c++20 -emit-module-interface %t/e.cppm -o %t/e.pcm
-// RUN: %clang_cc1 -std=c++20 -emit-module-interface %t/f.cppm -o %t/f.pcm
-// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify %t/g.cpp \
-// RUN: -fprebuilt-module-path=%t
-//
// RUN: %clang_cc1 -std=c++20 -emit-module-interface %t/h.cppm \
// RUN: -fprebuilt-module-path=%t -o %t/h.pcm
-// RUN: %clang_cc1 -std=c++20 -emit-module-interface %t/i.cppm \
-// RUN: -fprebuilt-module-path=%t -o %t/i.pcm
// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify %t/j.cpp \
// RUN: -fprebuilt-module-path=%t
-// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify %t/k.cpp \
-// RUN: -fprebuilt-module-path=%t
//--- a.cppm
export module a;
@@ -53,58 +44,14 @@ void use() {
// expected-note@* {{but in 'a' found a
diff erent body}}
}
-//--- foo.h
-void foo() {
-
-}
-
-//--- bar.h
-void bar();
-void foo() {
- bar();
-}
-
-//--- e.cppm
-module;
-#include "foo.h"
-export module e;
-export using ::foo;
-
-//--- f.cppm
-module;
-#include "bar.h"
-export module f;
-export using ::foo;
-
-//--- g.cpp
-import e;
-import f;
-void use() {
- foo(); // expected-error@* {{'foo' has
diff erent definitions in
diff erent modules;}}
- // expected-note@* {{but in 'e.<global>' found a
diff erent body}}
-}
-
//--- h.cppm
export module h;
export import a;
export import b;
-//--- i.cppm
-export module i;
-export import e;
-export import f;
-
//--- j.cpp
import h;
void use() {
foo(); // expected-error@* {{'foo' has
diff erent definitions in
diff erent modules;}}
// expected-note@* {{but in 'a' found a
diff erent body}}
}
-
-//--- k.cpp
-import i;
-void use() {
- foo(); // expected-error@* {{'foo' has
diff erent definitions in
diff erent modules;}}
- // expected-note@* {{but in 'e.<global>' found a
diff erent body}}
-}
-
diff --git a/clang/test/Modules/polluted-operator.cppm b/clang/test/Modules/polluted-operator.cppm
index b24464aa6ad21e..d4b0041b5d346e 100644
--- a/clang/test/Modules/polluted-operator.cppm
+++ b/clang/test/Modules/polluted-operator.cppm
@@ -46,12 +46,10 @@ module;
export module a;
//--- b.cppm
+// This is actually an ODR violation. But given https://github.com/llvm/llvm-project/issues/79240,
+// we don't count it as an ODR violation any more.
+// expected-no-diagnostics
module;
#include "bar.h"
export module b;
import a;
-
-// expected-error@* {{has
diff erent definitions in
diff erent modules; first
diff erence is defined here found data member '_S_copy_ctor' with an initializer}}
-// expected-note@* {{but in 'a.<global>' found data member '_S_copy_ctor' with a
diff erent initializer}}
-// expected-error@* {{from module 'a.<global>' is not present in definition of 'variant<_Types...>' provided earlier}}
-// expected-note@* {{declaration of 'swap' does not match}}
diff --git a/clang/test/Modules/pr76638.cppm b/clang/test/Modules/pr76638.cppm
index 8cc807961421b7..28d03384634ab2 100644
--- a/clang/test/Modules/pr76638.cppm
+++ b/clang/test/Modules/pr76638.cppm
@@ -57,6 +57,9 @@ export module mod3;
export using std::align_val_t;
//--- mod4.cppm
+// This is actually an ODR violation. But given https://github.com/llvm/llvm-project/issues/79240,
+// we don't count it as an ODR violation now.
+// expected-no-diagnostics
module;
#include "signed_size_t.h"
#include "csize_t"
@@ -64,6 +67,3 @@ module;
export module mod4;
import mod3;
export using std::align_val_t;
-
-// expected-error at align.h:* {{'std::align_val_t' has
diff erent definitions in
diff erent modules; defined here first
diff erence is enum with specified type 'size_t' (aka 'int')}}
-// expected-note at align.h:* {{but in 'mod3.<global>' found enum with specified type 'size_t' (aka 'unsigned int')}}
More information about the llvm-branch-commits
mailing list