llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang Author: Cyndy Ishida (cyndyishida) <details> <summary>Changes</summary> With the introduction of tbd-v5 holding rpaths, the order in which attributes are passed to `clang-installapi` must be represented in tbd files. Previously, all of these attributes were stored in a non-deterministic `StringMap`. Instead, hold them in a custom collection with an underlying vector to continue supporting searching by attribute. Resolves errors when building with reverse-iteration. --- Full diff: https://github.com/llvm/llvm-project/pull/139087.diff 6 Files Affected: - (modified) clang/include/clang/InstallAPI/DylibVerifier.h (+23-1) - (modified) clang/lib/InstallAPI/DiagnosticBuilderWrappers.cpp (+5-5) - (modified) clang/lib/InstallAPI/DiagnosticBuilderWrappers.h (+2-1) - (modified) clang/lib/InstallAPI/DylibVerifier.cpp (+35-14) - (modified) clang/tools/clang-installapi/ClangInstallAPI.cpp (+3-3) - (modified) clang/tools/clang-installapi/Options.cpp (+19-22) ``````````diff diff --git a/clang/include/clang/InstallAPI/DylibVerifier.h b/clang/include/clang/InstallAPI/DylibVerifier.h index 333f0cff077fd..4cf70a8adc9bc 100644 --- a/clang/include/clang/InstallAPI/DylibVerifier.h +++ b/clang/include/clang/InstallAPI/DylibVerifier.h @@ -25,9 +25,31 @@ enum class VerificationMode { Pedantic, }; -using LibAttrs = llvm::StringMap<ArchitectureSet>; using ReexportedInterfaces = llvm::SmallVector<llvm::MachO::InterfaceFile, 8>; +/// Represents dynamic library specific attributes that are tied to +/// architecture slices. It is commonly used for comparing options +/// passed on the command line to installapi and what exists in dylib load +/// commands. +class LibAttrs { +public: + using Entry = std::pair<std::string, ArchitectureSet>; + using AttrsToArchs = llvm::SmallVector<Entry, 10>; + + // Mutable access to architecture set tied to the input attribute. + ArchitectureSet &getArchSet(StringRef Attr); + // Get entry based on the attribute. + std::optional<Entry> find(StringRef Attr) const; + // Immutable access to underlying container. + const AttrsToArchs &get() const { return LibraryAttributes; }; + // Mutable access to underlying container. + AttrsToArchs &get() { return LibraryAttributes; }; + bool operator==(const LibAttrs &Other) const { return Other.get() == get(); }; + +private: + AttrsToArchs LibraryAttributes; +}; + // Pointers to information about a zippered declaration used for // querying and reporting violations against different // declarations that all map to the same symbol. diff --git a/clang/lib/InstallAPI/DiagnosticBuilderWrappers.cpp b/clang/lib/InstallAPI/DiagnosticBuilderWrappers.cpp index c8d07f229902b..fd9db8113a41e 100644 --- a/clang/lib/InstallAPI/DiagnosticBuilderWrappers.cpp +++ b/clang/lib/InstallAPI/DiagnosticBuilderWrappers.cpp @@ -97,12 +97,12 @@ const DiagnosticBuilder &operator<<(const DiagnosticBuilder &DB, const clang::DiagnosticBuilder & operator<<(const clang::DiagnosticBuilder &DB, - const StringMapEntry<ArchitectureSet> &LibAttr) { - std::string IFAsString; - raw_string_ostream OS(IFAsString); + const clang::installapi::LibAttrs::Entry &LibAttr) { + std::string Entry; + raw_string_ostream OS(Entry); - OS << LibAttr.getKey() << " [ " << LibAttr.getValue() << " ]"; - DB.AddString(IFAsString); + OS << LibAttr.first << " [ " << LibAttr.second << " ]"; + DB.AddString(Entry); return DB; } diff --git a/clang/lib/InstallAPI/DiagnosticBuilderWrappers.h b/clang/lib/InstallAPI/DiagnosticBuilderWrappers.h index 48cfefbf65e6b..ba24ee415dfcf 100644 --- a/clang/lib/InstallAPI/DiagnosticBuilderWrappers.h +++ b/clang/lib/InstallAPI/DiagnosticBuilderWrappers.h @@ -14,6 +14,7 @@ #define LLVM_CLANG_INSTALLAPI_DIAGNOSTICBUILDER_WRAPPER_H #include "clang/Basic/Diagnostic.h" +#include "clang/InstallAPI/DylibVerifier.h" #include "llvm/TextAPI/Architecture.h" #include "llvm/TextAPI/ArchitectureSet.h" #include "llvm/TextAPI/InterfaceFile.h" @@ -42,7 +43,7 @@ const clang::DiagnosticBuilder &operator<<(const clang::DiagnosticBuilder &DB, const clang::DiagnosticBuilder & operator<<(const clang::DiagnosticBuilder &DB, - const StringMapEntry<ArchitectureSet> &LibAttr); + const clang::installapi::LibAttrs::Entry &LibAttr); } // namespace MachO } // namespace llvm diff --git a/clang/lib/InstallAPI/DylibVerifier.cpp b/clang/lib/InstallAPI/DylibVerifier.cpp index d5d760767b41f..45c84c00d9236 100644 --- a/clang/lib/InstallAPI/DylibVerifier.cpp +++ b/clang/lib/InstallAPI/DylibVerifier.cpp @@ -18,6 +18,25 @@ using namespace llvm::MachO; namespace clang { namespace installapi { +ArchitectureSet &LibAttrs::getArchSet(StringRef Attr) { + auto *It = llvm::find_if(LibraryAttributes, [&Attr](const auto &Input) { + return Attr == Input.first; + }); + if (It != LibraryAttributes.end()) + return It->second; + LibraryAttributes.push_back({Attr.str(), ArchitectureSet()}); + return LibraryAttributes.back().second; +} + +std::optional<LibAttrs::Entry> LibAttrs::find(StringRef Attr) const { + auto *It = llvm::find_if(LibraryAttributes, [&Attr](const auto &Input) { + return Attr == Input.first; + }); + if (It == LibraryAttributes.end()) + return std::nullopt; + return *It; +} + /// Metadata stored about a mapping of a declaration to a symbol. struct DylibVerifier::SymbolContext { // Name to use for all querying and verification @@ -825,13 +844,13 @@ bool DylibVerifier::verifyBinaryAttrs(const ArrayRef<Target> ProvidedTargets, DylibTargets.push_back(RS->getTarget()); const BinaryAttrs &BinInfo = RS->getBinaryAttrs(); for (const StringRef LibName : BinInfo.RexportedLibraries) - DylibReexports[LibName].set(DylibTargets.back().Arch); + DylibReexports.getArchSet(LibName).set(DylibTargets.back().Arch); for (const StringRef LibName : BinInfo.AllowableClients) - DylibClients[LibName].set(DylibTargets.back().Arch); + DylibClients.getArchSet(LibName).set(DylibTargets.back().Arch); // Compare attributes that are only representable in >= TBD_V5. if (FT >= FileType::TBD_V5) for (const StringRef Name : BinInfo.RPaths) - DylibRPaths[Name].set(DylibTargets.back().Arch); + DylibRPaths.getArchSet(Name).set(DylibTargets.back().Arch); } // Check targets first. @@ -923,31 +942,33 @@ bool DylibVerifier::verifyBinaryAttrs(const ArrayRef<Target> ProvidedTargets, if (Provided == Dylib) return true; - for (const llvm::StringMapEntry<ArchitectureSet> &PAttr : Provided) { - const auto DAttrIt = Dylib.find(PAttr.getKey()); - if (DAttrIt == Dylib.end()) { - Ctx.Diag->Report(DiagID_missing) << "binary file" << PAttr; + for (const LibAttrs::Entry &PEntry : Provided.get()) { + const auto &[PAttr, PArchSet] = PEntry; + auto DAttrEntry = Dylib.find(PAttr); + if (!DAttrEntry) { + Ctx.Diag->Report(DiagID_missing) << "binary file" << PEntry; if (Fatal) return false; } - if (PAttr.getValue() != DAttrIt->getValue()) { - Ctx.Diag->Report(DiagID_mismatch) << PAttr << *DAttrIt; + if (PArchSet != DAttrEntry->second) { + Ctx.Diag->Report(DiagID_mismatch) << PEntry << *DAttrEntry; if (Fatal) return false; } } - for (const llvm::StringMapEntry<ArchitectureSet> &DAttr : Dylib) { - const auto PAttrIt = Provided.find(DAttr.getKey()); - if (PAttrIt == Provided.end()) { - Ctx.Diag->Report(DiagID_missing) << "installAPI option" << DAttr; + for (const LibAttrs::Entry &DEntry : Dylib.get()) { + const auto &[DAttr, DArchSet] = DEntry; + const auto &PAttrEntry = Provided.find(DAttr); + if (!PAttrEntry) { + Ctx.Diag->Report(DiagID_missing) << "installAPI option" << DEntry; if (!Fatal) continue; return false; } - if (PAttrIt->getValue() != DAttr.getValue()) { + if (PAttrEntry->second != DArchSet) { if (Fatal) llvm_unreachable("this case was already covered above."); } diff --git a/clang/tools/clang-installapi/ClangInstallAPI.cpp b/clang/tools/clang-installapi/ClangInstallAPI.cpp index 37b69ccf4e00e..14e7b53d74b09 100644 --- a/clang/tools/clang-installapi/ClangInstallAPI.cpp +++ b/clang/tools/clang-installapi/ClangInstallAPI.cpp @@ -170,9 +170,9 @@ static bool run(ArrayRef<const char *> Args, const char *ProgName) { [&IF]( const auto &Attrs, std::function<void(InterfaceFile *, StringRef, const Target &)> Add) { - for (const auto &Lib : Attrs) - for (const auto &T : IF.targets(Lib.getValue())) - Add(&IF, Lib.getKey(), T); + for (const auto &[Attr, ArchSet] : Attrs.get()) + for (const auto &T : IF.targets(ArchSet)) + Add(&IF, Attr, T); }; assignLibAttrs(Opts.LinkerOpts.AllowableClients, diff --git a/clang/tools/clang-installapi/Options.cpp b/clang/tools/clang-installapi/Options.cpp index 9bc168c8cd4f8..73f5470b82486 100644 --- a/clang/tools/clang-installapi/Options.cpp +++ b/clang/tools/clang-installapi/Options.cpp @@ -610,35 +610,35 @@ Options::processAndFilterOutInstallAPIOptions(ArrayRef<const char *> Args) { for (const Arg *A : ParsedArgs.filtered(OPT_allowable_client)) { auto It = ArgToArchMap.find(A); - LinkerOpts.AllowableClients[A->getValue()] = + LinkerOpts.AllowableClients.getArchSet(A->getValue()) = It != ArgToArchMap.end() ? It->second : ArchitectureSet(); A->claim(); } for (const Arg *A : ParsedArgs.filtered(OPT_reexport_l)) { auto It = ArgToArchMap.find(A); - LinkerOpts.ReexportedLibraries[A->getValue()] = + LinkerOpts.ReexportedLibraries.getArchSet(A->getValue()) = It != ArgToArchMap.end() ? It->second : ArchitectureSet(); A->claim(); } for (const Arg *A : ParsedArgs.filtered(OPT_reexport_library)) { auto It = ArgToArchMap.find(A); - LinkerOpts.ReexportedLibraryPaths[A->getValue()] = + LinkerOpts.ReexportedLibraryPaths.getArchSet(A->getValue()) = It != ArgToArchMap.end() ? It->second : ArchitectureSet(); A->claim(); } for (const Arg *A : ParsedArgs.filtered(OPT_reexport_framework)) { auto It = ArgToArchMap.find(A); - LinkerOpts.ReexportedFrameworks[A->getValue()] = + LinkerOpts.ReexportedFrameworks.getArchSet(A->getValue()) = It != ArgToArchMap.end() ? It->second : ArchitectureSet(); A->claim(); } for (const Arg *A : ParsedArgs.filtered(OPT_rpath)) { auto It = ArgToArchMap.find(A); - LinkerOpts.RPaths[A->getValue()] = + LinkerOpts.RPaths.getArchSet(A->getValue()) = It != ArgToArchMap.end() ? It->second : ArchitectureSet(); A->claim(); } @@ -733,9 +733,9 @@ Options::Options(DiagnosticsEngine &Diag, FileManager *FM, llvm::for_each(DriverOpts.Targets, [&AllArchs](const auto &T) { AllArchs.set(T.first.Arch); }); auto assignDefaultLibAttrs = [&AllArchs](LibAttrs &Attrs) { - for (StringMapEntry<ArchitectureSet> &Entry : Attrs) - if (Entry.getValue().empty()) - Entry.setValue(AllArchs); + for (auto &[_, Archs] : Attrs.get()) + if (Archs.empty()) + Archs = AllArchs; }; assignDefaultLibAttrs(LinkerOpts.AllowableClients); assignDefaultLibAttrs(LinkerOpts.ReexportedFrameworks); @@ -789,7 +789,7 @@ std::pair<LibAttrs, ReexportedInterfaces> Options::getReexportedLibraries() { std::unique_ptr<InterfaceFile> Reexport = std::move(*ReexportIFOrErr); StringRef InstallName = Reexport->getInstallName(); assert(!InstallName.empty() && "Parse error for install name"); - Reexports.insert({InstallName, Archs}); + Reexports.getArchSet(InstallName) = Archs; ReexportIFs.emplace_back(std::move(*Reexport)); return true; }; @@ -802,39 +802,36 @@ std::pair<LibAttrs, ReexportedInterfaces> Options::getReexportedLibraries() { for (const PlatformType P : Platforms) { PathSeq PlatformSearchPaths = getPathsForPlatform(FEOpts.SystemFwkPaths, P); llvm::append_range(FwkSearchPaths, PlatformSearchPaths); - for (const StringMapEntry<ArchitectureSet> &Lib : - LinkerOpts.ReexportedFrameworks) { - std::string Name = (Lib.getKey() + ".framework/" + Lib.getKey()).str(); + for (const auto &[Lib, Archs] : LinkerOpts.ReexportedFrameworks.get()) { + std::string Name = (Lib + ".framework/" + Lib); std::string Path = findLibrary(Name, *FM, FwkSearchPaths, {}, {}); if (Path.empty()) { - Diags->Report(diag::err_cannot_find_reexport) << false << Lib.getKey(); + Diags->Report(diag::err_cannot_find_reexport) << false << Lib; return {}; } if (DriverOpts.TraceLibraryLocation) errs() << Path << "\n"; - AccumulateReexports(Path, Lib.getValue()); + AccumulateReexports(Path, Archs); } FwkSearchPaths.resize(FwkSearchPaths.size() - PlatformSearchPaths.size()); } - for (const StringMapEntry<ArchitectureSet> &Lib : - LinkerOpts.ReexportedLibraries) { - std::string Name = "lib" + Lib.getKey().str() + ".dylib"; + for (const auto &[Lib, Archs] : LinkerOpts.ReexportedLibraries.get()) { + std::string Name = "lib" + Lib + ".dylib"; std::string Path = findLibrary(Name, *FM, {}, LinkerOpts.LibPaths, {}); if (Path.empty()) { - Diags->Report(diag::err_cannot_find_reexport) << true << Lib.getKey(); + Diags->Report(diag::err_cannot_find_reexport) << true << Lib; return {}; } if (DriverOpts.TraceLibraryLocation) errs() << Path << "\n"; - AccumulateReexports(Path, Lib.getValue()); + AccumulateReexports(Path, Archs); } - for (const StringMapEntry<ArchitectureSet> &Lib : - LinkerOpts.ReexportedLibraryPaths) - AccumulateReexports(Lib.getKey(), Lib.getValue()); + for (const auto &[Lib, Archs] : LinkerOpts.ReexportedLibraryPaths.get()) + AccumulateReexports(Lib, Archs); return {std::move(Reexports), std::move(ReexportIFs)}; } `````````` </details> https://github.com/llvm/llvm-project/pull/139087 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
