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...@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-er...@b.cppm:* {{call to object of type '__fn' is ambiguous}} +// expected-note@* 1+{{candidate function}} +#elif defined(DIFFERENT) +// expected-error@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-er...@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@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@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@func2.h:1 {{'func' has different definitions in different modules;}} +// expected-note@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...@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. _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
