[Serialization] Downgrade inconsistent flags from erros to warnings #115416
Conversation
This file contains hidden or 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 @llvm/pr-subscribers-clang-modules Author: Chuanqi Xu (ChuanqiXu9) ChangesThere 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 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. ( 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 Patch is 22.43 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/115416.diff 13 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 30bcb6313b6ade..65aa8acd7b8263 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -511,6 +511,8 @@ Improvements to Clang's diagnostics
- Clang now diagnoses ``[[deprecated]]`` attribute usage on local variables (#GH90073).
+- 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 72eada50a56cc9..ba42ddfc37f35a 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -555,6 +555,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 79615dc3c018ea..1c94e8e9a212a9 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,
@@ -5401,8 +5391,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
// RUN: %clang_cc1 -emit-pch -fblocks -triple x...
[truncated]
|
I think this is the correct behavior if the incompatibility caused by said option could cause issues down the line (compile errors, linking errors, runtime errors), all of which will be a lot more confusing and harder to understand to the user. Also, perhaps relevant: https://github.com/build2/HOWTO/blob/master/entries/compile-options-in-buildfile.md |
Yeah, we will still emit warnings to let users know what happened. It's the user's responsibility if the user silent the warning. Let the user pay what they do. And there are a lot of false positive hard errors which are very annoying. In the early days, the users may not be able to explore modules due to these false positive errors are my major concerns. Update: after reading your links, I realized you're not against the change but saying build2 is good. Agreed : ) |
|
I don't know, I feel like you are just kicking the can down the road: if the user sees a harmless warning continuously, they will suppress this warning, and then won't see the harmful ones in the future. But then again we do this all the time in C++, so I guess it's par for the course.
Well, not exactly. We have better chance in |
True if we're talking about the C++'s ecosystem. My point is more based on the users. The current mechanism indeed stops more people trying modules. And I feel people's patience for modules are decreasing. Some people may prefer to not change the existing code than moving to modules. So I prefer to leave the chance to them. All of us know it's not ideal. But the C++'s world is never ideal.
Yes, build2's design will help here for sure. And there are some escaping case like |
|
I'd like to land this in next week if no objection comes in. |
|
@Bigcheese -- I'd appreciate an explicit review from you on this as a component maintainer. |
ChuanqiXu9
force-pushed
the
DowngradeErrorToWarning
branch
from
6e07163 to
48230bc
Compare
ChuanqiXu9
force-pushed
the
DowngradeErrorToWarning
branch
from
48230bc to
168d155
Compare
|
Let's continue the discussion in post commit review if any. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/154/builds/8111 Here is the relevant piece of the build log for the reference |
ChuanqiXu9
added a commit
to ChuanqiXu9/llvm-project
that referenced
this pull request
Nov 27, 2024
…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=`.
|
Resent #117840 to debug the CI failure |
ChuanqiXu9
added a commit
to ChuanqiXu9/llvm-project
that referenced
this pull request
Nov 27, 2024
…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=`.
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.
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.defas 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=.