[clang] [C++20] [Modules] Introduce -fskip-odr-check-in-gmf (PR #79959)
Chuanqi Xu via cfe-commits
cfe-commits at lists.llvm.org
Tue Jan 30 18:22:08 PST 2024
https://github.com/ChuanqiXu9 updated https://github.com/llvm/llvm-project/pull/79959
>From beb1a4b89f941f41a6e220447dcda6d6fc231a0b Mon Sep 17 00:00:00 2001
From: Chuanqi Xu <yedeng.yd at linux.alibaba.com>
Date: Tue, 30 Jan 2024 15:57:35 +0800
Subject: [PATCH 1/2] [C++20] [Modules] Introduce -fskip-odr-check-in-gmf
Close https://github.com/llvm/llvm-project/issues/79240
Cite the comment from @mizvekov in
//github.com/llvm/llvm-project/issues/79240:
> There are two kinds of bugs / issues relevant here:
>
> Clang bugs that this change hides
> Here we can add a Frontend flag that disables the GMF ODR check, just
> so
> we can keep tracking, testing and fixing these issues.
> The Driver would just always pass that flag.
> We could add that flag in this current issue.
> Bugs in user code:
> I don't think it's worth adding a corresponding Driver flag for
> controlling the above Frontend flag, since we intend it's behavior to
> become default as we fix the problems, and users interested in testing
> the more strict behavior can just use the Frontend flag directly.
This patch follows the suggestion:
- Introduce the CC1 flag `-fskip-odr-check-in-gmf` which is by default
off,
so that the every existing test will still be tested with checking ODR
violations.
- Passing `-fskip-odr-check-in-gmf` in the driver to keep the behavior
we intended.
- Edit the document to tell the users who are still interested in more
strict checks can use `-Xclang -fno-skip-odr-check-in-gmf` to get the
existing behavior.
---
clang/docs/ReleaseNotes.rst | 3 +-
clang/docs/StandardCPlusPlusModules.rst | 22 ++++++++
clang/include/clang/Basic/LangOptions.def | 1 +
clang/include/clang/Driver/Options.td | 8 +++
clang/include/clang/Serialization/ASTReader.h | 6 +-
clang/lib/Driver/ToolChains/Clang.cpp | 8 +++
clang/lib/Serialization/ASTReader.cpp | 2 +-
clang/lib/Serialization/ASTReaderDecl.cpp | 17 +++---
clang/lib/Serialization/ASTWriter.cpp | 2 +-
clang/lib/Serialization/ASTWriterDecl.cpp | 8 +--
.../Driver/modules-skip-odr-check-in-gmf.cpp | 10 ++++
clang/test/Modules/concept.cppm | 23 +++++---
clang/test/Modules/polluted-operator.cppm | 18 +++++-
clang/test/Modules/pr76638.cppm | 16 +++++-
clang/test/Modules/skip-odr-check-in-gmf.cppm | 56 +++++++++++++++++++
15 files changed, 170 insertions(+), 30 deletions(-)
create mode 100644 clang/test/Driver/modules-skip-odr-check-in-gmf.cpp
create mode 100644 clang/test/Modules/skip-odr-check-in-gmf.cppm
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index bac6c7162a45b..74f34eeef65f0 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -64,7 +64,8 @@ C++20 Feature Support
- 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.
+ This follows the MSVC's behavior. Users interested in testing the more strict
+ behavior can use the flag '-Xclang -fno-skip-odr-check-in-gmf'.
(`#79240 <https://github.com/llvm/llvm-project/issues/79240>`_).
C++23 Feature Support
diff --git a/clang/docs/StandardCPlusPlusModules.rst b/clang/docs/StandardCPlusPlusModules.rst
index 4e853990a7338..44870f210cde6 100644
--- a/clang/docs/StandardCPlusPlusModules.rst
+++ b/clang/docs/StandardCPlusPlusModules.rst
@@ -457,6 +457,28 @@ Note that **currently** the compiler doesn't consider inconsistent macro definit
Currently Clang would accept the above example. But it may produce surprising results if the
debugging code depends on consistent use of ``NDEBUG`` also in other translation units.
+Definitions consistency
+^^^^^^^^^^^^^^^^^^^^^^^
+
+The C++ language defines that same declarations in different translation units should have
+the same definition, as known as ODR (One Definition Rule). Prior to modules, the translation
+units don't dependent on each other and the compiler itself don't and can't perform a strong
+ODR violation check. Sometimes it is the linker does some jobs related to ODR, where the
+higher level semantics are missing. With the introduction of modules, now the compiler have
+the chance to perform ODR violations with language semantics across translation units.
+
+However, in the practice we found the existing ODR checking mechanism may be too aggressive.
+In the many issue reports about ODR violation diagnostics, most of them are false positive
+ODR violations and the true positive ODR violations are rarely reported. Also MSVC don't
+perform ODR check for declarations in the global module fragment.
+
+So in order to get better user experience, save the time checking ODR and keep consistent
+behavior with MSVC, we disabled the ODR check for the declarations in the global module
+fragment by default. Users who want more strict check can still use the
+``-Xclang -fno-skip-odr-check-in-gmf`` flag to get the ODR check enabled. It is also
+encouraged to report issues if users find false positive ODR violations or false negative ODR
+violations with the flag enabled.
+
ABI Impacts
-----------
diff --git a/clang/include/clang/Basic/LangOptions.def b/clang/include/clang/Basic/LangOptions.def
index 1e671a7c46016..2b42b521a3036 100644
--- a/clang/include/clang/Basic/LangOptions.def
+++ b/clang/include/clang/Basic/LangOptions.def
@@ -174,6 +174,7 @@ LANGOPT(MathErrno , 1, 1, "errno in math functions")
BENIGN_LANGOPT(HeinousExtensions , 1, 0, "extensions that we really don't like and may be ripped out at any time")
LANGOPT(Modules , 1, 0, "modules semantics")
COMPATIBLE_LANGOPT(CPlusPlusModules, 1, 0, "C++ modules syntax")
+LANGOPT(SkipODRCheckInGMF, 1, 0, "Skip ODR checks for decls in the global module fragment")
LANGOPT(BuiltinHeadersInSystemModules, 1, 0, "builtin headers belong to system modules, and _Builtin_ modules are ignored for cstdlib headers")
BENIGN_ENUM_LANGOPT(CompilingModule, CompilingModuleKind, 3, CMK_None,
"compiling a module interface")
diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index d940665969339..a4b35e370999e 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -2985,6 +2985,14 @@ def fmodule_output : Flag<["-"], "fmodule-output">, Flags<[NoXarchOption]>,
Visibility<[ClangOption, CC1Option]>,
HelpText<"Save intermediate module file results when compiling a standard C++ module unit.">;
+defm skip_odr_check_in_gmf : BoolOption<"f", "skip-odr-check-in-gmf",
+ LangOpts<"SkipODRCheckInGMF">, DefaultFalse,
+ PosFlag<SetTrue, [], [CC1Option],
+ "Skip ODR checks for decls in the global module fragment.">,
+ NegFlag<SetFalse, [], [CC1Option],
+ "Perform ODR checks for decls in the global module fragment.">>,
+ Group<f_Group>;
+
def fmodules_prune_interval : Joined<["-"], "fmodules-prune-interval=">, Group<i_Group>,
Visibility<[ClangOption, CC1Option]>, MetaVarName<"<seconds>">,
HelpText<"Specify the interval (in seconds) between attempts to prune the module cache">,
diff --git a/clang/include/clang/Serialization/ASTReader.h b/clang/include/clang/Serialization/ASTReader.h
index ba06ab0cd4509..cd28226c295b3 100644
--- a/clang/include/clang/Serialization/ASTReader.h
+++ b/clang/include/clang/Serialization/ASTReader.h
@@ -2452,8 +2452,10 @@ class BitsUnpacker {
uint32_t CurrentBitsIndex = ~0;
};
-inline bool isFromExplicitGMF(const Decl *D) {
- return D->getOwningModule() && D->getOwningModule()->isExplicitGlobalModule();
+inline bool shouldSkipCheckingODR(const Decl *D) {
+ return D->getOwningModule() &&
+ D->getASTContext().getLangOpts().SkipODRCheckInGMF &&
+ D->getOwningModule()->isExplicitGlobalModule();
}
} // namespace clang
diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp
index 8d8965fdf76fb..6acd3adea03ad 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -3940,6 +3940,14 @@ static bool RenderModulesOptions(Compilation &C, const Driver &D,
Args.ClaimAllArgs(options::OPT_fmodules_disable_diagnostic_validation);
}
+ // Don't check ODR violations for decls in the global module fragment.
+ // 1. To keep consistent behavior with MSVC, which don't check ODR violations
+ // in the global module fragment too.
+ // 2. Give users better using experience since most issue reports complains
+ // the false positive ODR violations diagnostic and the true positive ODR
+ // violations are rarely reported.
+ CmdArgs.push_back("-fskip-odr-check-in-gmf");
+
// Claim `-fmodule-output` and `-fmodule-output=` to avoid unused warnings.
Args.ClaimAllArgs(options::OPT_fmodule_output);
Args.ClaimAllArgs(options::OPT_fmodule_output_EQ);
diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp
index 2abe5e44e2e98..c1d61dc946ff5 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -9749,7 +9749,7 @@ void ASTReader::finishPendingActions() {
!NonConstDefn->isLateTemplateParsed() &&
// We only perform ODR checks for decls not in the explicit
// global module fragment.
- !isFromExplicitGMF(FD) &&
+ !shouldSkipCheckingODR(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 29ece2a132bf8..ffba04f28782e 100644
--- a/clang/lib/Serialization/ASTReaderDecl.cpp
+++ b/clang/lib/Serialization/ASTReaderDecl.cpp
@@ -804,7 +804,7 @@ void ASTDeclReader::VisitEnumDecl(EnumDecl *ED) {
ED->setScopedUsingClassTag(EnumDeclBits.getNextBit());
ED->setFixed(EnumDeclBits.getNextBit());
- if (!isFromExplicitGMF(ED)) {
+ if (!shouldSkipCheckingODR(ED)) {
ED->setHasODRHash(true);
ED->ODRHash = Record.readInt();
}
@@ -831,7 +831,8 @@ void ASTDeclReader::VisitEnumDecl(EnumDecl *ED) {
Reader.mergeDefinitionVisibility(OldDef, ED);
// We don't want to check the ODR hash value for declarations from global
// module fragment.
- if (!isFromExplicitGMF(ED) && OldDef->getODRHash() != ED->getODRHash())
+ if (!shouldSkipCheckingODR(ED) &&
+ OldDef->getODRHash() != ED->getODRHash())
Reader.PendingEnumOdrMergeFailures[OldDef].push_back(ED);
} else {
OldDef = ED;
@@ -872,7 +873,7 @@ 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));
+ assert(!shouldSkipCheckingODR(RD));
RD->setODRHash(Record.readInt());
// Maintain the invariant of a redeclaration chain containing only
@@ -1101,7 +1102,7 @@ void ASTDeclReader::VisitFunctionDecl(FunctionDecl *FD) {
if (FD->isExplicitlyDefaulted())
FD->setDefaultLoc(readSourceLocation());
- if (!isFromExplicitGMF(FD)) {
+ if (!shouldSkipCheckingODR(FD)) {
FD->ODRHash = Record.readInt();
FD->setHasODRHash(true);
}
@@ -1981,7 +1982,7 @@ void ASTDeclReader::ReadCXXDefinitionData(
#undef FIELD
// We only perform ODR checks for decls not in GMF.
- if (!isFromExplicitGMF(D)) {
+ if (!shouldSkipCheckingODR(D)) {
// Note: the caller has deserialized the IsLambda bit already.
Data.ODRHash = Record.readInt();
Data.HasODRHash = true;
@@ -2147,7 +2148,7 @@ void ASTDeclReader::MergeDefinitionData(
}
// We don't want to check ODR for decls in the global module fragment.
- if (isFromExplicitGMF(MergeDD.Definition))
+ if (shouldSkipCheckingODR(MergeDD.Definition))
return;
if (D->getODRHash() != MergeDD.ODRHash) {
@@ -3520,8 +3521,8 @@ ASTDeclReader::FindExistingResult ASTDeclReader::findExisting(NamedDecl *D) {
// 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() && !isFromExplicitGMF(D) &&
- MergedDCIt->second == D->getDeclContext())
+ if (MergedDCIt != Reader.MergedDeclContexts.end() &&
+ !shouldSkipCheckingODR(D) && MergedDCIt->second == D->getDeclContext())
Reader.PendingOdrMergeChecks.push_back(D);
return FindExistingResult(Reader, D, /*Existing=*/nullptr,
diff --git a/clang/lib/Serialization/ASTWriter.cpp b/clang/lib/Serialization/ASTWriter.cpp
index dcb18ad338932..4d067ed54b9e2 100644
--- a/clang/lib/Serialization/ASTWriter.cpp
+++ b/clang/lib/Serialization/ASTWriter.cpp
@@ -6028,7 +6028,7 @@ void ASTRecordWriter::AddCXXDefinitionData(const CXXRecordDecl *D) {
Record->push_back(DefinitionBits);
// We only perform ODR checks for decls not in GMF.
- if (!isFromExplicitGMF(D)) {
+ if (!shouldSkipCheckingODR(D)) {
// getODRHash will compute the ODRHash if it has not been previously
// computed.
Record->push_back(D->getODRHash());
diff --git a/clang/lib/Serialization/ASTWriterDecl.cpp b/clang/lib/Serialization/ASTWriterDecl.cpp
index 86d40436a9bac..f224075643e99 100644
--- a/clang/lib/Serialization/ASTWriterDecl.cpp
+++ b/clang/lib/Serialization/ASTWriterDecl.cpp
@@ -494,7 +494,7 @@ void ASTDeclWriter::VisitEnumDecl(EnumDecl *D) {
Record.push_back(EnumDeclBits);
// We only perform ODR checks for decls not in GMF.
- if (!isFromExplicitGMF(D))
+ if (!shouldSkipCheckingODR(D))
Record.push_back(D->getODRHash());
if (MemberSpecializationInfo *MemberInfo = D->getMemberSpecializationInfo()) {
@@ -512,7 +512,7 @@ void ASTDeclWriter::VisitEnumDecl(EnumDecl *D) {
!D->isTopLevelDeclInObjCContainer() &&
!CXXRecordDecl::classofKind(D->getKind()) &&
!D->getIntegerTypeSourceInfo() && !D->getMemberSpecializationInfo() &&
- !needsAnonymousDeclarationNumber(D) && !isFromExplicitGMF(D) &&
+ !needsAnonymousDeclarationNumber(D) && !shouldSkipCheckingODR(D) &&
D->getDeclName().getNameKind() == DeclarationName::Identifier)
AbbrevToUse = Writer.getDeclEnumAbbrev();
@@ -704,7 +704,7 @@ void ASTDeclWriter::VisitFunctionDecl(FunctionDecl *D) {
Record.AddSourceLocation(D->getDefaultLoc());
// We only perform ODR checks for decls not in GMF.
- if (!isFromExplicitGMF(D))
+ if (!shouldSkipCheckingODR(D))
Record.push_back(D->getODRHash());
if (D->isDefaulted()) {
@@ -1510,7 +1510,7 @@ void ASTDeclWriter::VisitCXXMethodDecl(CXXMethodDecl *D) {
D->getFirstDecl() == D->getMostRecentDecl() && !D->isInvalidDecl() &&
!D->hasAttrs() && !D->isTopLevelDeclInObjCContainer() &&
D->getDeclName().getNameKind() == DeclarationName::Identifier &&
- !isFromExplicitGMF(D) && !D->hasExtInfo() &&
+ !shouldSkipCheckingODR(D) && !D->hasExtInfo() &&
!D->isExplicitlyDefaulted()) {
if (D->getTemplatedKind() == FunctionDecl::TK_NonTemplate ||
D->getTemplatedKind() == FunctionDecl::TK_FunctionTemplate ||
diff --git a/clang/test/Driver/modules-skip-odr-check-in-gmf.cpp b/clang/test/Driver/modules-skip-odr-check-in-gmf.cpp
new file mode 100644
index 0000000000000..b00b6d330ba45
--- /dev/null
+++ b/clang/test/Driver/modules-skip-odr-check-in-gmf.cpp
@@ -0,0 +1,10 @@
+// RUN: %clang -std=c++20 -### -c %s 2>&1 | FileCheck %s
+// RUN: %clang -std=c++20 -fno-skip-odr-check-in-gmf -### -c %s 2>&1 \
+// RUN: | FileCheck %s --check-prefix=UNUSED
+// RUN: %clang -std=c++20 -Xclang -fno-skip-odr-check-in-gmf -### -c %s 2>&1 \
+// RUN: | FileCheck %s --check-prefix=NO-SKIP
+
+// CHECK: -fskip-odr-check-in-gmf
+// UNUSED: warning: argument unused during compilation: '-fno-skip-odr-check-in-gmf'
+// UNUSED-NOT: -fno-skip-odr-check-in-gmf
+// NO-SKIP: -fskip-odr-check-in-gmf{{.*}}-fno-skip-odr-check-in-gmf
diff --git a/clang/test/Modules/concept.cppm b/clang/test/Modules/concept.cppm
index a343c9cd76a11..0fdb5ea896808 100644
--- a/clang/test/Modules/concept.cppm
+++ b/clang/test/Modules/concept.cppm
@@ -5,6 +5,12 @@
// RUN: %clang_cc1 -std=c++20 %t/A.cppm -emit-module-interface -o %t/A.pcm
// RUN: %clang_cc1 -std=c++20 -fprebuilt-module-path=%t -I%t -DDIFFERENT %t/B.cppm -verify
// RUN: %clang_cc1 -std=c++20 -fprebuilt-module-path=%t -I%t %t/B.cppm -verify
+//
+// Testing the behavior of `-fskip-odr-check-in-gmf`
+// RUN: %clang_cc1 -std=c++20 -fskip-odr-check-in-gmf %t/A.cppm -emit-module-interface -o %t/A.pcm
+// RUN: %clang_cc1 -std=c++20 -fskip-odr-check-in-gmf -fprebuilt-module-path=%t -I%t \
+// RUN: -DDIFFERENT -DSKIP_ODR_CHECK_IN_GMF %t/B.cppm -verify
+
//--- foo.h
#ifndef FOO_H
@@ -70,6 +76,16 @@ module;
export module B;
import A;
+#ifdef SKIP_ODR_CHECK_IN_GMF
+// expected-error at B.cppm:* {{call to object of type '__fn' is ambiguous}}
+// expected-note@* 1+{{candidate function}}
+#elif defined(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; }
@@ -87,10 +103,3 @@ 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/polluted-operator.cppm b/clang/test/Modules/polluted-operator.cppm
index d4b0041b5d346..721ca061c939f 100644
--- a/clang/test/Modules/polluted-operator.cppm
+++ b/clang/test/Modules/polluted-operator.cppm
@@ -4,6 +4,12 @@
//
// RUN: %clang_cc1 -std=c++20 -emit-module-interface %t/a.cppm -o %t/a.pcm
// RUN: %clang_cc1 -std=c++20 %t/b.cppm -fprebuilt-module-path=%t -emit-module-interface -o %t/b.pcm -verify
+//
+// Testing the behavior of `-fskip-odr-check-in-gmf`
+// RUN: %clang_cc1 -std=c++20 -fskip-odr-check-in-gmf -emit-module-interface %t/a.cppm -o \
+// RUN: %t/a.pcm
+// RUN: %clang_cc1 -std=c++20 -fskip-odr-check-in-gmf %t/b.cppm -fprebuilt-module-path=%t \
+// RUN: -emit-module-interface -DSKIP_ODR_CHECK_IN_GMF -o %t/b.pcm -verify
//--- foo.h
@@ -46,10 +52,16 @@ 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;
+
+#ifdef SKIP_ODR_CHECK_IN_GMF
+// expected-no-diagnostics
+#else
+// expected-error@* {{has different definitions in different modules; first difference 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 different 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}}
+#endif
diff --git a/clang/test/Modules/pr76638.cppm b/clang/test/Modules/pr76638.cppm
index 28d03384634ab..e4820ba3d79d9 100644
--- a/clang/test/Modules/pr76638.cppm
+++ b/clang/test/Modules/pr76638.cppm
@@ -10,6 +10,12 @@
// RUN: %clang_cc1 -std=c++20 %t/mod4.cppm -fmodule-file=mod3=%t/mod3.pcm \
// RUN: -fsyntax-only -verify
+// Testing the behavior of `-fskip-odr-check-in-gmf`
+// RUN: %clang_cc1 -std=c++20 %t/mod3.cppm -fskip-odr-check-in-gmf \
+// RUN: -emit-module-interface -o %t/mod3.pcm
+// RUN: %clang_cc1 -std=c++20 %t/mod4.cppm -fmodule-file=mod3=%t/mod3.pcm \
+// RUN: -fskip-odr-check-in-gmf -DSKIP_ODR_CHECK_IN_GMF -fsyntax-only -verify
+
//--- size_t.h
extern "C" {
@@ -57,9 +63,6 @@ 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"
@@ -67,3 +70,10 @@ module;
export module mod4;
import mod3;
export using std::align_val_t;
+
+#ifdef SKIP_ODR_CHECK_IN_GMF
+// expected-no-diagnostics
+#else
+// expected-error at align.h:* {{'std::align_val_t' has different definitions in different modules; defined here first difference 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')}}
+#endif
diff --git a/clang/test/Modules/skip-odr-check-in-gmf.cppm b/clang/test/Modules/skip-odr-check-in-gmf.cppm
new file mode 100644
index 0000000000000..3ee7d09224bfa
--- /dev/null
+++ b/clang/test/Modules/skip-odr-check-in-gmf.cppm
@@ -0,0 +1,56 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+//
+// Baseline testing to make sure we can detect the ODR violation from the CC1 invocation.
+// RUNX: %clang_cc1 -std=c++20 %t/a.cppm -emit-module-interface -o %t/a.pcm
+// RUNX: %clang_cc1 -std=c++20 %t/b.cppm -emit-module-interface -o %t/b.pcm
+// RUNX: %clang_cc1 -std=c++20 %t/test.cc -fprebuilt-module-path=%t -fsyntax-only -verify
+//
+// Testing that we can ignore the ODR violation from the driver invocation.
+// RUN: %clang -std=c++20 %t/a.cppm --precompile -o %t/a.pcm
+// RUN: %clang -std=c++20 %t/b.cppm --precompile -o %t/b.pcm
+// RUN: %clang -std=c++20 %t/test.cc -fprebuilt-module-path=%t -fsyntax-only -Xclang -verify \
+// RUN: -DIGNORE_ODR_VIOLATION
+//
+// Testing that the driver can require to check the ODR violation.
+// RUN: %clang -std=c++20 -Xclang -fno-skip-odr-check-in-gmf %t/a.cppm --precompile -o %t/a.pcm
+// RUN: %clang -std=c++20 -Xclang -fno-skip-odr-check-in-gmf %t/b.cppm --precompile -o %t/b.pcm
+// RUN: %clang -std=c++20 -Xclang -fno-skip-odr-check-in-gmf %t/test.cc -fprebuilt-module-path=%t \
+// RUN: -fsyntax-only -Xclang -verify
+
+//--- func1.h
+bool func(int x, int y) {
+ return true;
+}
+
+//--- func2.h
+bool func(int x, int y) {
+ return false;
+}
+
+//--- a.cppm
+module;
+#include "func1.h"
+export module a;
+export using ::func;
+
+//--- b.cppm
+module;
+#include "func2.h"
+export module b;
+export using ::func;
+
+//--- test.cc
+import a;
+import b;
+bool test() {
+ return func(1, 2);
+}
+
+#ifdef IGNORE_ODR_VIOLATION
+// expected-no-diagnostics
+#else
+// expected-error at func2.h:1 {{'func' has different definitions in different modules;}}
+// expected-note at func1.h:1 {{but in 'a.<global>' found a different body}}
+#endif
>From 48a37e6b8375602472b570d5d8c8ff2a7b34e3de Mon Sep 17 00:00:00 2001
From: Chuanqi Xu <yedeng.yd at linux.alibaba.com>
Date: Wed, 31 Jan 2024 10:21:45 +0800
Subject: [PATCH 2/2] Update Document
---
clang/docs/StandardCPlusPlusModules.rst | 15 ++++++++-------
clang/lib/Driver/ToolChains/Clang.cpp | 8 ++------
2 files changed, 10 insertions(+), 13 deletions(-)
diff --git a/clang/docs/StandardCPlusPlusModules.rst b/clang/docs/StandardCPlusPlusModules.rst
index 44870f210cde6..c322805d8db5b 100644
--- a/clang/docs/StandardCPlusPlusModules.rst
+++ b/clang/docs/StandardCPlusPlusModules.rst
@@ -462,15 +462,16 @@ Definitions consistency
The C++ language defines that same declarations in different translation units should have
the same definition, as known as ODR (One Definition Rule). Prior to modules, the translation
-units don't dependent on each other and the compiler itself don't and can't perform a strong
-ODR violation check. Sometimes it is the linker does some jobs related to ODR, where the
-higher level semantics are missing. With the introduction of modules, now the compiler have
+units don't dependent on each other and the compiler itself can't perform a strong
+ODR violation check. With the introduction of modules, now the compiler have
the chance to perform ODR violations with language semantics across translation units.
-However, in the practice we found the existing ODR checking mechanism may be too aggressive.
-In the many issue reports about ODR violation diagnostics, most of them are false positive
-ODR violations and the true positive ODR violations are rarely reported. Also MSVC don't
-perform ODR check for declarations in the global module fragment.
+However, in the practice, we found the existing ODR checking mechanism is not stable
+enough. Many people suffers from the false positive ODR violation diagnostics, AKA,
+the compiler are complaining two identical declarations have different definitions
+incorrectly. Also the true positive ODR violations are rarely reported.
+Also we learned that MSVC don't perform ODR check for declarations in the global module
+fragment.
So in order to get better user experience, save the time checking ODR and keep consistent
behavior with MSVC, we disabled the ODR check for the declarations in the global module
diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp
index 6acd3adea03ad..14c76bb9c9269 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -3940,12 +3940,8 @@ static bool RenderModulesOptions(Compilation &C, const Driver &D,
Args.ClaimAllArgs(options::OPT_fmodules_disable_diagnostic_validation);
}
- // Don't check ODR violations for decls in the global module fragment.
- // 1. To keep consistent behavior with MSVC, which don't check ODR violations
- // in the global module fragment too.
- // 2. Give users better using experience since most issue reports complains
- // the false positive ODR violations diagnostic and the true positive ODR
- // violations are rarely reported.
+ // FIXME: We provisionally don't check ODR violations for decls in the global
+ // module fragment.
CmdArgs.push_back("-fskip-odr-check-in-gmf");
// Claim `-fmodule-output` and `-fmodule-output=` to avoid unused warnings.
More information about the cfe-commits
mailing list