mstorsjo updated this revision to Diff 451434.
mstorsjo added a comment.
Rebased, to fix (trivial) conflicts in the release notes file.
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
@@ -5088,16 +5114,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 {
@@ -5122,9 +5151,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);
}
};
@@ -5401,9 +5432,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
@@ -85,6 +85,13 @@
- ``-Wbitfield-constant-conversion`` now diagnoses implicit truncation when 1 is
assigned to a 1-bit signed integer bitfield. This fixes
`Issue 53253 <https://github.com/llvm/llvm-project/issues/53253>`_.
+- 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
