From ea360412c53fa32213c8c86206ceb6fbabb3ff8a Mon Sep 17 00:00:00 2001 From: Jan Svoboda Date: Wed, 16 Aug 2023 19:28:05 -0700 Subject: [PATCH] [clang][modules] Avoid storing command-line macro definitions into implicitly built PCM files With implicit modules, it's impossible to load a PCM file that was built using different command-line macro definitions. This is guaranteed by the fact that they contribute to the context hash. This means that we don't need to store those macros into PCM files for validation purposes. This patch avoids serializing them in those circumstances, since there's no other use for command-line macro definitions (besides "-module-file-info"). For a typical Apple project, this speeds up the dependency scan by 5.6% and shrinks the cache with scanning PCMs by 26%. Reviewed By: benlangmuir Differential Revision: https://reviews.llvm.org/D158136 --- clang/include/clang/Serialization/ASTReader.h | 10 +- clang/include/clang/Serialization/ASTWriter.h | 8 +- clang/lib/Frontend/ASTUnit.cpp | 3 +- clang/lib/Frontend/FrontendActions.cpp | 8 +- clang/lib/Serialization/ASTReader.cpp | 152 +++++++++--------- clang/lib/Serialization/ASTWriter.cpp | 23 ++- clang/lib/Serialization/GeneratePCH.cpp | 4 +- clang/test/Modules/module_file_info.m | 20 +-- 8 files changed, 127 insertions(+), 101 deletions(-) diff --git a/clang/include/clang/Serialization/ASTReader.h b/clang/include/clang/Serialization/ASTReader.h index 9fa03a7f25dd9..c0e7379cbb1be 100644 --- a/clang/include/clang/Serialization/ASTReader.h +++ b/clang/include/clang/Serialization/ASTReader.h @@ -199,7 +199,7 @@ class ASTReaderListener { /// \returns true to indicate the preprocessor options are invalid, or false /// otherwise. virtual bool ReadPreprocessorOptions(const PreprocessorOptions &PPOpts, - bool Complain, + bool ReadMacros, bool Complain, std::string &SuggestedPredefines) { return false; } @@ -294,7 +294,7 @@ class ChainedASTReaderListener : public ASTReaderListener { StringRef SpecificModuleCachePath, bool Complain) override; bool ReadPreprocessorOptions(const PreprocessorOptions &PPOpts, - bool Complain, + bool ReadMacros, bool Complain, std::string &SuggestedPredefines) override; void ReadCounter(const serialization::ModuleFile &M, unsigned Value) override; @@ -328,7 +328,8 @@ class PCHValidator : public ASTReaderListener { bool AllowCompatibleDifferences) override; bool ReadDiagnosticOptions(IntrusiveRefCntPtr DiagOpts, bool Complain) override; - bool ReadPreprocessorOptions(const PreprocessorOptions &PPOpts, bool Complain, + bool ReadPreprocessorOptions(const PreprocessorOptions &PPOpts, + bool ReadMacros, bool Complain, std::string &SuggestedPredefines) override; bool ReadHeaderSearchOptions(const HeaderSearchOptions &HSOpts, StringRef SpecificModuleCachePath, @@ -349,7 +350,8 @@ class SimpleASTReaderListener : public ASTReaderListener { public: SimpleASTReaderListener(Preprocessor &PP) : PP(PP) {} - bool ReadPreprocessorOptions(const PreprocessorOptions &PPOpts, bool Complain, + bool ReadPreprocessorOptions(const PreprocessorOptions &PPOpts, + bool ReadMacros, bool Complain, std::string &SuggestedPredefines) override; }; diff --git a/clang/include/clang/Serialization/ASTWriter.h b/clang/include/clang/Serialization/ASTWriter.h index 409b56b44bcd4..ecbcb96bf63bd 100644 --- a/clang/include/clang/Serialization/ASTWriter.h +++ b/clang/include/clang/Serialization/ASTWriter.h @@ -143,6 +143,11 @@ class ASTWriter : public ASTDeserializationListener, /// file is up to date, but not otherwise. bool IncludeTimestamps; + /// Indicates whether the AST file being written is an implicit module. + /// If that's the case, we may be able to skip writing some information that + /// are guaranteed to be the same in the importer by the context hash. + bool BuildingImplicitModule = false; + /// Indicates when the AST writing is actively performing /// serialization, rather than just queueing updates. bool WritingAST = false; @@ -571,7 +576,7 @@ class ASTWriter : public ASTDeserializationListener, ASTWriter(llvm::BitstreamWriter &Stream, SmallVectorImpl &Buffer, InMemoryModuleCache &ModuleCache, ArrayRef> Extensions, - bool IncludeTimestamps = true); + bool IncludeTimestamps = true, bool BuildingImplicitModule = false); ~ASTWriter() override; ASTContext &getASTContext() const { @@ -809,6 +814,7 @@ class PCHGenerator : public SemaConsumer { std::shared_ptr Buffer, ArrayRef> Extensions, bool AllowASTWithErrors = false, bool IncludeTimestamps = true, + bool BuildingImplicitModule = false, bool ShouldCacheASTInMemory = false); ~PCHGenerator() override; diff --git a/clang/lib/Frontend/ASTUnit.cpp b/clang/lib/Frontend/ASTUnit.cpp index 6dc375691ff07..0abcd6b44144b 100644 --- a/clang/lib/Frontend/ASTUnit.cpp +++ b/clang/lib/Frontend/ASTUnit.cpp @@ -586,7 +586,8 @@ class ASTInfoCollector : public ASTReaderListener { return false; } - bool ReadPreprocessorOptions(const PreprocessorOptions &PPOpts, bool Complain, + bool ReadPreprocessorOptions(const PreprocessorOptions &PPOpts, + bool ReadMacros, bool Complain, std::string &SuggestedPredefines) override { this->PPOpts = PPOpts; return false; diff --git a/clang/lib/Frontend/FrontendActions.cpp b/clang/lib/Frontend/FrontendActions.cpp index 01a1c50ad5aa8..39e40bb3214c2 100644 --- a/clang/lib/Frontend/FrontendActions.cpp +++ b/clang/lib/Frontend/FrontendActions.cpp @@ -140,7 +140,8 @@ GeneratePCHAction::CreateASTConsumer(CompilerInstance &CI, StringRef InFile) { CI.getPreprocessor(), CI.getModuleCache(), OutputFile, Sysroot, Buffer, FrontendOpts.ModuleFileExtensions, CI.getPreprocessorOpts().AllowPCHWithCompilerErrors, - FrontendOpts.IncludeTimestamps, +CI.getLangOpts().CacheGeneratedPCH)); + FrontendOpts.IncludeTimestamps, FrontendOpts.BuildingImplicitModule, + +CI.getLangOpts().CacheGeneratedPCH)); Consumers.push_back(CI.getPCHContainerWriter().CreatePCHContainerGenerator( CI, std::string(InFile), OutputFile, std::move(OS), Buffer)); @@ -202,6 +203,7 @@ GenerateModuleAction::CreateASTConsumer(CompilerInstance &CI, +CI.getFrontendOpts().AllowPCMWithCompilerErrors, /*IncludeTimestamps=*/ +CI.getFrontendOpts().BuildingImplicitModule, + /*BuildingImplicitModule=*/+CI.getFrontendOpts().BuildingImplicitModule, /*ShouldCacheASTInMemory=*/ +CI.getFrontendOpts().BuildingImplicitModule)); Consumers.push_back(CI.getPCHContainerWriter().CreatePCHContainerGenerator( @@ -728,7 +730,7 @@ namespace { } bool ReadPreprocessorOptions(const PreprocessorOptions &PPOpts, - bool Complain, + bool ReadMacros, bool Complain, std::string &SuggestedPredefines) override { Out.indent(2) << "Preprocessor options:\n"; DUMP_BOOLEAN(PPOpts.UsePredefines, @@ -736,7 +738,7 @@ namespace { DUMP_BOOLEAN(PPOpts.DetailedRecord, "Uses detailed preprocessing record (for indexing)"); - if (!PPOpts.Macros.empty()) { + if (ReadMacros) { Out.indent(4) << "Predefined macros:\n"; } diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp index 1d9b01cc462b2..0e43d6875cb22 100644 --- a/clang/lib/Serialization/ASTReader.cpp +++ b/clang/lib/Serialization/ASTReader.cpp @@ -208,11 +208,12 @@ bool ChainedASTReaderListener::ReadHeaderSearchOptions( } bool ChainedASTReaderListener::ReadPreprocessorOptions( - const PreprocessorOptions &PPOpts, bool Complain, + const PreprocessorOptions &PPOpts, bool ReadMacros, bool Complain, std::string &SuggestedPredefines) { - return First->ReadPreprocessorOptions(PPOpts, Complain, + return First->ReadPreprocessorOptions(PPOpts, ReadMacros, Complain, SuggestedPredefines) || - Second->ReadPreprocessorOptions(PPOpts, Complain, SuggestedPredefines); + Second->ReadPreprocessorOptions(PPOpts, ReadMacros, Complain, + SuggestedPredefines); } void ChainedASTReaderListener::ReadCounter(const serialization::ModuleFile &M, @@ -668,68 +669,70 @@ enum OptionValidation { /// 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, + const PreprocessorOptions &ExistingPPOpts, bool ReadMacros, + DiagnosticsEngine *Diags, FileManager &FileMgr, + std::string &SuggestedPredefines, const LangOptions &LangOpts, OptionValidation Validation = OptionValidateContradictions) { - // Check macro definitions. - MacroDefinitionsMap ASTFileMacros; - collectMacroDefinitions(PPOpts, ASTFileMacros); - MacroDefinitionsMap ExistingMacros; - SmallVector ExistingMacroNames; - collectMacroDefinitions(ExistingPPOpts, ExistingMacros, &ExistingMacroNames); - - for (unsigned I = 0, N = ExistingMacroNames.size(); I != N; ++I) { + if (ReadMacros) { + // Check macro definitions. + MacroDefinitionsMap ASTFileMacros; + collectMacroDefinitions(PPOpts, ASTFileMacros); + MacroDefinitionsMap ExistingMacros; + SmallVector ExistingMacroNames; + collectMacroDefinitions(ExistingPPOpts, ExistingMacros, + &ExistingMacroNames); + + for (unsigned I = 0, N = ExistingMacroNames.size(); I != N; ++I) { // Dig out the macro definition in the existing preprocessor options. StringRef MacroName = ExistingMacroNames[I]; std::pair Existing = ExistingMacros[MacroName]; - // Check whether we know anything about this macro name or not. - llvm::StringMap>::iterator Known = - ASTFileMacros.find(MacroName); - 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. + // Check whether we know anything about this macro name or not. + llvm::StringMap>::iterator Known = + ASTFileMacros.find(MacroName); + 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? + + if (Existing.second) { + SuggestedPredefines += "#undef "; + SuggestedPredefines += MacroName.str(); + SuggestedPredefines += '\n'; + } else { + SuggestedPredefines += "#define "; + SuggestedPredefines += MacroName.str(); + SuggestedPredefines += ' '; + SuggestedPredefines += Existing.first.str(); + SuggestedPredefines += '\n'; + } + continue; + } + + // If the macro was defined in one but undef'd in the other, we have a + // conflict. + if (Existing.second != Known->second.second) { if (Diags) { - Diags->Report(diag::err_pch_macro_def_undef) << MacroName << true; + Diags->Report(diag::err_pch_macro_def_undef) + << MacroName << Known->second.second; } 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? - - if (Existing.second) { - SuggestedPredefines += "#undef "; - SuggestedPredefines += MacroName.str(); - SuggestedPredefines += '\n'; - } else { - SuggestedPredefines += "#define "; - SuggestedPredefines += MacroName.str(); - SuggestedPredefines += ' '; - SuggestedPredefines += Existing.first.str(); - SuggestedPredefines += '\n'; - } - continue; - } - // If the macro was defined in one but undef'd in the other, we have a - // conflict. - if (Existing.second != Known->second.second) { - if (Diags) { - Diags->Report(diag::err_pch_macro_def_undef) - << MacroName << Known->second.second; + // 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) { + ASTFileMacros.erase(Known); + continue; } - return true; - } - - // 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) { - ASTFileMacros.erase(Known); - continue; - } // The macro bodies differ; complain. if (Diags) { @@ -745,7 +748,7 @@ static bool checkPreprocessorOptions( if (Diags) { Diags->Report(diag::err_pch_macro_def_undef) << MacroName << false; } - return true; + return true;} } } @@ -807,24 +810,22 @@ static bool checkPreprocessorOptions( } bool PCHValidator::ReadPreprocessorOptions(const PreprocessorOptions &PPOpts, - bool Complain, + bool ReadMacros, bool Complain, std::string &SuggestedPredefines) { const PreprocessorOptions &ExistingPPOpts = PP.getPreprocessorOpts(); - return checkPreprocessorOptions(PPOpts, ExistingPPOpts, - Complain? &Reader.Diags : nullptr, - PP.getFileManager(), - SuggestedPredefines, - PP.getLangOpts()); + return checkPreprocessorOptions( + PPOpts, ExistingPPOpts, ReadMacros, Complain ? &Reader.Diags : nullptr, + PP.getFileManager(), SuggestedPredefines, PP.getLangOpts()); } bool SimpleASTReaderListener::ReadPreprocessorOptions( - const PreprocessorOptions &PPOpts, - bool Complain, - std::string &SuggestedPredefines) { - return checkPreprocessorOptions(PPOpts, PP.getPreprocessorOpts(), nullptr, - PP.getFileManager(), SuggestedPredefines, - PP.getLangOpts(), OptionValidateNone); + const PreprocessorOptions &PPOpts, bool ReadMacros, bool Complain, + std::string &SuggestedPredefines) { + return checkPreprocessorOptions(PPOpts, PP.getPreprocessorOpts(), ReadMacros, + nullptr, PP.getFileManager(), + SuggestedPredefines, PP.getLangOpts(), + OptionValidateNone); } /// Check the header search options deserialized from the control block @@ -5234,10 +5235,10 @@ namespace { } bool ReadPreprocessorOptions(const PreprocessorOptions &PPOpts, - bool Complain, + bool ReadMacros, bool Complain, std::string &SuggestedPredefines) override { return checkPreprocessorOptions( - PPOpts, ExistingPPOpts, /*Diags=*/nullptr, FileMgr, + PPOpts, ExistingPPOpts, ReadMacros, /*Diags=*/nullptr, FileMgr, SuggestedPredefines, ExistingLangOpts, StrictOptionMatches ? OptionValidateStrictMatches : OptionValidateContradictions); @@ -6018,10 +6019,13 @@ bool ASTReader::ParsePreprocessorOptions(const RecordData &Record, unsigned Idx = 0; // Macro definitions/undefs - for (unsigned N = Record[Idx++]; N; --N) { - std::string Macro = ReadString(Record, Idx); - bool IsUndef = Record[Idx++]; - PPOpts.Macros.push_back(std::make_pair(Macro, IsUndef)); + bool ReadMacros = Record[Idx++]; + if (ReadMacros) { + for (unsigned N = Record[Idx++]; N; --N) { + std::string Macro = ReadString(Record, Idx); + bool IsUndef = Record[Idx++]; + PPOpts.Macros.push_back(std::make_pair(Macro, IsUndef)); + } } // Includes @@ -6040,7 +6044,7 @@ bool ASTReader::ParsePreprocessorOptions(const RecordData &Record, PPOpts.ObjCXXARCStandardLibrary = static_cast(Record[Idx++]); SuggestedPredefines.clear(); - return Listener.ReadPreprocessorOptions(PPOpts, Complain, + return Listener.ReadPreprocessorOptions(PPOpts, ReadMacros, Complain, SuggestedPredefines); } diff --git a/clang/lib/Serialization/ASTWriter.cpp b/clang/lib/Serialization/ASTWriter.cpp index 8b2f4527b57b8..9bcb3417e3861 100644 --- a/clang/lib/Serialization/ASTWriter.cpp +++ b/clang/lib/Serialization/ASTWriter.cpp @@ -1497,11 +1497,19 @@ void ASTWriter::WriteControlBlock(Preprocessor &PP, ASTContext &Context, Record.clear(); const PreprocessorOptions &PPOpts = PP.getPreprocessorOpts(); - // Macro definitions. - Record.push_back(PPOpts.Macros.size()); - for (unsigned I = 0, N = PPOpts.Macros.size(); I != N; ++I) { - AddString(PPOpts.Macros[I].first, Record); - Record.push_back(PPOpts.Macros[I].second); + // If we're building an implicit module with a context hash, the importer is + // guaranteed to have the same macros defined on the command line. Skip + // writing them. + bool SkipMacros = BuildingImplicitModule && !HSOpts.DisableModuleHash; + bool WriteMacros = !SkipMacros; + Record.push_back(WriteMacros); + if (WriteMacros) { + // Macro definitions. + Record.push_back(PPOpts.Macros.size()); + for (unsigned I = 0, N = PPOpts.Macros.size(); I != N; ++I) { + AddString(PPOpts.Macros[I].first, Record); + Record.push_back(PPOpts.Macros[I].second); + } } // Includes @@ -4506,9 +4514,10 @@ ASTWriter::ASTWriter(llvm::BitstreamWriter &Stream, SmallVectorImpl &Buffer, InMemoryModuleCache &ModuleCache, ArrayRef> Extensions, - bool IncludeTimestamps) + bool IncludeTimestamps, bool BuildingImplicitModule) : Stream(Stream), Buffer(Buffer), ModuleCache(ModuleCache), - IncludeTimestamps(IncludeTimestamps) { + IncludeTimestamps(IncludeTimestamps), + BuildingImplicitModule(BuildingImplicitModule) { for (const auto &Ext : Extensions) { if (auto Writer = Ext->createExtensionWriter(*this)) ModuleFileExtensionWriters.push_back(std::move(Writer)); diff --git a/clang/lib/Serialization/GeneratePCH.cpp b/clang/lib/Serialization/GeneratePCH.cpp index 6ec5c42e8b82a..601a24b4aec46 100644 --- a/clang/lib/Serialization/GeneratePCH.cpp +++ b/clang/lib/Serialization/GeneratePCH.cpp @@ -25,11 +25,11 @@ PCHGenerator::PCHGenerator( StringRef OutputFile, StringRef isysroot, std::shared_ptr Buffer, ArrayRef> Extensions, bool AllowASTWithErrors, bool IncludeTimestamps, - bool ShouldCacheASTInMemory) + bool BuildingImplicitModule, bool ShouldCacheASTInMemory) : PP(PP), OutputFile(OutputFile), isysroot(isysroot.str()), SemaPtr(nullptr), Buffer(std::move(Buffer)), Stream(this->Buffer->Data), Writer(Stream, this->Buffer->Data, ModuleCache, Extensions, - IncludeTimestamps), + IncludeTimestamps, BuildingImplicitModule), AllowASTWithErrors(AllowASTWithErrors), ShouldCacheASTInMemory(ShouldCacheASTInMemory) { this->Buffer->IsComplete = false; diff --git a/clang/test/Modules/module_file_info.m b/clang/test/Modules/module_file_info.m index 4cd30cf177bd2..f4edb4a82d176 100644 --- a/clang/test/Modules/module_file_info.m +++ b/clang/test/Modules/module_file_info.m @@ -1,14 +1,15 @@ // UNSUPPORTED: -zos, -aix @import DependsOnModule; -// RUN: rm -rf %t %t-obj +// RUN: rm -rf %t %t-obj %t-hash // RUN: %clang_cc1 -w -Wunused -fmodules -fmodule-format=raw -fimplicit-module-maps -fdisable-module-hash -fmodules-cache-path=%t -F %S/Inputs -DBLARG -DWIBBLE=WOBBLE -fmodule-feature myfeature %s -// RUN: %clang_cc1 -module-file-info %t/DependsOnModule.pcm | FileCheck %s -// RUN: %clang_cc1 -module-file-info %t/DependsOnModule.pcm | FileCheck %s --check-prefix=RAW +// RUN: %clang_cc1 -module-file-info %t/DependsOnModule.pcm | FileCheck %s --check-prefixes=RAW,CHECK,MACROS // RUN: %clang_cc1 -w -Wunused -fmodules -fmodule-format=obj -fimplicit-module-maps -fdisable-module-hash -fmodules-cache-path=%t-obj -F %S/Inputs -DBLARG -DWIBBLE=WOBBLE -fmodule-feature myfeature %s -// RUN: %clang_cc1 -module-file-info %t-obj/DependsOnModule.pcm | FileCheck %s -// RUN: %clang_cc1 -module-file-info %t-obj/DependsOnModule.pcm | FileCheck %s --check-prefix=OBJ +// RUN: %clang_cc1 -module-file-info %t-obj/DependsOnModule.pcm | FileCheck %s --check-prefixes=OBJ,CHECK,MACROS + +// RUN: %clang_cc1 -w -Wunused -fmodules -fmodule-format=obj -fimplicit-module-maps -fmodules-cache-path=%t-hash -F %S/Inputs -DBLARG -DWIBBLE=WOBBLE -fmodule-feature myfeature %s +// RUN: %clang_cc1 -module-file-info %t-hash/*/DependsOnModule-*.pcm | FileCheck %s --check-prefixes=OBJ,CHECK,NO_MACROS // RAW: Module format: raw // OBJ: Module format: obj @@ -16,7 +17,7 @@ // CHECK: Module name: DependsOnModule // CHECK: Module map file: {{.*}}DependsOnModule.framework{{[/\\]}}module.map -// CHECK: Imports module 'Module': {{.*}}Module.pcm +// CHECK: Imports module 'Module': {{.*}}Module{{.*}}.pcm // CHECK: Language options: // CHECK: C99: Yes @@ -42,9 +43,10 @@ // CHECK: Preprocessor options: // CHECK: Uses compiler/target-specific predefines [-undef]: Yes // CHECK: Uses detailed preprocessing record (for indexing): No -// CHECK: Predefined macros: -// CHECK: -DBLARG -// CHECK: -DWIBBLE=WOBBLE +// NO_MACROS-NOT: Predefined macros: +// MACROS: Predefined macros: +// MACROS-NEXT: -DBLARG +// MACROS-NEXT: -DWIBBLE=WOBBLE // CHECK: Input file: {{.*}}module.map // CHECK-NEXT: Input file: {{.*}}module_private.map // CHECK-NEXT: Input file: {{.*}}DependsOnModule.h