[clang] c843c92 - [clang] Require strict matches for defines for PCH in GCC style directories
Martin Storsjö via cfe-commits
cfe-commits at lists.llvm.org
Wed Aug 10 12:47:58 PDT 2022
Author: Martin Storsjö
Date: 2022-08-10T22:47:27+03:00
New Revision: c843c921a1a385bb805b2338d980436c94f83f19
URL: https://github.com/llvm/llvm-project/commit/c843c921a1a385bb805b2338d980436c94f83f19
DIFF: https://github.com/llvm/llvm-project/commit/c843c921a1a385bb805b2338d980436c94f83f19.diff
LOG: [clang] Require strict matches for defines for PCH in GCC style directories
When clang includes a PCH, it tolerates some amount of differences
between the defines used when creating and when including the PCH
- this seems to be intentionally allowed in
c379c072405f39bca1d3552408fc0427328e8b6d (and later extended in
b63687519610a73dd565be1fec28332211b4df5b).
When using a PCH (or when picking a PCH out of a directory containing
multiple candidates) Clang used to accept the header if there were
defines on the command line when creating the PCH that are missing
when using the PCH, or vice versa, defines only set when using the
PCH.
The only cases where Clang explicitly rejected the use of a PCH
is if there was an explicit conflict between the options, e.g.
-DFOO=1 vs -DFOO=2, or -DFOO vs -UFOO.
The latter commit added a FIXME that we really should check whether
mismatched defines actually were used somewhere in the PCH, so that
the define would affect the outcome. This FIXME has stood unaddressed
since 2012.
This differs from GCC, which rejects PCH files if the defines differ
at all.
When explicitly including a single PCH file, the relaxed policy
of allowing minor differences is harmless for correct use cases
(but may fail to diagnose mismtaches), and potentially allow using
PCHs in wider cases (where the user intentionally know that the
differences in defines are harmless for the PCH).
However, for GCC style PCH directories, with a directory containing
multiple PCH variants and the compiler should pick the correct match
out of them, Clang's relaxed logic was problematic. The directory
could contain two otherwise identical PCHs, but one built with -DFOO
and one without. When attempting to include a PCH and iterating over
the candidates in the directory, Clang would essentially pick the
first one out of the two, even if there existed a better, exact
match in the directory.
Keep the relaxed checking when specificlly including one named
PCH file, but require strict matches when trying to pick the right
candidate out of a GCC style directory with alternatives.
This fixes https://github.com/lhmouse/mcfgthread/issues/63.
Differential Revision: https://reviews.llvm.org/D126676
Added:
Modified:
clang/docs/ReleaseNotes.rst
clang/include/clang/Serialization/ASTReader.h
clang/lib/Frontend/FrontendAction.cpp
clang/lib/Serialization/ASTReader.cpp
clang/test/PCH/pch-dir.c
Removed:
################################################################################
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index a37120ba2d56c..3b1d2a412f459 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -89,6 +89,13 @@ Improvements to Clang's diagnostics
language modes. It may be downgraded to a warning with
``-Wno-error=incompatible-function-pointer-types`` or disabled entirely with
``-Wno-implicit-function-pointer-types``.
+- When including a PCH from a GCC style directory with multiple alternative PCH
+ files, Clang now requires all defines set on the command line while generating
+ the PCH and when including it to match. This matches GCC's behaviour.
+ Previously Clang would tolerate defines to be set when creating the PCH but
+ missing when used, or vice versa. This makes sure that Clang picks the
+ correct one, where it previously would consider multiple ones as potentially
+ acceptable (and erroneously use whichever one is tried first).
Non-comprehensive list of changes in this release
-------------------------------------------------
diff --git a/clang/include/clang/Serialization/ASTReader.h b/clang/include/clang/Serialization/ASTReader.h
index 7cdebefaf7a99..25b14f4dfa422 100644
--- a/clang/include/clang/Serialization/ASTReader.h
+++ b/clang/include/clang/Serialization/ASTReader.h
@@ -1738,7 +1738,8 @@ class ASTReader
const LangOptions &LangOpts,
const TargetOptions &TargetOpts,
const PreprocessorOptions &PPOpts,
- StringRef ExistingModuleCachePath);
+ StringRef ExistingModuleCachePath,
+ bool RequireStrictOptionMatches = false);
/// Returns the suggested contents of the predefines buffer,
/// which contains a (typically-empty) subset of the predefines
diff --git a/clang/lib/Frontend/FrontendAction.cpp b/clang/lib/Frontend/FrontendAction.cpp
index 7b07ab948f643..53cb48d2de9e6 100644
--- a/clang/lib/Frontend/FrontendAction.cpp
+++ b/clang/lib/Frontend/FrontendAction.cpp
@@ -772,7 +772,7 @@ bool FrontendAction::BeginSourceFile(CompilerInstance &CI,
if (ASTReader::isAcceptableASTFile(
Dir->path(), FileMgr, CI.getPCHContainerReader(),
CI.getLangOpts(), CI.getTargetOpts(), CI.getPreprocessorOpts(),
- SpecificModuleCachePath)) {
+ SpecificModuleCachePath, /*RequireStrictOptionMatches=*/true)) {
PPOpts.ImplicitPCHInclude = std::string(Dir->path());
Found = true;
break;
diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp
index abe14b68e84d4..efd05a7764e09 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -624,20 +624,28 @@ collectMacroDefinitions(const PreprocessorOptions &PPOpts,
}
}
+enum OptionValidation {
+ OptionValidateNone,
+ OptionValidateContradictions,
+ OptionValidateStrictMatches,
+};
+
/// Check the preprocessor options deserialized from the control block
/// against the preprocessor options in an existing preprocessor.
///
/// \param Diags If non-null, produce diagnostics for any mismatches incurred.
-/// \param Validate If true, validate preprocessor options. If false, allow
-/// macros defined by \p ExistingPPOpts to override those defined by
-/// \p PPOpts in SuggestedPredefines.
-static bool checkPreprocessorOptions(const PreprocessorOptions &PPOpts,
- const PreprocessorOptions &ExistingPPOpts,
- DiagnosticsEngine *Diags,
- FileManager &FileMgr,
- std::string &SuggestedPredefines,
- const LangOptions &LangOpts,
- bool Validate = true) {
+/// \param Validation If set to OptionValidateNone, ignore
diff erences in
+/// preprocessor options. If set to OptionValidateContradictions,
+/// require that options passed both in the AST file and on the command
+/// line (-D or -U) match, but tolerate options missing in one or the
+/// other. If set to OptionValidateContradictions, require that there
+/// are no
diff erences in the options between the two.
+static bool checkPreprocessorOptions(
+ const PreprocessorOptions &PPOpts,
+ const PreprocessorOptions &ExistingPPOpts, DiagnosticsEngine *Diags,
+ FileManager &FileMgr, std::string &SuggestedPredefines,
+ const LangOptions &LangOpts,
+ OptionValidation Validation = OptionValidateContradictions) {
// Check macro definitions.
MacroDefinitionsMap ASTFileMacros;
collectMacroDefinitions(PPOpts, ASTFileMacros);
@@ -653,7 +661,15 @@ static bool checkPreprocessorOptions(const PreprocessorOptions &PPOpts,
// Check whether we know anything about this macro name or not.
llvm::StringMap<std::pair<StringRef, bool /*IsUndef*/>>::iterator Known =
ASTFileMacros.find(MacroName);
- if (!Validate || Known == ASTFileMacros.end()) {
+ if (Validation == OptionValidateNone || Known == ASTFileMacros.end()) {
+ if (Validation == OptionValidateStrictMatches) {
+ // If strict matches are requested, don't tolerate any extra defines on
+ // the command line that are missing in the AST file.
+ if (Diags) {
+ Diags->Report(diag::err_pch_macro_def_undef) << MacroName << true;
+ }
+ return true;
+ }
// FIXME: Check whether this identifier was referenced anywhere in the
// AST file. If so, we should reject the AST file. Unfortunately, this
// information isn't in the control block. What shall we do about it?
@@ -684,8 +700,10 @@ static bool checkPreprocessorOptions(const PreprocessorOptions &PPOpts,
// If the macro was #undef'd in both, or if the macro bodies are identical,
// it's fine.
- if (Existing.second || Existing.first == Known->second.first)
+ if (Existing.second || Existing.first == Known->second.first) {
+ ASTFileMacros.erase(Known);
continue;
+ }
// The macro bodies
diff er; complain.
if (Diags) {
@@ -694,9 +712,20 @@ static bool checkPreprocessorOptions(const PreprocessorOptions &PPOpts,
}
return true;
}
+ if (Validation == OptionValidateStrictMatches) {
+ // If strict matches are requested, don't tolerate any extra defines in
+ // the AST file that are missing on the command line.
+ for (const auto &MacroName : ASTFileMacros.keys()) {
+ if (Diags) {
+ Diags->Report(diag::err_pch_macro_def_undef) << MacroName << false;
+ }
+ return true;
+ }
+ }
// Check whether we're using predefines.
- if (PPOpts.UsePredefines != ExistingPPOpts.UsePredefines && Validate) {
+ if (PPOpts.UsePredefines != ExistingPPOpts.UsePredefines &&
+ Validation != OptionValidateNone) {
if (Diags) {
Diags->Report(diag::err_pch_undef) << ExistingPPOpts.UsePredefines;
}
@@ -705,7 +734,8 @@ static bool checkPreprocessorOptions(const PreprocessorOptions &PPOpts,
// Detailed record is important since it is used for the module cache hash.
if (LangOpts.Modules &&
- PPOpts.DetailedRecord != ExistingPPOpts.DetailedRecord && Validate) {
+ PPOpts.DetailedRecord != ExistingPPOpts.DetailedRecord &&
+ Validation != OptionValidateNone) {
if (Diags) {
Diags->Report(diag::err_pch_pp_detailed_record) << PPOpts.DetailedRecord;
}
@@ -766,13 +796,9 @@ bool SimpleASTReaderListener::ReadPreprocessorOptions(
const PreprocessorOptions &PPOpts,
bool Complain,
std::string &SuggestedPredefines) {
- return checkPreprocessorOptions(PPOpts,
- PP.getPreprocessorOpts(),
- nullptr,
- PP.getFileManager(),
- SuggestedPredefines,
- PP.getLangOpts(),
- false);
+ return checkPreprocessorOptions(PPOpts, PP.getPreprocessorOpts(), nullptr,
+ PP.getFileManager(), SuggestedPredefines,
+ PP.getLangOpts(), OptionValidateNone);
}
/// Check the header search options deserialized from the control block
@@ -5088,16 +5114,19 @@ namespace {
const PreprocessorOptions &ExistingPPOpts;
std::string ExistingModuleCachePath;
FileManager &FileMgr;
+ bool StrictOptionMatches;
public:
SimplePCHValidator(const LangOptions &ExistingLangOpts,
const TargetOptions &ExistingTargetOpts,
const PreprocessorOptions &ExistingPPOpts,
- StringRef ExistingModuleCachePath, FileManager &FileMgr)
+ StringRef ExistingModuleCachePath, FileManager &FileMgr,
+ bool StrictOptionMatches)
: ExistingLangOpts(ExistingLangOpts),
ExistingTargetOpts(ExistingTargetOpts),
ExistingPPOpts(ExistingPPOpts),
- ExistingModuleCachePath(ExistingModuleCachePath), FileMgr(FileMgr) {}
+ ExistingModuleCachePath(ExistingModuleCachePath), FileMgr(FileMgr),
+ StrictOptionMatches(StrictOptionMatches) {}
bool ReadLanguageOptions(const LangOptions &LangOpts, bool Complain,
bool AllowCompatibleDifferences) override {
@@ -5122,9 +5151,11 @@ namespace {
bool ReadPreprocessorOptions(const PreprocessorOptions &PPOpts,
bool Complain,
std::string &SuggestedPredefines) override {
- return checkPreprocessorOptions(PPOpts, ExistingPPOpts, /*Diags=*/nullptr,
- FileMgr, SuggestedPredefines,
- ExistingLangOpts);
+ return checkPreprocessorOptions(
+ PPOpts, ExistingPPOpts, /*Diags=*/nullptr, FileMgr,
+ SuggestedPredefines, ExistingLangOpts,
+ StrictOptionMatches ? OptionValidateStrictMatches
+ : OptionValidateContradictions);
}
};
@@ -5401,9 +5432,11 @@ bool ASTReader::isAcceptableASTFile(StringRef Filename, FileManager &FileMgr,
const LangOptions &LangOpts,
const TargetOptions &TargetOpts,
const PreprocessorOptions &PPOpts,
- StringRef ExistingModuleCachePath) {
+ StringRef ExistingModuleCachePath,
+ bool RequireStrictOptionMatches) {
SimplePCHValidator validator(LangOpts, TargetOpts, PPOpts,
- ExistingModuleCachePath, FileMgr);
+ ExistingModuleCachePath, FileMgr,
+ RequireStrictOptionMatches);
return !readASTFileControlBlock(Filename, FileMgr, PCHContainerRdr,
/*FindModuleFileExtensions=*/false,
validator,
diff --git a/clang/test/PCH/pch-dir.c b/clang/test/PCH/pch-dir.c
index f8b8c05878f45..afdd7b1f4f307 100644
--- a/clang/test/PCH/pch-dir.c
+++ b/clang/test/PCH/pch-dir.c
@@ -6,7 +6,7 @@
// RUN: %clang -x c++-header -std=c++98 %S/pch-dir.h -o %t.h.gch/cpp.gch
// RUN: %clang -include %t.h -DFOO=foo -fsyntax-only %s -Xclang -print-stats 2> %t.clog
// RUN: FileCheck -check-prefix=CHECK-C %s < %t.clog
-// RUN: %clang -include %t.h -DFOO=bar -DBAR=bar -fsyntax-only %s -Xclang -ast-print > %t.cbarlog
+// RUN: %clang -include %t.h -DFOO=bar -fsyntax-only %s -Xclang -ast-print > %t.cbarlog
// RUN: FileCheck -check-prefix=CHECK-CBAR %s < %t.cbarlog
// RUN: %clang -x c++ -include %t.h -std=c++98 -fsyntax-only %s -Xclang -print-stats 2> %t.cpplog
// RUN: FileCheck -check-prefix=CHECK-CPP %s < %t.cpplog
@@ -14,6 +14,11 @@
// RUN: not %clang -x c++ -std=c++11 -include %t.h -fsyntax-only %s 2> %t.cpp11log
// RUN: FileCheck -check-prefix=CHECK-NO-SUITABLE %s < %t.cpp11log
+// RUN: not %clang -include %t.h -fsyntax-only %s 2> %t.missinglog2
+// RUN: FileCheck -check-prefix=CHECK-NO-SUITABLE %s < %t.missinglog2
+// RUN: not %clang -include %t.h -DFOO=foo -DBAR=bar -fsyntax-only %s 2> %t.missinglog2
+// RUN: FileCheck -check-prefix=CHECK-NO-SUITABLE %s < %t.missinglog2
+
// Don't crash if the precompiled header file is missing.
// RUN: not %clang_cc1 -include-pch %t.h.gch -DFOO=baz -fsyntax-only %s -print-stats 2> %t.missinglog
// RUN: FileCheck -check-prefix=CHECK-NO-SUITABLE %s < %t.missinglog
More information about the cfe-commits
mailing list