llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang Author: Jan Svoboda (jansvoboda11) <details> <summary>Changes</summary> Deserialization of the `DIAGNOSTIC_OPTIONS` and `HEADER_SEARCH_PATHS` records is slow and done for every transitively loaded PCM. Deserialization of these records cannot be skipped, because the words are VBR6-encoded and we don't store the length of the entire record. We could either turn them into binary blobs that can be skipped during deserialization, or skip writing them altogether. This patch takes the latter approach, since these records are not necessary in scanning PCMs. We don't make any guarantees about diagnostics reported by the scanner, and we always have the same header search paths due to strict context hashing. --- Full diff: https://github.com/llvm/llvm-project/pull/69975.diff 5 Files Affected: - (modified) clang/include/clang/Driver/Options.td (+4) - (modified) clang/include/clang/Lex/HeaderSearchOptions.h (+9-1) - (modified) clang/lib/Serialization/ASTWriter.cpp (+26-24) - (modified) clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp (+2) - (added) clang/test/Modules/diagnostic-options-mismatch.c (+41) ``````````diff diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td index 5415b18d3f406df..68ec32bdf56a555 100644 --- a/clang/include/clang/Driver/Options.td +++ b/clang/include/clang/Driver/Options.td @@ -2944,6 +2944,10 @@ def fno_modules_validate_textual_header_includes : MarshallingInfoNegativeFlag<LangOpts<"ModulesValidateTextualHeaderIncludes">>, HelpText<"Do not enforce -fmodules-decluse and private header restrictions for textual headers. " "This flag will be removed in a future Clang release.">; +defm modules_skip_diagnostic_options : BoolFOption<"modules-skip-diagnostic-options", + HeaderSearchOpts<"ModulesSkipDiagnosticOptions">, DefaultFalse, + PosFlag<SetTrue, [], [], "Disable writing diagnostic options">, + NegFlag<SetFalse>, BothFlags<[], [CC1Option]>>; def fincremental_extensions : Flag<["-"], "fincremental-extensions">, diff --git a/clang/include/clang/Lex/HeaderSearchOptions.h b/clang/include/clang/Lex/HeaderSearchOptions.h index c7d95006bb779ad..766b1919a5b5eab 100644 --- a/clang/include/clang/Lex/HeaderSearchOptions.h +++ b/clang/include/clang/Lex/HeaderSearchOptions.h @@ -219,6 +219,12 @@ class HeaderSearchOptions { unsigned ModulesValidateDiagnosticOptions : 1; + /// Whether to entirely skip writing diagnostic options. + unsigned ModulesSkipDiagnosticOptions : 1; + + /// Whether to entirely skip writing header search paths. + unsigned ModulesSkipHeaderSearchPaths : 1; + unsigned ModulesHashContent : 1; /// Whether we should include all things that could impact the module in the @@ -238,7 +244,9 @@ class HeaderSearchOptions { ModulesValidateSystemHeaders(false), ValidateASTInputFilesContent(false), ForceCheckCXX20ModulesInputFiles(false), UseDebugInfo(false), - ModulesValidateDiagnosticOptions(true), ModulesHashContent(false), + ModulesValidateDiagnosticOptions(true), + ModulesSkipDiagnosticOptions(false), + ModulesSkipHeaderSearchPaths(false), ModulesHashContent(false), ModulesStrictContextHash(false) {} /// AddPath - Add the \p Path path to the specified \p Group list. diff --git a/clang/lib/Serialization/ASTWriter.cpp b/clang/lib/Serialization/ASTWriter.cpp index 27700c711d52fdd..d5689cb3c97cb39 100644 --- a/clang/lib/Serialization/ASTWriter.cpp +++ b/clang/lib/Serialization/ASTWriter.cpp @@ -1212,9 +1212,12 @@ void ASTWriter::writeUnhashedControlBlock(Preprocessor &PP, Record.clear(); } + const auto &HSOpts = PP.getHeaderSearchInfo().getHeaderSearchOpts(); + // Diagnostic options. const auto &Diags = Context.getDiagnostics(); const DiagnosticOptions &DiagOpts = Diags.getDiagnosticOptions(); + if (!HSOpts.ModulesSkipDiagnosticOptions) { #define DIAGOPT(Name, Bits, Default) Record.push_back(DiagOpts.Name); #define ENUM_DIAGOPT(Name, Type, Bits, Default) \ Record.push_back(static_cast<unsigned>(DiagOpts.get##Name())); @@ -1229,35 +1232,34 @@ void ASTWriter::writeUnhashedControlBlock(Preprocessor &PP, // are generally transient files and will almost always be overridden. Stream.EmitRecord(DIAGNOSTIC_OPTIONS, Record); Record.clear(); + } // Header search paths. - Record.clear(); - const HeaderSearchOptions &HSOpts = - PP.getHeaderSearchInfo().getHeaderSearchOpts(); - - // Include entries. - Record.push_back(HSOpts.UserEntries.size()); - for (unsigned I = 0, N = HSOpts.UserEntries.size(); I != N; ++I) { - const HeaderSearchOptions::Entry &Entry = HSOpts.UserEntries[I]; - AddString(Entry.Path, Record); - Record.push_back(static_cast<unsigned>(Entry.Group)); - Record.push_back(Entry.IsFramework); - Record.push_back(Entry.IgnoreSysRoot); - } + if (!HSOpts.ModulesSkipHeaderSearchPaths) { + // Include entries. + Record.push_back(HSOpts.UserEntries.size()); + for (unsigned I = 0, N = HSOpts.UserEntries.size(); I != N; ++I) { + const HeaderSearchOptions::Entry &Entry = HSOpts.UserEntries[I]; + AddString(Entry.Path, Record); + Record.push_back(static_cast<unsigned>(Entry.Group)); + Record.push_back(Entry.IsFramework); + Record.push_back(Entry.IgnoreSysRoot); + } - // System header prefixes. - Record.push_back(HSOpts.SystemHeaderPrefixes.size()); - for (unsigned I = 0, N = HSOpts.SystemHeaderPrefixes.size(); I != N; ++I) { - AddString(HSOpts.SystemHeaderPrefixes[I].Prefix, Record); - Record.push_back(HSOpts.SystemHeaderPrefixes[I].IsSystemHeader); - } + // System header prefixes. + Record.push_back(HSOpts.SystemHeaderPrefixes.size()); + for (unsigned I = 0, N = HSOpts.SystemHeaderPrefixes.size(); I != N; ++I) { + AddString(HSOpts.SystemHeaderPrefixes[I].Prefix, Record); + Record.push_back(HSOpts.SystemHeaderPrefixes[I].IsSystemHeader); + } - // VFS overlay files. - Record.push_back(HSOpts.VFSOverlayFiles.size()); - for (StringRef VFSOverlayFile : HSOpts.VFSOverlayFiles) - AddString(VFSOverlayFile, Record); + // VFS overlay files. + Record.push_back(HSOpts.VFSOverlayFiles.size()); + for (StringRef VFSOverlayFile : HSOpts.VFSOverlayFiles) + AddString(VFSOverlayFile, Record); - Stream.EmitRecord(HEADER_SEARCH_PATHS, Record); + Stream.EmitRecord(HEADER_SEARCH_PATHS, Record); + } // Write out the diagnostic/pragma mappings. WritePragmaDiagnosticMappings(Diags, /* isModule = */ WritingModule); diff --git a/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp b/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp index 29df0c3a0afdb5c..dc7bb289cf8f905 100644 --- a/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp +++ b/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp @@ -252,6 +252,8 @@ class DependencyScanningAction : public tooling::ToolAction { // TODO: Implement diagnostic bucketing to reduce the impact of strict // context hashing. ScanInstance.getHeaderSearchOpts().ModulesStrictContextHash = true; + ScanInstance.getHeaderSearchOpts().ModulesSkipDiagnosticOptions = true; + ScanInstance.getHeaderSearchOpts().ModulesSkipHeaderSearchPaths = true; // Avoid some checks and module map parsing when loading PCM files. ScanInstance.getPreprocessorOpts().ModulesCheckRelocated = false; diff --git a/clang/test/Modules/diagnostic-options-mismatch.c b/clang/test/Modules/diagnostic-options-mismatch.c new file mode 100644 index 000000000000000..cdd6f897729a9d8 --- /dev/null +++ b/clang/test/Modules/diagnostic-options-mismatch.c @@ -0,0 +1,41 @@ +// RUN: rm -rf %t && mkdir -p %t +// RUN: split-file %s %t + +//--- module.modulemap +module Mod { header "mod.h" } +//--- mod.h +//--- tu.c +#include "mod.h" + +// Without any extra compiler flags, mismatched diagnostic options trigger recompilation of modules. +// +// RUN: %clang_cc1 -fmodules -fmodule-map-file=%t/module.modulemap -fmodules-cache-path=%t/cache1 -fdisable-module-hash \ +// RUN: -fsyntax-only %t/tu.c -Wnon-modular-include-in-module +// RUN: %clang_cc1 -fmodules -fmodule-map-file=%t/module.modulemap -fmodules-cache-path=%t/cache1 -fdisable-module-hash \ +// RUN: -fsyntax-only %t/tu.c -Werror=non-modular-include-in-module -Rmodule-build 2>&1 \ +// RUN: | FileCheck %s --check-prefix=DID-REBUILD +// DID-REBUILD: remark: building module 'Mod' + +// When skipping serialization of diagnostic options, mismatches cannot be detected, old PCM file gets reused. +// +// RUN: %clang_cc1 -fmodules -fmodule-map-file=%t/module.modulemap -fmodules-cache-path=%t/cache2 -fdisable-module-hash \ +// RUN: -fsyntax-only %t/tu.c -fmodules-skip-diagnostic-options -Wnon-modular-include-in-module +// RUN: %clang_cc1 -fmodules -fmodule-map-file=%t/module.modulemap -fmodules-cache-path=%t/cache2 -fdisable-module-hash \ +// RUN: -fsyntax-only %t/tu.c -fmodules-skip-diagnostic-options -Werror=non-modular-include-in-module -Rmodule-build 2>&1 \ +// RUN: | FileCheck %s --check-prefix=DID-REUSE --allow-empty +// DID-REUSE-NOT: remark: building module 'Mod' +// +// RUN: %clang_cc1 -module-file-info %t/cache2/Mod.pcm | FileCheck %s --check-prefix=NO-DIAG-OPTS +// NO-DIAG-OPTS-NOT: Diagnostic flags: + +// When disabling validation of diagnostic options, mismatches are not checked for, old PCM file gets reused. +// +// RUN: %clang_cc1 -fmodules -fmodule-map-file=%t/module.modulemap -fmodules-cache-path=%t/cache3 -fdisable-module-hash \ +// RUN: -fsyntax-only %t/tu.c -fmodules-disable-diagnostic-validation -Wnon-modular-include-in-module +// RUN: %clang_cc1 -fmodules -fmodule-map-file=%t/module.modulemap -fmodules-cache-path=%t/cache3 -fdisable-module-hash \ +// RUN: -fsyntax-only %t/tu.c -fmodules-disable-diagnostic-validation -Werror=non-modular-include-in-module -Rmodule-build 2>&1 \ +// RUN: | FileCheck %s --check-prefix=DID-REUSE --allow-empty +// +// RUN: %clang_cc1 -module-file-info %t/cache3/Mod.pcm | FileCheck %s --check-prefix=OLD-DIAG-OPTS +// OLD-DIAG-OPTS: Diagnostic flags: +// OLD-DIAG-OPTS-NEXT: -Wnon-modular-include-in-module `````````` </details> https://github.com/llvm/llvm-project/pull/69975 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
