[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