mstorsjo updated this revision to Diff 448708.
mstorsjo added a comment.
Update the parameters to the internal `checkPreprocessorOptions` function to
use a three-state enum instead of having two separate `bool` flags.
The 15.x release already has been branched, but I'd like to have this
backported and included if possible (the regression risk should be quite small
as long as it's restricted to GCC PCH directories). To handle that, I'd omit
the release notes when pushing to the main branch (as the release notes
document differs between the branches) and manually add that to the release
branch, if this is accepted for backport.
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
@@ -624,20 +624,28 @@
}
}
+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 differences 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 differences 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 @@
// 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 @@
// 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,9 +712,20 @@
}
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 @@
// 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 @@
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
@@ -5138,16 +5164,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 {
@@ -5172,9 +5201,11 @@
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);
}
};
@@ -5451,9 +5482,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
@@ -52,6 +52,13 @@
Improvements to Clang's diagnostics
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+- 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
