[backport] [C++20] [Modules] Backport the ability to skip ODR checks in GMF #80249
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-modules @llvm/pr-subscribers-clang-driver Author: Chuanqi Xu (ChuanqiXu9) ChangesThe backport follows the new practice suggested in https://discourse.llvm.org/t/release-18-x-branch-has-been-created/76480. See #79240 and #79959 for the full context. This is pretty helpful to improve the user experiences in modules given there are a lot of issue reports about false positive ODR violation diagnostics. Patch is 25.64 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/80249.diff 18 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 060bc7669b72a..ca6f4439971f1 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -188,6 +188,12 @@ 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. 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
^^^^^^^^^^^^^^^^^^^^^
- Implemented `P0847R7: Deducing this <https://wg21.link/P0847R7>`_. Some related core issues were also
@@ -1041,9 +1047,6 @@ Bug Fixes to C++ Support
in different visibility.
Fixes (`#67893 <https://github.com/llvm/llvm-project/issues/67893>`_)
-- Fix a false-positive ODR violation for different definitions for `std::align_val_t`.
- Fixes (`#76638 <https://github.com/llvm/llvm-project/issues/76638>`_)
-
- Remove recorded `#pragma once` state for headers included in named modules.
Fixes (`#77995 <https://github.com/llvm/llvm-project/issues/77995>`_)
diff --git a/clang/docs/StandardCPlusPlusModules.rst b/clang/docs/StandardCPlusPlusModules.rst
index 81043ff25be02..0f85065f464a8 100644
--- a/clang/docs/StandardCPlusPlusModules.rst
+++ b/clang/docs/StandardCPlusPlusModules.rst
@@ -457,6 +457,29 @@ 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 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 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
+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 8fc75e1cca039..4942dcaa086ea 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 773bc1dcda01d..e8d03fc269023 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 dd1451bbf2d2c..cd28226c295b3 100644
--- a/clang/include/clang/Serialization/ASTReader.h
+++ b/clang/include/clang/Serialization/ASTReader.h
@@ -2452,6 +2452,12 @@ class BitsUnpacker {
uint32_t CurrentBitsIndex = ~0;
};
+inline bool shouldSkipCheckingODR(const Decl *D) {
+ return D->getOwningModule() &&
+ D->getASTContext().getLangOpts().SkipODRCheckInGMF &&
+ D->getOwningModule()->isExplicitGlobalModule();
+}
+
} // namespace clang
#endif // LLVM_CLANG_SERIALIZATION_ASTREADER_H
diff --git a/clang/lib/AST/ODRHash.cpp b/clang/lib/AST/ODRHash.cpp
index 5b98646a1e8dc..2dbc259138a89 100644
--- a/clang/lib/AST/ODRHash.cpp
+++ b/clang/lib/AST/ODRHash.cpp
@@ -745,55 +745,8 @@ void ODRHash::AddEnumDecl(const EnumDecl *Enum) {
if (Enum->isScoped())
AddBoolean(Enum->isScopedUsingClassTag());
- if (Enum->getIntegerTypeSourceInfo()) {
- // FIMXE: This allows two enums with different spellings to have the same
- // hash.
- //
- // // mod1.cppm
- // module;
- // extern "C" {
- // typedef unsigned __int64 size_t;
- // }
- // namespace std {
- // using :: size_t;
- // }
- //
- // extern "C++" {
- // namespace std {
- // enum class align_val_t : std::size_t {};
- // }
- // }
- //
- // export module mod1;
- // export using std::align_val_t;
- //
- // // mod2.cppm
- // module;
- // extern "C" {
- // typedef unsigned __int64 size_t;
- // }
- //
- // extern "C++" {
- // namespace std {
- // enum class align_val_t : size_t {};
- // }
- // }
- //
- // export module mod2;
- // import mod1;
- // export using std::align_val_t;
- //
- // The above example should be disallowed since it violates
- // [basic.def.odr]p14:
- //
- // Each such definition shall consist of the same sequence of tokens
- //
- // The definitions of `std::align_val_t` in two module units have different
- // spellings but we failed to give an error here.
- //
- // See https://github.com/llvm/llvm-project/issues/76638 for details.
+ if (Enum->getIntegerTypeSourceInfo())
AddQualType(Enum->getIntegerType().getCanonicalType());
- }
// Filter out sub-Decls which will not be processed in order to get an
// accurate count of Decl's.
diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp
index 8092fc050b0ee..98272a27574d4 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -3942,6 +3942,10 @@ static bool RenderModulesOptions(Compilation &C, const Driver &D,
Args.ClaimAllArgs(options::OPT_fmodules_disable_diagnostic_validation);
}
+ // 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.
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 fecd94e875f67..028610deb3001 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.
+ !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 a149d82153037..1fadd8039462d 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 (!shouldSkipCheckingODR(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,10 @@ 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 (!shouldSkipCheckingODR(ED) &&
+ OldDef->getODRHash() != ED->getODRHash())
Reader.PendingEnumOdrMergeFailures[OldDef].push_back(ED);
} else {
OldDef = ED;
@@ -866,6 +871,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(!shouldSkipCheckingODR(RD));
RD->setODRHash(Record.readInt());
// Maintain the invariant of a redeclaration chain containing only
@@ -1094,8 +1102,10 @@ void ASTDeclReader::VisitFunctionDecl(FunctionDecl *FD) {
if (FD->isExplicitlyDefaulted())
FD->setDefaultLoc(readSourceLocation());
- FD->ODRHash = Record.readInt();
- FD->setHasODRHash(true);
+ if (!shouldSkipCheckingODR(FD)) {
+ FD->ODRHash = Record.readInt();
+ FD->setHasODRHash(true);
+ }
if (FD->isDefaulted()) {
if (unsigned NumLookups = Record.readInt()) {
@@ -1971,9 +1981,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 (!shouldSkipCheckingODR(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 +2147,10 @@ void ASTDeclReader::MergeDefinitionData(
}
}
+ // We don't want to check ODR for decls in the global module fragment.
+ if (shouldSkipCheckingODR(MergeDD.Definition))
+ return;
+
if (D->getODRHash() != MergeDD.ODRHash) {
DetectedOdrViolation = true;
}
@@ -3498,11 +3515,14 @@ 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() &&
- MergedDCIt->second == D->getDeclContext())
+ !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 03bddfe0f5047..3b79a9238d1af 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 (!shouldSkipCheckingODR(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 bb1f51786d281..f224075643e99 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 (!shouldSkipCheckingODR(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) && !shouldSkipCheckingODR(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 (!shouldSkipCheckingODR(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()) {
+ !shouldSkipCheckingODR(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/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 0e85a46411a54..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,7 +76,10 @@ module;
export module B;
import A;
-#ifdef DIFFERENT
+#ifdef SKIP_ODR_CHECK_IN_GMF
+// expected-error@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
diff --git a/clang/test/Modules/cxx20-modules-enum-odr.cppm b/clang/test/Modules/cxx20-modules-enum-odr.cppm
new file mode 100644
index 0000000000000..831c01143a27b
--- /dev/null
+++ b/clang/test/Modules/cxx20-modules-enum-odr.cppm
@@ -0,0 +1,51 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+//
+// RUN: %clang_cc1 -std=c++20 %t/mod1.cppm -emit-module-interface -o %t/mod1.pcm
+// RUN: %clang_cc1 -std=c++20 %t/mod2.cppm -emit-module-interface -o %t/mod2.pcm
+// RUN: %clang_cc1 -std=c++20 %t/test.cpp -fprebuilt-module-path=%t -verify -fsyntax-only
+
+//--- size_t.h
+
+extern "C" {
+ typedef unsigned int size_t;
+}
+
+//--- csize_t
+namespace std {
+ using :: size_t;
+}
+
+//--- align.h
+namespace std {
+ enum class align_val_t : size_t {};
+}
+
+//--- mod1.cppm
+module;
+#include "size_t.h"
+#include "align.h"
+export module mod1;
+namespace std {
+export using std::align_val_t;
+}
+
+//--- mod2.cppm
+module;
+#include "size_t.h"
+#include "csize_t"
+#include "align.h"
+export module mod2;
+namespace std {
+export using std::align_val_t;
+}
+
+//--- test.cpp
+// expected-no-diagnostics
+import mod1;
+import mod2;
+void test() {
+ std::align_val_t v;
+}
+
diff --git a/clang/test/Modules/no-eager-load.cppm b/clang/test/Modules/no-eager-load.cppm
index 6632cc60c8eb8..8a2c7656bca2b 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 -ver...
[truncated]
|
|
@llvm/pr-subscribers-clang Author: Chuanqi Xu (ChuanqiXu9) ChangesThe backport follows the new practice suggested in https://discourse.llvm.org/t/release-18-x-branch-has-been-created/76480. See #79240 and #79959 for the full context. This is pretty helpful to improve the user experiences in modules given there are a lot of issue reports about false positive ODR violation diagnostics. Patch is 25.64 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/80249.diff 18 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 060bc7669b72a..ca6f4439971f1 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -188,6 +188,12 @@ 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. 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
^^^^^^^^^^^^^^^^^^^^^
- Implemented `P0847R7: Deducing this <https://wg21.link/P0847R7>`_. Some related core issues were also
@@ -1041,9 +1047,6 @@ Bug Fixes to C++ Support
in different visibility.
Fixes (`#67893 <https://github.com/llvm/llvm-project/issues/67893>`_)
-- Fix a false-positive ODR violation for different definitions for `std::align_val_t`.
- Fixes (`#76638 <https://github.com/llvm/llvm-project/issues/76638>`_)
-
- Remove recorded `#pragma once` state for headers included in named modules.
Fixes (`#77995 <https://github.com/llvm/llvm-project/issues/77995>`_)
diff --git a/clang/docs/StandardCPlusPlusModules.rst b/clang/docs/StandardCPlusPlusModules.rst
index 81043ff25be02..0f85065f464a8 100644
--- a/clang/docs/StandardCPlusPlusModules.rst
+++ b/clang/docs/StandardCPlusPlusModules.rst
@@ -457,6 +457,29 @@ 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 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 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
+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 8fc75e1cca039..4942dcaa086ea 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 773bc1dcda01d..e8d03fc269023 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 dd1451bbf2d2c..cd28226c295b3 100644
--- a/clang/include/clang/Serialization/ASTReader.h
+++ b/clang/include/clang/Serialization/ASTReader.h
@@ -2452,6 +2452,12 @@ class BitsUnpacker {
uint32_t CurrentBitsIndex = ~0;
};
+inline bool shouldSkipCheckingODR(const Decl *D) {
+ return D->getOwningModule() &&
+ D->getASTContext().getLangOpts().SkipODRCheckInGMF &&
+ D->getOwningModule()->isExplicitGlobalModule();
+}
+
} // namespace clang
#endif // LLVM_CLANG_SERIALIZATION_ASTREADER_H
diff --git a/clang/lib/AST/ODRHash.cpp b/clang/lib/AST/ODRHash.cpp
index 5b98646a1e8dc..2dbc259138a89 100644
--- a/clang/lib/AST/ODRHash.cpp
+++ b/clang/lib/AST/ODRHash.cpp
@@ -745,55 +745,8 @@ void ODRHash::AddEnumDecl(const EnumDecl *Enum) {
if (Enum->isScoped())
AddBoolean(Enum->isScopedUsingClassTag());
- if (Enum->getIntegerTypeSourceInfo()) {
- // FIMXE: This allows two enums with different spellings to have the same
- // hash.
- //
- // // mod1.cppm
- // module;
- // extern "C" {
- // typedef unsigned __int64 size_t;
- // }
- // namespace std {
- // using :: size_t;
- // }
- //
- // extern "C++" {
- // namespace std {
- // enum class align_val_t : std::size_t {};
- // }
- // }
- //
- // export module mod1;
- // export using std::align_val_t;
- //
- // // mod2.cppm
- // module;
- // extern "C" {
- // typedef unsigned __int64 size_t;
- // }
- //
- // extern "C++" {
- // namespace std {
- // enum class align_val_t : size_t {};
- // }
- // }
- //
- // export module mod2;
- // import mod1;
- // export using std::align_val_t;
- //
- // The above example should be disallowed since it violates
- // [basic.def.odr]p14:
- //
- // Each such definition shall consist of the same sequence of tokens
- //
- // The definitions of `std::align_val_t` in two module units have different
- // spellings but we failed to give an error here.
- //
- // See https://github.com/llvm/llvm-project/issues/76638 for details.
+ if (Enum->getIntegerTypeSourceInfo())
AddQualType(Enum->getIntegerType().getCanonicalType());
- }
// Filter out sub-Decls which will not be processed in order to get an
// accurate count of Decl's.
diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp
index 8092fc050b0ee..98272a27574d4 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -3942,6 +3942,10 @@ static bool RenderModulesOptions(Compilation &C, const Driver &D,
Args.ClaimAllArgs(options::OPT_fmodules_disable_diagnostic_validation);
}
+ // 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.
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 fecd94e875f67..028610deb3001 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.
+ !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 a149d82153037..1fadd8039462d 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 (!shouldSkipCheckingODR(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,10 @@ 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 (!shouldSkipCheckingODR(ED) &&
+ OldDef->getODRHash() != ED->getODRHash())
Reader.PendingEnumOdrMergeFailures[OldDef].push_back(ED);
} else {
OldDef = ED;
@@ -866,6 +871,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(!shouldSkipCheckingODR(RD));
RD->setODRHash(Record.readInt());
// Maintain the invariant of a redeclaration chain containing only
@@ -1094,8 +1102,10 @@ void ASTDeclReader::VisitFunctionDecl(FunctionDecl *FD) {
if (FD->isExplicitlyDefaulted())
FD->setDefaultLoc(readSourceLocation());
- FD->ODRHash = Record.readInt();
- FD->setHasODRHash(true);
+ if (!shouldSkipCheckingODR(FD)) {
+ FD->ODRHash = Record.readInt();
+ FD->setHasODRHash(true);
+ }
if (FD->isDefaulted()) {
if (unsigned NumLookups = Record.readInt()) {
@@ -1971,9 +1981,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 (!shouldSkipCheckingODR(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 +2147,10 @@ void ASTDeclReader::MergeDefinitionData(
}
}
+ // We don't want to check ODR for decls in the global module fragment.
+ if (shouldSkipCheckingODR(MergeDD.Definition))
+ return;
+
if (D->getODRHash() != MergeDD.ODRHash) {
DetectedOdrViolation = true;
}
@@ -3498,11 +3515,14 @@ 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() &&
- MergedDCIt->second == D->getDeclContext())
+ !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 03bddfe0f5047..3b79a9238d1af 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 (!shouldSkipCheckingODR(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 bb1f51786d281..f224075643e99 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 (!shouldSkipCheckingODR(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) && !shouldSkipCheckingODR(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 (!shouldSkipCheckingODR(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()) {
+ !shouldSkipCheckingODR(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/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 0e85a46411a54..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,7 +76,10 @@ module;
export module B;
import A;
-#ifdef DIFFERENT
+#ifdef SKIP_ODR_CHECK_IN_GMF
+// expected-error@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
diff --git a/clang/test/Modules/cxx20-modules-enum-odr.cppm b/clang/test/Modules/cxx20-modules-enum-odr.cppm
new file mode 100644
index 0000000000000..831c01143a27b
--- /dev/null
+++ b/clang/test/Modules/cxx20-modules-enum-odr.cppm
@@ -0,0 +1,51 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+//
+// RUN: %clang_cc1 -std=c++20 %t/mod1.cppm -emit-module-interface -o %t/mod1.pcm
+// RUN: %clang_cc1 -std=c++20 %t/mod2.cppm -emit-module-interface -o %t/mod2.pcm
+// RUN: %clang_cc1 -std=c++20 %t/test.cpp -fprebuilt-module-path=%t -verify -fsyntax-only
+
+//--- size_t.h
+
+extern "C" {
+ typedef unsigned int size_t;
+}
+
+//--- csize_t
+namespace std {
+ using :: size_t;
+}
+
+//--- align.h
+namespace std {
+ enum class align_val_t : size_t {};
+}
+
+//--- mod1.cppm
+module;
+#include "size_t.h"
+#include "align.h"
+export module mod1;
+namespace std {
+export using std::align_val_t;
+}
+
+//--- mod2.cppm
+module;
+#include "size_t.h"
+#include "csize_t"
+#include "align.h"
+export module mod2;
+namespace std {
+export using std::align_val_t;
+}
+
+//--- test.cpp
+// expected-no-diagnostics
+import mod1;
+import mod2;
+void test() {
+ std::align_val_t v;
+}
+
diff --git a/clang/test/Modules/no-eager-load.cppm b/clang/test/Modules/no-eager-load.cppm
index 6632cc60c8eb8..8a2c7656bca2b 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 -ver...
[truncated]
|
Close llvm#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.
Previosly we land llvm@085eae6 to workaround the false positive ODR violations in llvm#76638. However, we decided to not perform ODR checks for decls from GMF in llvm#79240 and we land the corresponding change. So we should be able to remove the workaround now. The original tests get remained.
Close llvm#79240 Cite the comment from @mizvekov in //github.com/llvm/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.
tstellar
force-pushed
the
SkipODRCheckInGMF
branch
from
c429f61
to
c6c8696
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The backport follows the new practice suggested in https://discourse.llvm.org/t/release-18-x-branch-has-been-created/76480.
See #79240 and #79959 for the full context.
This is pretty helpful to improve the user experiences in modules given there are a lot of issue reports about false positive ODR violation diagnostics.
I saw many issue reports complaining the false positive ODR violation diagnostics. In fact, after I disabled the ODR checks in GMF, there 2 people reporting github issues saying they feel good and one people sent me a private email to say it solves his problems. So I feel this is the "correct" way to go while I understand it is not strictly correct.