diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst --- a/clang/docs/ReleaseNotes.rst +++ b/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 ------------------------------------------------- diff --git a/clang/include/clang/Serialization/ASTReader.h b/clang/include/clang/Serialization/ASTReader.h --- a/clang/include/clang/Serialization/ASTReader.h +++ b/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 diff --git a/clang/lib/Frontend/FrontendAction.cpp b/clang/lib/Frontend/FrontendAction.cpp --- a/clang/lib/Frontend/FrontendAction.cpp +++ b/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; diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp --- a/clang/lib/Serialization/ASTReader.cpp +++ b/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>::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, diff --git a/clang/test/PCH/pch-dir.c b/clang/test/PCH/pch-dir.c --- 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