llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-tools-extra @llvm/pr-subscribers-clang Author: Jan Svoboda (jansvoboda11) <details> <summary>Changes</summary> This PR removes the `HeaderFileInfo::Framework` member and reduces the size of this data type from 32B to 16B. This should improve Clang's memory usage in situations where it keeps track of lots of header files. NFCI. Depends on #<!-- -->114459. --- Patch is 26.90 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/114460.diff 14 Files Affected: - (modified) clang-tools-extra/clangd/index/SymbolCollector.cpp (+28-30) - (modified) clang/include/clang/Driver/Options.td (-3) - (modified) clang/include/clang/Lex/DirectoryLookup.h (+3-13) - (modified) clang/include/clang/Lex/HeaderSearch.h (+3-15) - (modified) clang/include/clang/Lex/HeaderSearchOptions.h (-3) - (modified) clang/lib/Driver/ToolChains/Clang.cpp (+1-2) - (modified) clang/lib/Frontend/CompilerInvocation.cpp (+6-24) - (modified) clang/lib/Lex/HeaderSearch.cpp (-60) - (modified) clang/lib/Lex/InitHeaderSearch.cpp (+4-5) - (modified) clang/lib/Serialization/ASTReader.cpp (+7-15) - (modified) clang/lib/Serialization/ASTReaderInternals.h (+2-5) - (modified) clang/lib/Serialization/ASTWriter.cpp (+2-25) - (removed) clang/test/Driver/index-header-map.c (-4) - (modified) clang/unittests/Lex/HeaderSearchTest.cpp (+1-4) ``````````diff diff --git a/clang-tools-extra/clangd/index/SymbolCollector.cpp b/clang-tools-extra/clangd/index/SymbolCollector.cpp index d1d744a21cfd55..91ae9d3003a971 100644 --- a/clang-tools-extra/clangd/index/SymbolCollector.cpp +++ b/clang-tools-extra/clangd/index/SymbolCollector.cpp @@ -335,9 +335,10 @@ class SymbolCollector::HeaderFileURICache { } struct FrameworkHeaderPath { - // Path to the framework directory containing the Headers/PrivateHeaders - // directories e.g. /Frameworks/Foundation.framework/ - llvm::StringRef HeadersParentDir; + // Path to the frameworks directory containing the .framework directory. + llvm::StringRef FrameworkParentDir; + // Name of the framework. + llvm::StringRef FrameworkName; // Subpath relative to the Headers or PrivateHeaders dir, e.g. NSObject.h // Note: This is NOT relative to the `HeadersParentDir`. llvm::StringRef HeaderSubpath; @@ -351,19 +352,17 @@ class SymbolCollector::HeaderFileURICache { path::reverse_iterator I = path::rbegin(Path); path::reverse_iterator Prev = I; path::reverse_iterator E = path::rend(Path); + FrameworkHeaderPath HeaderPath; while (I != E) { - if (*I == "Headers") { - FrameworkHeaderPath HeaderPath; - HeaderPath.HeadersParentDir = Path.substr(0, I - E); + if (*I == "Headers" || *I == "PrivateHeaders") { HeaderPath.HeaderSubpath = Path.substr(Prev - E); - HeaderPath.IsPrivateHeader = false; - return HeaderPath; - } - if (*I == "PrivateHeaders") { - FrameworkHeaderPath HeaderPath; - HeaderPath.HeadersParentDir = Path.substr(0, I - E); - HeaderPath.HeaderSubpath = Path.substr(Prev - E); - HeaderPath.IsPrivateHeader = true; + HeaderPath.IsPrivateHeader = *I == "PrivateHeaders"; + if (++I == E) + break; + HeaderPath.FrameworkName = *I; + if (!HeaderPath.FrameworkName.consume_back(".framework")) + break; + HeaderPath.FrameworkParentDir = Path.substr(0, I - E); return HeaderPath; } Prev = I; @@ -379,26 +378,27 @@ class SymbolCollector::HeaderFileURICache { // <Foundation/NSObject_Private.h> which should be used instead of directly // importing the header. std::optional<std::string> - getFrameworkUmbrellaSpelling(llvm::StringRef Framework, - const HeaderSearch &HS, + getFrameworkUmbrellaSpelling(const HeaderSearch &HS, FrameworkHeaderPath &HeaderPath) { + StringRef Framework = HeaderPath.FrameworkName; auto Res = CacheFrameworkToUmbrellaHeaderSpelling.try_emplace(Framework); auto *CachedSpelling = &Res.first->second; if (!Res.second) { return HeaderPath.IsPrivateHeader ? CachedSpelling->PrivateHeader : CachedSpelling->PublicHeader; } - SmallString<256> UmbrellaPath(HeaderPath.HeadersParentDir); - llvm::sys::path::append(UmbrellaPath, "Headers", Framework + ".h"); + SmallString<256> UmbrellaPath(HeaderPath.FrameworkParentDir); + llvm::sys::path::append(UmbrellaPath, Framework + ".framework", "Headers", + Framework + ".h"); llvm::vfs::Status Status; auto StatErr = HS.getFileMgr().getNoncachedStatValue(UmbrellaPath, Status); if (!StatErr) CachedSpelling->PublicHeader = llvm::formatv("<{0}/{0}.h>", Framework); - UmbrellaPath = HeaderPath.HeadersParentDir; - llvm::sys::path::append(UmbrellaPath, "PrivateHeaders", - Framework + "_Private.h"); + UmbrellaPath = HeaderPath.FrameworkParentDir; + llvm::sys::path::append(UmbrellaPath, Framework + ".framework", + "PrivateHeaders", Framework + "_Private.h"); StatErr = HS.getFileMgr().getNoncachedStatValue(UmbrellaPath, Status); if (!StatErr) @@ -414,8 +414,7 @@ class SymbolCollector::HeaderFileURICache { // give <Foundation/Foundation.h> if the umbrella header exists, otherwise // <Foundation/NSObject.h>. std::optional<llvm::StringRef> - getFrameworkHeaderIncludeSpelling(FileEntryRef FE, llvm::StringRef Framework, - HeaderSearch &HS) { + getFrameworkHeaderIncludeSpelling(FileEntryRef FE, HeaderSearch &HS) { auto Res = CachePathToFrameworkSpelling.try_emplace(FE.getName()); auto *CachedHeaderSpelling = &Res.first->second; if (!Res.second) @@ -429,13 +428,15 @@ class SymbolCollector::HeaderFileURICache { return std::nullopt; } if (auto UmbrellaSpelling = - getFrameworkUmbrellaSpelling(Framework, HS, *HeaderPath)) { + getFrameworkUmbrellaSpelling(HS, *HeaderPath)) { *CachedHeaderSpelling = *UmbrellaSpelling; return llvm::StringRef(*CachedHeaderSpelling); } *CachedHeaderSpelling = - llvm::formatv("<{0}/{1}>", Framework, HeaderPath->HeaderSubpath).str(); + llvm::formatv("<{0}/{1}>", HeaderPath->FrameworkName, + HeaderPath->HeaderSubpath) + .str(); return llvm::StringRef(*CachedHeaderSpelling); } @@ -454,11 +455,8 @@ class SymbolCollector::HeaderFileURICache { // Framework headers are spelled as <FrameworkName/Foo.h>, not // "path/FrameworkName.framework/Headers/Foo.h". auto &HS = PP->getHeaderSearchInfo(); - if (const auto *HFI = HS.getExistingFileInfo(*FE)) - if (!HFI->Framework.empty()) - if (auto Spelling = - getFrameworkHeaderIncludeSpelling(*FE, HFI->Framework, HS)) - return *Spelling; + if (auto Spelling = getFrameworkHeaderIncludeSpelling(*FE, HS)) + return *Spelling; if (!tooling::isSelfContainedHeader(*FE, PP->getSourceManager(), PP->getHeaderSearchInfo())) { diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td index c8bc2fe377b8ec..7a05cc03353ac1 100644 --- a/clang/include/clang/Driver/Options.td +++ b/clang/include/clang/Driver/Options.td @@ -4564,9 +4564,6 @@ def ibuiltininc : Flag<["-"], "ibuiltininc">, Group<clang_i_Group>, HelpText<"Enable builtin #include directories even when -nostdinc is used " "before or after -ibuiltininc. " "Using -nobuiltininc after the option disables it">; -def index_header_map : Flag<["-"], "index-header-map">, - Visibility<[ClangOption, CC1Option]>, - HelpText<"Make the next included directory (-I or -F) an indexer header map">; def iapinotes_modules : JoinedOrSeparate<["-"], "iapinotes-modules">, Group<clang_i_Group>, Visibility<[ClangOption, CC1Option]>, HelpText<"Add directory to the API notes search path referenced by module name">, MetaVarName<"<directory>">; diff --git a/clang/include/clang/Lex/DirectoryLookup.h b/clang/include/clang/Lex/DirectoryLookup.h index 81680d3b271e08..bb703dfad2b28f 100644 --- a/clang/include/clang/Lex/DirectoryLookup.h +++ b/clang/include/clang/Lex/DirectoryLookup.h @@ -58,10 +58,6 @@ class DirectoryLookup { LLVM_PREFERRED_TYPE(LookupType_t) unsigned LookupType : 2; - /// Whether this is a header map used when building a framework. - LLVM_PREFERRED_TYPE(bool) - unsigned IsIndexHeaderMap : 1; - /// Whether we've performed an exhaustive search for module maps /// within the subdirectories of this directory. LLVM_PREFERRED_TYPE(bool) @@ -73,13 +69,12 @@ class DirectoryLookup { bool isFramework) : u(Dir), DirCharacteristic(DT), LookupType(isFramework ? LT_Framework : LT_NormalDir), - IsIndexHeaderMap(false), SearchedAllModuleMaps(false) {} + SearchedAllModuleMaps(false) {} /// This ctor *does not take ownership* of 'Map'. - DirectoryLookup(const HeaderMap *Map, SrcMgr::CharacteristicKind DT, - bool isIndexHeaderMap) + DirectoryLookup(const HeaderMap *Map, SrcMgr::CharacteristicKind DT) : u(Map), DirCharacteristic(DT), LookupType(LT_HeaderMap), - IsIndexHeaderMap(isIndexHeaderMap), SearchedAllModuleMaps(false) {} + SearchedAllModuleMaps(false) {} /// getLookupType - Return the kind of directory lookup that this is: either a /// normal directory, a framework path, or a HeaderMap. @@ -146,11 +141,6 @@ class DirectoryLookup { return getDirCharacteristic() != SrcMgr::C_User; } - /// Whether this header map is building a framework or not. - bool isIndexHeaderMap() const { - return isHeaderMap() && IsIndexHeaderMap; - } - /// LookupFile - Lookup the specified file in this search path, returning it /// if it exists or returning null if not. /// diff --git a/clang/include/clang/Lex/HeaderSearch.h b/clang/include/clang/Lex/HeaderSearch.h index df75c192c700a0..a10adae17998b5 100644 --- a/clang/include/clang/Lex/HeaderSearch.h +++ b/clang/include/clang/Lex/HeaderSearch.h @@ -108,16 +108,6 @@ struct HeaderFileInfo { LLVM_PREFERRED_TYPE(bool) unsigned Resolved : 1; - /// Whether this is a header inside a framework that is currently - /// being built. - /// - /// When a framework is being built, the headers have not yet been placed - /// into the appropriate framework subdirectories, and therefore are - /// provided via a header map. This bit indicates when this is one of - /// those framework headers. - LLVM_PREFERRED_TYPE(bool) - unsigned IndexHeaderMapHeader : 1; - /// Whether this file has been looked up as a header. LLVM_PREFERRED_TYPE(bool) unsigned IsValid : 1; @@ -132,15 +122,11 @@ struct HeaderFileInfo { /// external storage. LazyIdentifierInfoPtr LazyControllingMacro; - /// If this header came from a framework include, this is the name - /// of the framework. - StringRef Framework; - HeaderFileInfo() : IsLocallyIncluded(false), isImport(false), isPragmaOnce(false), DirInfo(SrcMgr::C_User), External(false), isModuleHeader(false), isTextualModuleHeader(false), isCompilingModuleHeader(false), - Resolved(false), IndexHeaderMapHeader(false), IsValid(false) {} + Resolved(false), IsValid(false) {} /// Retrieve the controlling macro for this header file, if /// any. @@ -154,6 +140,8 @@ struct HeaderFileInfo { void mergeModuleMembership(ModuleMap::ModuleHeaderRole Role); }; +static_assert(sizeof(HeaderFileInfo) <= 16); + /// An external source of header file information, which may supply /// information about header files already included. class ExternalHeaderFileInfoSource { diff --git a/clang/include/clang/Lex/HeaderSearchOptions.h b/clang/include/clang/Lex/HeaderSearchOptions.h index 83a95e9ad90a7f..c85e3d27281701 100644 --- a/clang/include/clang/Lex/HeaderSearchOptions.h +++ b/clang/include/clang/Lex/HeaderSearchOptions.h @@ -35,9 +35,6 @@ enum IncludeDirGroup { /// Paths for '\#include <>' added by '-I'. Angled, - /// Like Angled, but marks header maps used when building frameworks. - IndexHeaderMap, - /// Like Angled, but marks system directories. System, diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp index 04b3832327a99c..db2a4c4d6ff974 100644 --- a/clang/lib/Driver/ToolChains/Clang.cpp +++ b/clang/lib/Driver/ToolChains/Clang.cpp @@ -1185,8 +1185,7 @@ void Clang::AddPreprocessingOptions(Compilation &C, const JobAction &JA, Args.addAllArgs(CmdArgs, {options::OPT_D, options::OPT_U, options::OPT_I_Group, - options::OPT_F, options::OPT_index_header_map, - options::OPT_embed_dir_EQ}); + options::OPT_F, options::OPT_embed_dir_EQ}); // Add -Wp, and -Xpreprocessor if using the preprocessor. diff --git a/clang/lib/Frontend/CompilerInvocation.cpp b/clang/lib/Frontend/CompilerInvocation.cpp index d8261e12b08b5c..b5fd35aaa1e841 100644 --- a/clang/lib/Frontend/CompilerInvocation.cpp +++ b/clang/lib/Frontend/CompilerInvocation.cpp @@ -3190,15 +3190,10 @@ static void GenerateHeaderSearchArgs(const HeaderSearchOptions &Opts, auto It = Opts.UserEntries.begin(); auto End = Opts.UserEntries.end(); - // Add -I..., -F..., and -index-header-map options in order. - for (; It < End && Matches(*It, {frontend::IndexHeaderMap, frontend::Angled}, - std::nullopt, true); + // Add -I... and -F... options in order. + for (; It < End && Matches(*It, {frontend::Angled}, std::nullopt, true); ++It) { OptSpecifier Opt = [It, Matches]() { - if (Matches(*It, frontend::IndexHeaderMap, true, true)) - return OPT_F; - if (Matches(*It, frontend::IndexHeaderMap, false, true)) - return OPT_I; if (Matches(*It, frontend::Angled, true, true)) return OPT_F; if (Matches(*It, frontend::Angled, false, true)) @@ -3206,8 +3201,6 @@ static void GenerateHeaderSearchArgs(const HeaderSearchOptions &Opts, llvm_unreachable("Unexpected HeaderSearchOptions::Entry."); }(); - if (It->Group == frontend::IndexHeaderMap) - GenerateArg(Consumer, OPT_index_header_map); GenerateArg(Consumer, Opt, It->Path); }; @@ -3319,8 +3312,7 @@ static bool ParseHeaderSearchArgs(HeaderSearchOptions &Opts, ArgList &Args, llvm::CachedHashString(MacroDef.split('=').first)); } - // Add -I..., -F..., and -index-header-map options in order. - bool IsIndexHeaderMap = false; + // Add -I... and -F... options in order. bool IsSysrootSpecified = Args.hasArg(OPT__sysroot_EQ) || Args.hasArg(OPT_isysroot); @@ -3339,20 +3331,10 @@ static bool ParseHeaderSearchArgs(HeaderSearchOptions &Opts, ArgList &Args, return A->getValue(); }; - for (const auto *A : Args.filtered(OPT_I, OPT_F, OPT_index_header_map)) { - if (A->getOption().matches(OPT_index_header_map)) { - // -index-header-map applies to the next -I or -F. - IsIndexHeaderMap = true; - continue; - } - - frontend::IncludeDirGroup Group = - IsIndexHeaderMap ? frontend::IndexHeaderMap : frontend::Angled; - + for (const auto *A : Args.filtered(OPT_I, OPT_F)) { bool IsFramework = A->getOption().matches(OPT_F); - Opts.AddPath(PrefixHeaderPath(A, IsFramework), Group, IsFramework, - /*IgnoreSysroot*/ true); - IsIndexHeaderMap = false; + Opts.AddPath(PrefixHeaderPath(A, IsFramework), frontend::Angled, + IsFramework, /*IgnoreSysroot=*/true); } // Add -iprefix/-iwithprefix/-iwithprefixbefore options. diff --git a/clang/lib/Lex/HeaderSearch.cpp b/clang/lib/Lex/HeaderSearch.cpp index 052be1395161d4..c5614a8e0ee526 100644 --- a/clang/lib/Lex/HeaderSearch.cpp +++ b/clang/lib/Lex/HeaderSearch.cpp @@ -974,13 +974,9 @@ OptionalFileEntryRef HeaderSearch::LookupFile( const HeaderFileInfo *FromHFI = getExistingFileInfo(*Includer); assert(FromHFI && "includer without file info"); unsigned DirInfo = FromHFI->DirInfo; - bool IndexHeaderMapHeader = FromHFI->IndexHeaderMapHeader; - StringRef Framework = FromHFI->Framework; HeaderFileInfo &ToHFI = getFileInfo(*FE); ToHFI.DirInfo = DirInfo; - ToHFI.IndexHeaderMapHeader = IndexHeaderMapHeader; - ToHFI.Framework = Framework; if (SearchPath) { StringRef SearchPathRef(IncluderAndDir.second.getName()); @@ -1122,23 +1118,6 @@ OptionalFileEntryRef HeaderSearch::LookupFile( } } - // Set the `Framework` info if this file is in a header map with framework - // style include spelling or found in a framework dir. The header map case - // is possible when building frameworks which use header maps. - if (CurDir->isHeaderMap() && isAngled) { - size_t SlashPos = Filename.find('/'); - if (SlashPos != StringRef::npos) - HFI.Framework = - getUniqueFrameworkName(StringRef(Filename.begin(), SlashPos)); - if (CurDir->isIndexHeaderMap()) - HFI.IndexHeaderMapHeader = 1; - } else if (CurDir->isFramework()) { - size_t SlashPos = Filename.find('/'); - if (SlashPos != StringRef::npos) - HFI.Framework = - getUniqueFrameworkName(StringRef(Filename.begin(), SlashPos)); - } - if (checkMSVCHeaderSearch(Diags, MSFE, &File->getFileEntry(), IncludeLoc)) { if (SuggestedModule) *SuggestedModule = MSSuggestedModule; @@ -1156,41 +1135,6 @@ OptionalFileEntryRef HeaderSearch::LookupFile( return File; } - // If we are including a file with a quoted include "foo.h" from inside - // a header in a framework that is currently being built, and we couldn't - // resolve "foo.h" any other way, change the include to <Foo/foo.h>, where - // "Foo" is the name of the framework in which the including header was found. - if (!Includers.empty() && Includers.front().first && !isAngled && - !Filename.contains('/')) { - const HeaderFileInfo *IncludingHFI = - getExistingFileInfo(*Includers.front().first); - assert(IncludingHFI && "includer without file info"); - if (IncludingHFI->IndexHeaderMapHeader) { - SmallString<128> ScratchFilename; - ScratchFilename += IncludingHFI->Framework; - ScratchFilename += '/'; - ScratchFilename += Filename; - - OptionalFileEntryRef File = LookupFile( - ScratchFilename, IncludeLoc, /*isAngled=*/true, FromDir, &CurDir, - Includers.front(), SearchPath, RelativePath, RequestingModule, - SuggestedModule, IsMapped, /*IsFrameworkFound=*/nullptr); - - if (checkMSVCHeaderSearch(Diags, MSFE, - File ? &File->getFileEntry() : nullptr, - IncludeLoc)) { - if (SuggestedModule) - *SuggestedModule = MSSuggestedModule; - return MSFE; - } - - cacheLookupSuccess(LookupFileCache[Filename], - LookupFileCache[ScratchFilename].HitIt, IncludeLoc); - // FIXME: SuggestedModule. - return File; - } - } - if (checkMSVCHeaderSearch(Diags, MSFE, nullptr, IncludeLoc)) { if (SuggestedModule) *SuggestedModule = MSSuggestedModule; @@ -1358,10 +1302,6 @@ static void mergeHeaderFileInfo(HeaderFileInfo &HFI, HFI.DirInfo = OtherHFI.DirInfo; HFI.External = (!HFI.IsValid || HFI.External); HFI.IsValid = true; - HFI.IndexHeaderMapHeader = OtherHFI.IndexHeaderMapHeader; - - if (HFI.Framework.empty()) - HFI.Framework = OtherHFI.Framework; } HeaderFileInfo &HeaderSearch::getFileInfo(FileEntryRef FE) { diff --git a/clang/lib/Lex/InitHeaderSearch.cpp b/clang/lib/Lex/InitHeaderSearch.cpp index 2218db15013d92..86c2ecdf9e36eb 100644 --- a/clang/lib/Lex/InitHeaderSearch.cpp +++ b/clang/lib/Lex/InitHeaderSearch.cpp @@ -149,7 +149,7 @@ bool InitHeaderSearch::AddUnmappedPath(const Twine &Path, IncludeDirGroup Group, // Compute the DirectoryLookup type. SrcMgr::CharacteristicKind Type; - if (Group == Quoted || Group == Angled || Group == IndexHeaderMap) { + if (Group == Quoted || Group == Angled) { Type = SrcMgr::C_User; } else if (Group == ExternCSystem) { Type = SrcMgr::C_ExternCSystem; @@ -170,9 +170,8 @@ bool InitHeaderSearch::AddUnmappedPath(const Twine &Path, IncludeDirGroup Group, if (auto FE = FM.getOptionalFileRef(MappedPathStr)) { if (const HeaderMap *HM = Headers.CreateHeaderMap(*FE)) { // It is a headermap, add it to the search path. - IncludePath.emplace_back( - Group, DirectoryLookup(HM, Type, Group == IndexHeaderMap), - UserEntryIdx); + IncludePath.emplace_back(Group, DirectoryLookup(HM, Type), + UserEntryIdx); return true; } } @@ -488,7 +487,7 @@ void InitHeaderSearch::Realize(const LangOptions &Lang) { unsigned NumQuoted = SearchList.size(); for (auto &Include : IncludePath) - if (Include.Group == Angled || Include.Group == IndexHeaderMap) + if (Include.Group == Angled) SearchList.push_back(Include); RemoveDuplicates(SearchList, NumQuoted, Verbose); diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp index 8d8f9378cfeabe..6ccd57c1cca8f5 100644 --- a/clang/lib/Serialization/ASTReader.cpp +++ b/clang/lib/Serialization/ASTReader.cpp @@ -2123,16 +2123,11 @@ HeaderFileInfoTrait::ReadData(internal_key_ref key, const unsigned char *d, HFI.isImport |= (Flags >> 5) & 0x01; HFI.isPragmaOnce |= (Flags >> 4) & 0x01; HFI.DirInfo = (Flags >> 1) & 0x07; - HFI.IndexHeaderMap... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/114460 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
