[C++20] [Modules] Introduce -fskip-odr-check-in-gmf #79959
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-modules Author: Chuanqi Xu (ChuanqiXu9) ChangesClose #79240 Cite the comment from @mizvekov in > There are two kinds of bugs / issues relevant here: This patch follows the suggestion:
Full diff: https://github.com/llvm/llvm-project/pull/79959.diff 15 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index bac6c7162a45b..44344ba330c5d 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 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..c3d0c702a44c9 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, saving 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..0263ebd6b6b10 100644
--- a/clang/include/clang/Serialization/ASTReader.h
+++ b/clang/include/clang/Serialization/ASTReader.h
@@ -2452,8 +2452,9 @@ 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..0fad0b8940273 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,7 @@ 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 +872,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 +1101,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 +1981,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 +2147,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,7 +3520,7 @@ 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) &&
+ if (MergedDCIt != Reader.MergedDeclContexts.end() && !shouldSkipCheckingODR(D) &&
MergedDCIt->second == D->getDeclContext())
Reader.PendingOdrMergeChecks.push_back(D);
diff --git a/clang/lib/Serialization/ASTWriter.cpp b/clang/lib/Serialization/ASTWriter.cpp
index 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@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-error@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
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
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.
ChuanqiXu9
force-pushed
the
SkipODRCheckInGMF
branch
from
046ec7d
to
beb1a4b
Compare
|
I'd still idly vote against adding this flag/support - but if other modules contributors feel it's the right thing to do, I won't stand in the way. |
|
@dwblaikie The flag is for our own use within LLVM test suite. With the previous change, we were unconditionally weakening standards conformance and runtime checking, because there are a bunch of bugs which are believed to be mostly harmless. However, if we do that without some sort of switch, we can't keep testing and fixing things on our end, so that we can some day revert to the correct behavior. |
| 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. |
There was a problem hiding this comment.
I think there are many kinds of issues here and the text might not be conveying the right idea.
- There are some clang bugs where wrong semantic analysis occurs when building AST from pcm, where otherwise parsing from source produces correct results. This is not a problem in ODR checking per se, but it can manifest itself as that, and this ends up being helpful.
- Bugs in the ODR checker itself, where two identical definitions, per the standard, are mistaken as different, or the opposite.
- ODR violations in user code, which can some times be mostly harmless, and some times not.
I read this paragraph as talking about 3 in particular, and giving the idea that the ODR, as specified, ends up barfing at too many harmless violations, and we are finding that it's more trouble than it's worth as we enforce it in more situations.
Regarding MSVC, I don't think we normally would relax standards conformance so broadly in order to be consistent with another compiler. For example, see how we handle compatibility with MSVC regarding delayed template parsing.
There was a problem hiding this comment.
For 1, I don't understand why it is relevant here. I know what you're saying, but I just feel it is not related here. We're talking about ODR checker, right?
For 2, I thought the term false positive implies 2. But maybe it is not impressive. So I tried to rewrite this paragraph to make it more explicit.
For MSVC, it is a simple supporting argument here. I mean, the document is for users, and what I want to say or express is, while all of us know and understand it is not good, but you (the users) don't need to be too panic since MSVC did the same thing.
BTW, for the delayed template parsing, I remember one of the supporting reason to disable that in C++20 is that MSVC disable that in C++20 too. But this is not relevant here.
There was a problem hiding this comment.
For 2, I thought the term false positive implies 2. But maybe it is not impressive. So I tried to rewrite this paragraph to make it more explicit.
That is surprising to me, because the issue you linked in the original commit, #78850, seems to be a case of 3) to me, mostly harmless but nonetheless an ODR violation.
This is not just about the semantics of the resulting program itself, think about debug info: There will be two definitions of fun, with slightly different debug info, because they refer to different declarations for T, even if they resolve to the same type.
Even if the standard wording talks about the definitions consisting of the same series of tokens, this to me is a case of rule-as-written vs rule-as-intended. Ie the tokens not only have to be same textually, but they need to refer to the same entity.
For 1, I don't understand why it is relevant here. I know what you're saying, but I just feel it is not related here. We're talking about ODR checker, right?
See above, but I am also currently internally tracking two separate bugs which are a case of 1), and the ODR checker was just an innocent witness.
There was a problem hiding this comment.
That is surprising to me, because the issue you linked in the original commit, #78850, seems to be a case of 3) to me, mostly harmless but nonetheless an ODR violation.
Yeah, the original reproducer looks like a user's bug. But according to the comment, it should be fine since the function fun() have the same spellings and the looked up type refers to the same entity. The explanation comes from @zygoloid, who is the author of the modules TS.
This is not just about the semantics of the resulting program itself, think about debug info: There will be two definitions of fun, with slightly different debug info, because they refer to different declarations for T, even if they resolve to the same type.
Great insight. I feel this is meaningful. Maybe we need to reach out WG21 to confirm this. Can you access the mailing list of WG21?
For 1, I don't understand why it is relevant here. I know what you're saying, but I just feel it is not related here. We're talking about ODR checker, right?
See above, but I am also currently internally tracking two separate bugs which are a case of 1), and the ODR checker was just an innocent witness.
Sorry. I still don't understand why it is related to the patch itself. I feel you're saying, when we compile pcm to obj, we would do some additional works, then we have divergence between compliing source to obj directly with a two phase compilation (source to pcm, pcm to obj). But I feel it is not related to the patch itself or the document here. (I am open to discuss it. Just want to clarify to make us clear about whay we're saying)
And for the direction to disable ODR checks in GMF as I commented in #79240, I thought it as bad initially. But the explanation from MSVC developer moves me. Since the entities in the GMF are basically from headers. Then it wouldn't be a regression in some level. Also 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.
Also I feel your concern here is about deeper other problems. (e.g., what is ODR violation?) And I want to backport the series patches to 18.x. So I am wondering if we can approve this patch and I can start to backport it. Then we can discuss your concern in other places.
There was a problem hiding this comment.
Great insight. I feel this is meaningful. Maybe we need to reach out WG21 to confirm this. Can you access the mailing list of WG21?
I am not a member yet, but I know a few people in there.
Sorry. I still don't understand why it is related to the patch itself.
So a little background: The issue #78850 that was referenced in the original commit, which seems to be the main motivator for that change, was reported as two parts:
- A real-world-like repro where the user is just including different libstdc++ headers.
- The reduced repro, where there is the real ODR violation we were just arguing about.
I have reason to believe, based on similarity to clang bug I am currently working on, that 2) is unrelated to 1).
Based on the commonality between reproducers of #78850 and the bug I am working on.
They both involve UsingShadowDecls, the merging of which is broken, as I will show on an upcoming patch.
If you look at this from another angle, this reduction was performed by the user / reporter, and reducing a false-positive ODR violation repro is hard, because it's so easy you would introduce a real ODR-violation during a reduction step.
So it's reasonable to double-check / do some due-dilligence here.
So I feel we are taking a drastic step here based on potentially incorrect evidence.
If we can confirm what I am saying, then don't you agree we would be losing the whole casus belli against the GMF ODR checker, and we could go back to square one, revert the whole thing, and take a better look at this?
Even if that is not the only issue, my perception is that other supposed ODR violations could be simple issues like this.
And even if there are ODR violations in shipped system headers, couldn't we:
- Provide a more targeted workaround rather than disabling the whole thing.
- Stand our ground, since the whole C++20 modules thing is new, we can expect users to be using more recent / fixed system headers.
But the explanation from MSVC developer moves me. Since the entities in the GMF are basically from headers. Then it wouldn't be a regression in some level.
I have never seen MSVC source code, so I don't know how relevant it would be for comparison, but I think clang has enough differences in how sema is applied to textual source vs PCM, that this would give me pause and I wouldn't be so sure.
On the other hand, this does not seem to be a regression, since the whole C++20 modules thing is just coming up and we are talking about a new feature and new users.
Also I feel your concern here is about deeper other problems. (e.g., what is ODR violation?) And I want to backport the series patches to 18.x. So I am wondering if we can approve this patch and I can start to backport it. Then we can discuss your concern in other places.
See above. Would you mind waiting a little for my patch, for us to double check we are taking this step with solid footing?
There was a problem hiding this comment.
Great insight. I feel this is meaningful. Maybe we need to reach out WG21 to confirm this. Can you access the mailing list of WG21?
I am not a member yet, but I know a few people in there.
Do you need me to send this? I feel this is the deepest problem in the discussion.
clang bug I am currently working on
Is there an issue report?
So I feel we are taking a drastic step here based on potentially incorrect evidence.
To me, the issue is the straw that broke the camel's back. I've seen too many issue reports saying and complaining the false positive ODR violations publicly and privately. It is a long battle.
If we can confirm what I am saying, then don't you agree we would be losing the whole casus belli against the GMF ODR checker, and we could go back to square one, revert the whole thing, and take a better look at this?
If we can have a stable and correct ODR checker, we have no reasons to not enable it.
Even if that is not the only issue, my perception is that other supposed ODR violations could be simple issues like this.
My experience is "yes, a lot of false positive ODR violations can be fixed by simple patch". The only problem is that there is too many. Or this may be the reason I feel the current ODR checker is not stable enough.
See above. Would you mind waiting a little for my patch, for us to double check we are taking this step with solid footing?
Or let's take a step back. How about always enabling the ODR checker for decls in the GMF but offering a driver flag "-fskip-odr-check-in-gmf"? Then the users can get better experience and offer more precise issue report. Also we don't need to be in the panic of losing standard conformance.
On the other hand, this does not seem to be a regression, since the whole C++20 modules thing is just coming up and we are talking about a new feature and new users.
I think it is saying the users converting a repo from headers to modules.
There was a problem hiding this comment.
Do you need me to send this? I feel this is the deepest problem in the discussion.
Sure, we can move that part of the discussion over there.
Is there an issue report?
Not on the llvm issue tracker yet, I am working on getting issue + patch up soon.
To me, the issue is
the straw that broke the camel's back. I've seen too many issue reports saying and complaining the false positive ODR violations publicly and privately. It is a long battle.
How are we tracking these issues, is there a special tag for them? We could take a better look, attack this systematically.
My experience is "yes, a lot of false positive ODR violations can be fixed by simple patch". The only problem is that there is too many. Or this may be the reason I feel the current ODR checker is not stable enough.
Right, but reducing these issues takes a lot of effort. On the other hand, it's possible they all reduce to a few simple cases.
This is my perception so far, working on converting a medium size code base.
Or let's take a step back. How about always enabling the ODR checker for decls in the GMF but offering a driver flag "-fskip-odr-check-in-gmf"? Then the users can get better experience and offer more precise issue report. Also we don't need to be in the panic of losing standard conformance.
Yeah, one concern is losing the bug reports.
While we do offer driver flags to facilitate testing experimental features, which I think C++20 modules falls under,
this may be too technical detail to expose to users.
So I will go ahead and approve this, even though I feel this might have been too hasty, as long as we are running the clang test suite as before, then we can make progress in fixing all the problems and circle back to always enabling the checker.
I think it is saying the users converting a repo from headers to modules.
Right, but that's not a regression.
There was a problem hiding this comment.
Sure, we can move that part of the discussion over there.
I'll add you as CC if they agree to do so. I remember there is policy to forbid that. But I am not sure.
Not on the llvm issue tracker yet, I am working on getting issue + patch up soon.
Got it. Not bad. But generally an issue can let us know what's going on and maybe we meet such issues too.
How are we tracking these issues, is there a special tag for them? We could take a better look, attack this systematically.
No. All of them are falling into the clang:modules label. I did a just-in-time search and I found:
- [C++20] [Modules] <ranges> cannot be included in more than one module #61317
- [C++20] [Modules] Cannot use concept with class template argument deduction in global module fragment #60486
- [C++20] [Modules] Try to treat
B<A>andB<NS::A>as the same type in using declarations #63595 - [C++20] [Modules] The lambda in the requires expression prevents us to merge concepts across modules #63544
- [c++20][Modules] incorrect error regarding ambiguous specialization #62943
(Maybe there are more)
Note that not all of them fails due to bugs in ODR checker. Some of them fails due to bugs in other places and the ODR checker just fires it. Although the result is still false positive ODR violation diagnostics.
And there are another camp about, import-before-include (#61465). From the perspective of implementors, they are not the same thing. But from the perspective of users, it has a same feelings.
Right, but reducing these issues takes a lot of effort. On the other hand, it's possible they all reduce to a few simple cases.
This is my perception so far, working on converting a medium size code base.
Same here.
So I will go ahead and approve this, even though I feel this might have been too hasty, as long as we are running the clang test suite as before, then we can make progress in fixing all the problems and circle back to always enabling the checker.
Thanks. It is always easy to enable the check by default any time.
Yeah, as @mizvekov said, this is intended to be a transparent change to users. (unless the users are testing volunteers, which is highly welcomed) |
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.
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.
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.
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.
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.
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.
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.
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.
|
I accidentially found this PR also fixes an issue I encountered. Before: https://godbolt.org/z/E6cehd3oh After: https://godbolt.org/z/MMfq84rcK and the false error was |
Close #79240
Cite the comment from @mizvekov in
//github.com//issues/79240:
This patch follows the suggestion:
-fskip-odr-check-in-gmfwhich is by default off, so that the every existing test will still be tested with checking ODR violations.-fskip-odr-check-in-gmfin the driver to keep the behavior we intended.-Xclang -fno-skip-odr-check-in-gmfto get the existing behavior.