mstorsjo updated this revision to Diff 443461.
mstorsjo added a comment.
Reduce the scope and impact of the change: When telling Clang to include a
specifically named PCH file, keep tolerating the same set of mismatches as
before. (Clang has been tolerating such differences for over 10 years, and
there are a number of tests specifically for such fuzzy matches.) When using a
GCC style directory with multiple alternative PCH files, require an exact
match, to avoid the case where multiple ones are acceptable and Clang
erroneously picks the first seemingly acceptable one.
In this form, the patch no longer breaks other testcases than the one that is
meant to be fixed/changed.
This still has a small risk of breaking someone's setup, but as this makes
Clang match GCC's behaviour for the compatible option, the risk is probably
quite small.
(A more comprehensive solution to avoid breaking that case, would be to iterate
over the PCH directory, first requiring a strict match, and if none is found,
iterate over the directory again, tolerating inexact matches like before. But I
don't think that's necessary.)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D126676/new/
https://reviews.llvm.org/D126676
Files:
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
Index: clang/test/PCH/pch-dir.c
===================================================================
--- clang/test/PCH/pch-dir.c
+++ 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
Index: clang/lib/Serialization/ASTReader.cpp
===================================================================
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -637,7 +637,8 @@
FileManager &FileMgr,
std::string &SuggestedPredefines,
const LangOptions &LangOpts,
- bool Validate = true) {
+ bool Validate = true,
+ bool ValidateStrict = false) {
// Check macro definitions.
MacroDefinitionsMap ASTFileMacros;
collectMacroDefinitions(PPOpts, ASTFileMacros);
@@ -654,6 +655,14 @@
llvm::StringMap<std::pair<StringRef, bool /*IsUndef*/>>::iterator Known =
ASTFileMacros.find(MacroName);
if (!Validate || Known == ASTFileMacros.end()) {
+ if (ValidateStrict) {
+ // 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 +693,10 @@
// 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 differ; complain.
if (Diags) {
@@ -694,6 +705,16 @@
}
return true;
}
+ if (ValidateStrict) {
+ // 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) {
@@ -5137,16 +5158,19 @@
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 {
@@ -5173,7 +5197,8 @@
std::string &SuggestedPredefines) override {
return checkPreprocessorOptions(PPOpts, ExistingPPOpts, /*Diags=*/nullptr,
FileMgr, SuggestedPredefines,
- ExistingLangOpts);
+ ExistingLangOpts, /*Validate=*/true,
+ /*ValidateStrict=*/StrictOptionMatches);
}
};
@@ -5450,9 +5475,11 @@
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,
Index: clang/lib/Frontend/FrontendAction.cpp
===================================================================
--- clang/lib/Frontend/FrontendAction.cpp
+++ clang/lib/Frontend/FrontendAction.cpp
@@ -772,7 +772,7 @@
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;
Index: clang/include/clang/Serialization/ASTReader.h
===================================================================
--- clang/include/clang/Serialization/ASTReader.h
+++ clang/include/clang/Serialization/ASTReader.h
@@ -1738,7 +1738,8 @@
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
Index: clang/docs/ReleaseNotes.rst
===================================================================
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -279,6 +279,13 @@
unevaluated operands of a ``typeid`` expression, as they are now
modeled correctly in the CFG. This fixes
`Issue 21668 <https://github.com/llvm/llvm-project/issues/21668>`_.
+- 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
-------------------------------------------------
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
