[Serialization] Downgrade error to warning for inconsistent language flags #117840
Conversation
ChuanqiXu9
added
the
skip-precommit-approval
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-modules Author: Chuanqi Xu (ChuanqiXu9) ChangesResent from #115416 to debug the CI failure Patch is 22.45 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/117840.diff 13 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 6c40e48e2f49b3..30ae869d70c0fb 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -590,6 +590,8 @@ Improvements to Clang's diagnostics
- Fixed a false negative ``-Wunused-private-field`` diagnostic when a defaulted comparison operator is defined out of class (#GH116961).
+- Clang now downgrades the inconsistent language options between modules to warnings instead of errors.
+
Improvements to Clang's time-trace
----------------------------------
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td
index df9bf94b5d0398..287efb38bf176e 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -557,6 +557,7 @@ def ModuleLock : DiagGroup<"module-lock">;
def ModuleBuild : DiagGroup<"module-build">;
def ModuleImport : DiagGroup<"module-import">;
def ModuleConflict : DiagGroup<"module-conflict">;
+def ModuleMismatchedOption : DiagGroup<"module-mismatched-option">;
def ModuleFileExtension : DiagGroup<"module-file-extension">;
def ModuleIncludeDirectiveTranslation : DiagGroup<"module-include-translation">;
def RoundTripCC1Args : DiagGroup<"round-trip-cc1-args">;
diff --git a/clang/include/clang/Basic/DiagnosticSerializationKinds.td b/clang/include/clang/Basic/DiagnosticSerializationKinds.td
index 3914d3930bec79..4e1aff20617963 100644
--- a/clang/include/clang/Basic/DiagnosticSerializationKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSerializationKinds.td
@@ -36,10 +36,12 @@ def err_ast_file_targetopt_feature_mismatch : Error<
"%select{AST file '%1' was|current translation unit is}0 compiled with the target "
"feature '%2' but the %select{current translation unit is|AST file '%1' was}0 "
"not">;
-def err_ast_file_langopt_mismatch : Error<"%0 was %select{disabled|enabled}1 in "
- "AST file '%3' but is currently %select{disabled|enabled}2">;
-def err_ast_file_langopt_value_mismatch : Error<
- "%0 differs in AST file '%1' vs. current file">;
+def warn_ast_file_langopt_mismatch : Warning<"%0 was %select{disabled|enabled}1 in "
+ "AST file '%3' but is currently %select{disabled|enabled}2">,
+ InGroup<ModuleMismatchedOption>;
+def warn_ast_file_langopt_value_mismatch : Warning<
+ "%0 differs in AST file '%1' vs. current file">,
+ InGroup<ModuleMismatchedOption>;
def err_ast_file_diagopt_mismatch : Error<"%0 is currently enabled, but was not in "
"the AST file '%1'">;
def err_ast_file_modulecache_mismatch : Error<"AST file '%2' was compiled with module cache "
diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp
index ec85fad3389a1c..de24f65f970a53 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -279,9 +279,7 @@ ASTReaderListener::~ASTReaderListener() = default;
/// \param Diags If non-NULL, diagnostics will be emitted via this engine.
/// \param AllowCompatibleDifferences If true, differences between compatible
/// language options will be permitted.
-///
-/// \returns true if the languagae options mis-match, false otherwise.
-static bool checkLanguageOptions(const LangOptions &LangOpts,
+static void checkLanguageOptions(const LangOptions &LangOpts,
const LangOptions &ExistingLangOpts,
StringRef ModuleFilename,
DiagnosticsEngine *Diags,
@@ -290,30 +288,27 @@ static bool checkLanguageOptions(const LangOptions &LangOpts,
if (ExistingLangOpts.Name != LangOpts.Name) { \
if (Diags) { \
if (Bits == 1) \
- Diags->Report(diag::err_ast_file_langopt_mismatch) \
+ Diags->Report(diag::warn_ast_file_langopt_mismatch) \
<< Description << LangOpts.Name << ExistingLangOpts.Name \
<< ModuleFilename; \
else \
- Diags->Report(diag::err_ast_file_langopt_value_mismatch) \
+ Diags->Report(diag::warn_ast_file_langopt_value_mismatch) \
<< Description << ModuleFilename; \
} \
- return true; \
}
#define VALUE_LANGOPT(Name, Bits, Default, Description) \
if (ExistingLangOpts.Name != LangOpts.Name) { \
if (Diags) \
- Diags->Report(diag::err_ast_file_langopt_value_mismatch) \
+ Diags->Report(diag::warn_ast_file_langopt_value_mismatch) \
<< Description << ModuleFilename; \
- return true; \
}
#define ENUM_LANGOPT(Name, Type, Bits, Default, Description) \
if (ExistingLangOpts.get##Name() != LangOpts.get##Name()) { \
if (Diags) \
- Diags->Report(diag::err_ast_file_langopt_value_mismatch) \
+ Diags->Report(diag::warn_ast_file_langopt_value_mismatch) \
<< Description << ModuleFilename; \
- return true; \
}
#define COMPATIBLE_LANGOPT(Name, Bits, Default, Description) \
@@ -335,24 +330,21 @@ static bool checkLanguageOptions(const LangOptions &LangOpts,
if (ExistingLangOpts.ModuleFeatures != LangOpts.ModuleFeatures) {
if (Diags)
- Diags->Report(diag::err_ast_file_langopt_value_mismatch)
+ Diags->Report(diag::warn_ast_file_langopt_value_mismatch)
<< "module features" << ModuleFilename;
- return true;
}
if (ExistingLangOpts.ObjCRuntime != LangOpts.ObjCRuntime) {
if (Diags)
- Diags->Report(diag::err_ast_file_langopt_value_mismatch)
+ Diags->Report(diag::warn_ast_file_langopt_value_mismatch)
<< "target Objective-C runtime" << ModuleFilename;
- return true;
}
if (ExistingLangOpts.CommentOpts.BlockCommandNames !=
LangOpts.CommentOpts.BlockCommandNames) {
if (Diags)
- Diags->Report(diag::err_ast_file_langopt_value_mismatch)
+ Diags->Report(diag::warn_ast_file_langopt_value_mismatch)
<< "block command names" << ModuleFilename;
- return true;
}
// Sanitizer feature mismatches are treated as compatible differences. If
@@ -378,11 +370,8 @@ static bool checkLanguageOptions(const LangOptions &LangOpts,
}
#include "clang/Basic/Sanitizers.def"
}
- return true;
}
}
-
- return false;
}
/// Compare the given set of target options against an existing set of
@@ -459,9 +448,10 @@ bool PCHValidator::ReadLanguageOptions(const LangOptions &LangOpts,
StringRef ModuleFilename, bool Complain,
bool AllowCompatibleDifferences) {
const LangOptions &ExistingLangOpts = PP.getLangOpts();
- return checkLanguageOptions(LangOpts, ExistingLangOpts, ModuleFilename,
- Complain ? &Reader.Diags : nullptr,
- AllowCompatibleDifferences);
+ checkLanguageOptions(LangOpts, ExistingLangOpts, ModuleFilename,
+ Complain ? &Reader.Diags : nullptr,
+ AllowCompatibleDifferences);
+ return false;
}
bool PCHValidator::ReadTargetOptions(const TargetOptions &TargetOpts,
@@ -5398,8 +5388,9 @@ namespace {
bool ReadLanguageOptions(const LangOptions &LangOpts,
StringRef ModuleFilename, bool Complain,
bool AllowCompatibleDifferences) override {
- return checkLanguageOptions(ExistingLangOpts, LangOpts, ModuleFilename,
- nullptr, AllowCompatibleDifferences);
+ checkLanguageOptions(ExistingLangOpts, LangOpts, ModuleFilename, nullptr,
+ AllowCompatibleDifferences);
+ return false;
}
bool ReadTargetOptions(const TargetOptions &TargetOpts,
diff --git a/clang/test/Modules/explicit-build-missing-files.cpp b/clang/test/Modules/explicit-build-missing-files.cpp
index 3ea881d34c6b28..4682ede5e08089 100644
--- a/clang/test/Modules/explicit-build-missing-files.cpp
+++ b/clang/test/Modules/explicit-build-missing-files.cpp
@@ -33,7 +33,7 @@
// RUN: %clang_cc1 -fmodules -I %t -fmodule-file=%t/b.pcm %s
// RUN: not %clang_cc1 -fmodules -I %t -fmodule-file=%t/a.pcm %s -DERRORS 2>&1 | FileCheck %s --check-prefix=MISSING-B
// RUN: %clang_cc1 -fmodules -I %t -fmodule-file=%t/a.pcm -fmodule-map-file=%t/modulemap.moved %s
-// RUN: not %clang_cc1 -fmodules -I %t -fmodule-file=%t/a.pcm -fmodule-map-file=%t/modulemap.moved -std=c++1z %s
+// RUN: %clang_cc1 -fmodules -I %t -fmodule-file=%t/a.pcm -fmodule-map-file=%t/modulemap.moved -std=c++1z %s
// RUN: %clang_cc1 -fmodules -I %t -fmodule-file=%t/a.pcm -fmodule-map-file=%t/modulemap.moved -std=c++1z -Wno-module-file-config-mismatch %s -Db=a
// RUN: rm %t/a.h
// RUN: %clang_cc1 -fmodules -I %t -fmodule-file=%t/a.pcm %s -verify
diff --git a/clang/test/Modules/load_failure.c b/clang/test/Modules/load_failure.c
index 662b39b6f1874f..bc0abb46bdc2bc 100644
--- a/clang/test/Modules/load_failure.c
+++ b/clang/test/Modules/load_failure.c
@@ -11,11 +11,11 @@
// RUN: not %clang_cc1 -fmodules -fimplicit-module-maps -x objective-c -fmodules-cache-path=%t -I %S/Inputs -fdisable-module-hash %s -DNONEXISTENT 2>&1 | FileCheck -check-prefix=CHECK-NONEXISTENT %s
// CHECK-NONEXISTENT: load_failure.c:2:9: fatal error: module 'load_nonexistent' not found
-// RUN: not %clang_cc1 -fmodules -fimplicit-module-maps -x objective-c -fmodules-cache-path=%t -I %S/Inputs -fdisable-module-hash %s -DFAILURE 2> %t.out
-// RUN: FileCheck -check-prefix=CHECK-FAILURE %s < %t.out
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -x objective-c -fmodules-cache-path=%t -I %S/Inputs -fdisable-module-hash %s -DFAILURE 2> %t.out
+// RUN: FileCheck -check-prefix=CHECK-WARN %s < %t.out
// FIXME: Clean up diagnostic text below and give it a location
-// CHECK-FAILURE: error: C99 was disabled in AST file '{{.*}}load_failure.pcm' but is currently enabled
+// CHECK-WARN: warning: C99 was disabled in AST file '{{.*}}load_failure.pcm' but is currently enabled
// FIXME: When we have a syntax for modules in C, use that.
diff --git a/clang/test/Modules/mismatch-diagnostics.cpp b/clang/test/Modules/mismatch-diagnostics.cpp
index dffd4b46a678e5..745d6ee802f8f9 100644
--- a/clang/test/Modules/mismatch-diagnostics.cpp
+++ b/clang/test/Modules/mismatch-diagnostics.cpp
@@ -3,31 +3,55 @@
// RUN: split-file %s %t
// RUN: mkdir -p %t/prebuilt_modules
//
-// RUN: %clang_cc1 -triple %itanium_abi_triple \
-// RUN: -std=c++20 -fprebuilt-module-path=%t/prebuilt-modules \
-// RUN: -emit-module-interface -pthread -DBUILD_MODULE \
-// RUN: %t/mismatching_module.cppm -o \
+// RUN: %clang_cc1 -triple %itanium_abi_triple \
+// RUN: -std=c++20 -fprebuilt-module-path=%t/prebuilt-modules \
+// RUN: -emit-module-interface -pthread -DBUILD_MODULE \
+// RUN: %t/mismatching_module.cppm -o \
// RUN: %t/prebuilt_modules/mismatching_module.pcm
//
-// RUN: not %clang_cc1 -triple %itanium_abi_triple -std=c++20 \
-// RUN: -fprebuilt-module-path=%t/prebuilt_modules -DCHECK_MISMATCH \
-// RUN: %t/use.cpp 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++20 \
+// RUN: -fprebuilt-module-path=%t/prebuilt_modules -DCHECK_MISMATCH \
+// RUN: %t/use.cpp 2>&1 | FileCheck %t/use.cpp
// Test again with reduced BMI.
-// RUN: %clang_cc1 -triple %itanium_abi_triple \
-// RUN: -std=c++20 -fprebuilt-module-path=%t/prebuilt-modules \
-// RUN: -emit-reduced-module-interface -pthread -DBUILD_MODULE \
-// RUN: %t/mismatching_module.cppm -o \
+// RUN: %clang_cc1 -triple %itanium_abi_triple \
+// RUN: -std=c++20 -fprebuilt-module-path=%t/prebuilt-modules \
+// RUN: -emit-reduced-module-interface -pthread -DBUILD_MODULE \
+// RUN: %t/mismatching_module.cppm -o \
// RUN: %t/prebuilt_modules/mismatching_module.pcm
//
-// RUN: not %clang_cc1 -triple %itanium_abi_triple -std=c++20 \
-// RUN: -fprebuilt-module-path=%t/prebuilt_modules -DCHECK_MISMATCH \
-// RUN: %t/use.cpp 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++20 \
+// RUN: -fprebuilt-module-path=%t/prebuilt_modules -DCHECK_MISMATCH \
+// RUN: %t/use.cpp 2>&1 | FileCheck %t/use.cpp
+//
+// RUN: %clang_cc1 -triple %itanium_abi_triple \
+// RUN: -std=c++20 -fprebuilt-module-path=%t/prebuilt-modules \
+// RUN: -emit-module-interface -pthread -DBUILD_MODULE \
+// RUN: %t/mismatching_module.cppm -o \
+// RUN: %t/prebuilt_modules/mismatching_module.pcm
+//
+// RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++20 \
+// RUN: -fprebuilt-module-path=%t/prebuilt_modules -DCHECK_MISMATCH \
+// RUN: -Wno-module-mismatched-option %t/use.cpp 2>&1 | FileCheck %t/use.cpp \
+// RUN: --check-prefix=NOWARN --allow-empty
+
+// Test again with reduced BMI.
+// RUN: %clang_cc1 -triple %itanium_abi_triple \
+// RUN: -std=c++20 -fprebuilt-module-path=%t/prebuilt-modules \
+// RUN: -emit-reduced-module-interface -pthread -DBUILD_MODULE \
+// RUN: %t/mismatching_module.cppm -o \
+// RUN: %t/prebuilt_modules/mismatching_module.pcm
+//
+// RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++20 \
+// RUN: -fprebuilt-module-path=%t/prebuilt_modules -DCHECK_MISMATCH \
+// RUN: -Wno-module-mismatched-option %t/use.cpp 2>&1 | FileCheck %t/use.cpp \
+// RUN: --check-prefix=NOWARN --allow-empty
//--- mismatching_module.cppm
export module mismatching_module;
//--- use.cpp
import mismatching_module;
-// CHECK: error: POSIX thread support was enabled in AST file '{{.*[/|\\\\]}}mismatching_module.pcm' but is currently disabled
-// CHECK-NEXT: module file {{.*[/|\\\\]}}mismatching_module.pcm cannot be loaded due to a configuration mismatch with the current compilation
+// CHECK: warning: POSIX thread support was enabled in AST file '{{.*[/|\\\\]}}mismatching_module.pcm' but is currently disabled
+
+// NOWARN-NOT: warning
diff --git a/clang/test/Modules/module-feature.m b/clang/test/Modules/module-feature.m
index 4926d26515f860..bbc9b0220f761c 100644
--- a/clang/test/Modules/module-feature.m
+++ b/clang/test/Modules/module-feature.m
@@ -6,9 +6,9 @@
// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -fimplicit-module-maps -fmodule-feature f2 -fmodule-feature f1 -F %S/Inputs %s -Rmodule-build 2>&1 | FileCheck %s -allow-empty -check-prefix=ALREADY_BUILT
// ALREADY_BUILT-NOT: building module
-// Errors if we try to force the load.
+// Warns if we try to force the load.
// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t.nohash -fimplicit-module-maps -fdisable-module-hash -fmodule-feature f1 -fmodule-feature f2 -F %S/Inputs %s -verify -Rmodule-build
-// RUN: not %clang_cc1 -fmodules -fmodules-cache-path=%t.nohash -fimplicit-module-maps -fdisable-module-hash -fmodule-feature f2 -F %S/Inputs %s 2>&1 | FileCheck %s -check-prefix=DIFFERS
-// DIFFERS: error: module features differs
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t.nohash -fimplicit-module-maps -fdisable-module-hash -fmodule-feature f2 -F %S/Inputs %s 2>&1 | FileCheck %s -check-prefix=DIFFERS
+// DIFFERS: warning: module features differs
@import Module; // expected-remark {{building module 'Module'}} expected-remark {{finished}}
diff --git a/clang/test/Modules/pr62359.cppm b/clang/test/Modules/pr62359.cppm
index 7d9d3eec26cca7..f459d46f378618 100644
--- a/clang/test/Modules/pr62359.cppm
+++ b/clang/test/Modules/pr62359.cppm
@@ -3,9 +3,9 @@
// RUN: split-file %s %t
//
// RUN: %clang_cc1 -std=c++20 -emit-module-interface %t/Hello.cppm -o %t/Hello.pcm
-// RUN: not %clang_cc1 -std=c++20 -fopenmp %t/use.cpp -fmodule-file=hello=%t/Hello.pcm -fsyntax-only \
+// RUN: %clang_cc1 -std=c++20 -fopenmp %t/use.cpp -fmodule-file=hello=%t/Hello.pcm -fsyntax-only \
// RUN: 2>&1 | FileCheck %t/use.cpp
-// RUN: not %clang_cc1 -std=c++20 -fopenmp %t/use2.cpp -fmodule-file=hello=%t/Hello.pcm -fsyntax-only \
+// RUN: %clang_cc1 -std=c++20 -fopenmp %t/use2.cpp -fmodule-file=hello=%t/Hello.pcm -fsyntax-only \
// RUN: 2>&1 | FileCheck %t/use2.cpp
//
// RUN: %clang_cc1 -std=c++20 -fopenmp -emit-module-interface %t/Hello.cppm -o %t/Hello.pcm
@@ -18,9 +18,9 @@
// RUN: split-file %s %t
//
// RUN: %clang_cc1 -std=c++20 -emit-reduced-module-interface %t/Hello.cppm -o %t/Hello.pcm
-// RUN: not %clang_cc1 -std=c++20 -fopenmp %t/use.cpp -fmodule-file=hello=%t/Hello.pcm -fsyntax-only \
+// RUN: %clang_cc1 -std=c++20 -fopenmp %t/use.cpp -fmodule-file=hello=%t/Hello.pcm -fsyntax-only \
// RUN: 2>&1 | FileCheck %t/use.cpp
-// RUN: not %clang_cc1 -std=c++20 -fopenmp %t/use2.cpp -fmodule-file=hello=%t/Hello.pcm -fsyntax-only \
+// RUN: %clang_cc1 -std=c++20 -fopenmp %t/use2.cpp -fmodule-file=hello=%t/Hello.pcm -fsyntax-only \
// RUN: 2>&1 | FileCheck %t/use2.cpp
//
// RUN: %clang_cc1 -std=c++20 -fopenmp -emit-reduced-module-interface %t/Hello.cppm -o %t/Hello.pcm
@@ -56,4 +56,3 @@ int use2() {
}
// CHECK: OpenMP{{.*}}differs in AST file '{{.*}}Hello.pcm' vs. current file
-// CHECK: use of undeclared identifier 'pragma'
diff --git a/clang/test/Modules/prebuilt-implicit-modules.m b/clang/test/Modules/prebuilt-implicit-modules.m
index dc4fb55cb17a55..1bae214eb901f7 100644
--- a/clang/test/Modules/prebuilt-implicit-modules.m
+++ b/clang/test/Modules/prebuilt-implicit-modules.m
@@ -25,7 +25,7 @@
// RUN: mkdir -p %t2
// RUN: %clang_cc1 -x objective-c -fmodules %S/Inputs/prebuilt-implicit-module/module.modulemap -emit-module -fmodule-name=module_a -fmodules-cache-path=%t
// RUN: %clang_cc1 -x objective-c -fmodules %S/Inputs/prebuilt-implicit-module/module.modulemap -emit-module -fmodule-name=module_a -o %t/module_a.pcm -fno-signed-char
-// RUN: not %clang_cc1 -x objective-c %s -I%S/Inputs/prebuilt-implicit-module -fmodules -fmodule-map-file=%S/Inputs/prebuilt-implicit-module/module.modulemap -fprebuilt-implicit-modules -fprebuilt-module-path=%t -fmodules-cache-path=%t2
+// RUN: %clang_cc1 -x objective-c %s -I%S/Inputs/prebuilt-implicit-module -fmodules -fmodule-map-file=%S/Inputs/prebuilt-implicit-module/module.modulemap -fprebuilt-implicit-modules -fprebuilt-module-path=%t -fmodules-cache-path=%t2
// RUN: find %t2 -name "module_a*.pcm" | not grep module_a
// expected-no-diagnostics
diff --git a/clang/test/PCH/arc.m b/clang/test/PCH/arc.m
index e4ad71a469b956..d714512b48d7d7 100644
--- a/clang/test/PCH/arc.m
+++ b/clang/test/PCH/arc.m
@@ -6,10 +6,10 @@
// RUN: %clang_cc1 -emit-pch -fblocks -triple x86_64-apple-darwin11 -fobjc-arc -x objective-c-header -o %t %S/Inputs/arc.h
// RUN: %clang_cc1 -fblocks -triple x86_64-apple-darwin11 -fobjc-arc -include-pch %t -emit-llvm-only %s
-// Test error when pch's -fobjc-arc state is different.
-// RUN: not %clang_cc1 -fblocks -triple x86_64-apple-darwin11 -include-pch %t -emit-llvm-only %s 2>&1 | FileCheck -check-prefix=CHECK-ERR1 %s
+// Test warning when pch's -fobjc-arc state is different.
+// RUN: %clang_cc1 -fblocks -triple x86_64-apple-darwin11 -include-pch %t -emit-llvm-only %s 2>&1 | FileCheck -check-prefix=CHECK-ERR1 %s ...
[truncated]
|
ChuanqiXu9
force-pushed
the
DowngradeErrorToWarning
branch
from
01a6606 to
8b17949
Compare
…lvm#115416) There were many many "voices" about the too strict flags checking in modules. Although they rarely challenge this, maybe due to they respect to the compiler implementation details. But from my point of view, there are cases it is "fine" to have different flags. Especially we're too conservative to mark almost language options in `clang/include/clang/Basic/LangOptions.def` as incompatible options (see the comments in the front of the file). In my understanding, this should come from PCH initially since it is natural to ask your headers to be compiled with the same flags with your TU. And then, when Apple and Google goes to implement clang module, they don't challenge it too since they have a closed world where they have a strong control over the ecosystem so that they can make it consistent. Yes, consistency is great and ODR violation are awful. But this is the world we're living today. This is the C++'s ecosystem in the open ended world. Image a situation that we're using a third party module and we add a new option to our library, then the build bails out! THIS IS SUPER ANNOYING. And makes it non practical to make a modular C++ ecosystem. ( This was discussed many times in SG15. And the consensus is, the build systems should generate different BMI based on different flags. But this manner can't avoid ODR violation completely and it would add the times of module files that need to be built, which may kill the benefit of faster compilation of modules. However, I think the build systems may need to do the similar things in the end of the day. Considering libc++'s hardening mechanism (https://libcxx.llvm.org/Hardening.html). So the conclusion of the paragraph is, although this seems related to build systems, I think they are actually unrelated story. ) I think we should give our users a chance to disable such checks. It is theoretically unsafe. But we've done our job to tell the users that it **MAY** be bad. Then I feel it is C++-ish to give users more freedom even if they may shoot their foot. This shouldn't change any thing. Users who want previous behavior can get it easily by `-Werror=`.
ChuanqiXu9
force-pushed
the
DowngradeErrorToWarning
branch
from
8b17949 to
1f9c2a9
Compare
There was a problem hiding this comment.
I don't think the discussion here has run its course
#115416
In particular, I agree with @boris-kolpackov that this is likely to lead to hard-to understand ODR violations issues, which is the exact things modules were meant to solve.
If there was evidence of deployment challenges, we could consider a warning-defaulting-to-an-error, but no less, imo.
(But then again, deployment issues in a projects that mixes compilation flags is expected and it's very unclear to be there is value in deploying modules without first ensuring a consistent build environment)
You can take a look at https://www.youtube.com/watch?v=flu-f6SDnOE
Then it blocks people to use modules more. Users can't control the the flags from other libraries. I don't think we should force users for our too conservative decisions. As I said there are many false positive error messages which doesn't affect the process actually.
There will always be potential ODR violation issues. Since the build systems will generate different BMIs for the same module unit with different flags. It is just another way to work around the current problem. I feel it is really wishful thinking to ban this and hope we can be in a more safe world. I think it may only stop more people using modules and change nothing else. Again, please give users a chance to choose what they like. |
ChuanqiXu9
changed the title
ChuanqiXu9
changed the title
ChuanqiXu9
removed
the
skip-precommit-approval
cor3ntin
requested review from
Bigcheese,
AaronBallman,
erichkeane and
iains
Yeah, I had explicitly asked for a review from @Bigcheese before that landed, so I was surprised to see that get merged.
False positive error diagnostics? Warnings can have false positives; errors are not allowed to and that suggests we have some serious issues elsewhere to address first.
Maybe I'm misunderstanding something but that sounds like the exact opposite of how we usually operate. We typically do aim for improved safety and if that means people don't use modules as much, that's fine -- it's better for us to be too restrictive and then relax those restrictions in the future than to be too permissive and have to support that forever due to users relying on it. |
I misread it. I thought it wasn't message to me. It would be clear if you ask me for waiting more time for @Bigcheese
Yes. This was my point. Maybe I didn't make it clear. All the llvm-project/clang/include/clang/Basic/LangOptions.def Lines 380 to 381 in 700d9ac LANGOPT.
Maybe the above paragraph can explain it better. The point is false-positive diagnostics. And I fully agree safe is good and ODR violation is bad. I just thought we can transfer the right to the users. People who want it can have |
|
I think this is too risky to become the default. We could map out what are the most user affecting compat issues here, and start from there, without opening the floodgates all at once. |
I'll keep that in mind for next time!
I see, so because we no longer allow inconsistent options when importing, we now issue a lot of error diagnostics. But for many of the options, differences between TUs should have no impact on modules anyway, and so those errors are false positives. Is that about correct? If I'm right, then what about a different way forward: update the individual options to specify whether it is an error, a warning, or silently fine for the option to be inconsistent across module boundaries, the emit diagnostics (or not) as appropriate for each option. It's more up-front work, but it means we stop issue false positive errors without losing diagnostic fidelity. (Note, we may find we want to reorganize Options.td so it's easier for us to specify "this entire block of options are ones for which module inconsistency is an error".) |
Yes.
The problem may be about the amount of work. Although I am not sure how far we can make it in practice, it is technically doable. Some other thoughts about false-positive error diagnostics. I am wondering, if we don't want false-positive error diagnostics at all, then we have to do what I proposed in this PR (to downgrade errors to warnings for inconsistent flags). Otherwise, we can always construct at least one example that it is false-positive to diagnose the inconsistency. e.g., then is it really matter to compile |
To overcome this, we need to finer-grained analysis in Sema (or ASTReader on AST really) to emit errors to emit the true positive error message. A simple true positive error example is, then if we compile So my conclusion is, if we want to emit error without false-positive diagnostics, the current mechanism is too coarse-grained. The current mechanism only emit diagnostics based on comparing flags. But if we want to avoid false-positive diagnostics really, we must have finer-grained mechanism which can analysis the AST really. And we'd better to avoid to emit errors in the current mechanism to avoid the false positive error diagnostics. And my feeling is, we're too overly defensive when hearing this have may increase the ODR violation chances. But from my perspective, primarily from users, I do feel it is not comfortable that it bans too many use cases but not preventing the ODR violations after all. (both GCC and MSVC didn't make so strict rules as clang did. And I think this is not designed at all. It is just... what inherits from the era of PCH and we didn't change it) |
|
I think going down this path is going to cause real issues for users. There's a reason this behavior was added in the first place for Clang modules, as Clang makes assumptions about the shape of the AST which can lead to crashes in some cases. For example, in real world code that uses the stdlib you are incredibly unlikely to actually be able to mix code that uses different language versions. We've gone through a lot of the LANGOPTs already to try and see if we can demote the ones we see to BENIGN, and lots of them end up having macros or AST differences that make this not possible. There are some that could be changed to work by delaying their effect until later, but that hasn't been done yet. For instance we've even had issues with allowing differences in optimization level because it sets the There's also a problem with the global module fragment and header units which make these kinds of issues even worse, you really can't get the preprocessor state wrong or you get real ODR issues. We've discussed this a bunch in the Tooling study group in the committee, and the general consensus is that build systems need to arrange to build BMIs with the correct settings, and it's entirely expected that the BMI you use for a named module may not be the same one that was built when actually building the .o for that TU. Really we would have to remove the preprocessor from C++ to be able to get away from this. |
👏 Maybe we can get a magic |
For SG15 related things, I discussed them in #115416. In short, they're similar but different stories. Since we can't model
If there are crashes, I think we'd better to fix these crahes.
I still think this is related but another topic. We are already in this case now. We're already facing the potential ODR violations caused by preprocessors no matter we enabled modules or not. This is the same before and after this patch. |
From https://build2.org/build2/doc/build2-build-system-manual.xhtml#cxx-modules-install :
|
I see where your logic is coming from -- we have false positive error diagnostics, we know those are unacceptable, so if we downgrade everything to warnings then we no longer have false positive error diagnostics and that's an improvement. However, I think that trades false positive error diagnostics for a different kind of unacceptable behavior, which are crashes/assertions/miscompiles.
I agree, it's a significant undertaking. We could think of downgrading to warnings as an incremental step, but I think leaving these as errors does two things for us: 1) is the safest default for users in that it avoids crashes and miscompiles, 2) is the more frustrating behavior for users, which adds extra reasons for us to fix the underlying issue with more urgency. |
OK, although I still think it is much better to downgrade the coarse-grained error to user controllable warnings, I'd like to follow the communities' consensus. I'll try to refactor LangOptions.def to try to make it more precise. BTW, do you think it is acceptable to add a behavior under a flag? e.g., |
|
@Bigcheese anyone working on "given a clang invocation, produce a hash that tells you whether the options build compatible modules"? I think that would be neat. |
You can get a hash from Although in our experience we've hit enough deep issues with allowing differences that we now remove/canonicalize all -cc1 flags that we know don't impact the AST and then just hash the command line, although part of that removal and canonicalization requires scanning. It also includes the dependency graph up to that module. There are a lot of cases where |
Resent from #115416 to debug the CI failure