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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
