Author: Cyndy Ishida Date: 2025-05-08T21:15:10-07:00 New Revision: 823b1a582258f1417c648b3117ba08edc4855c68
URL: https://github.com/llvm/llvm-project/commit/823b1a582258f1417c648b3117ba08edc4855c68 DIFF: https://github.com/llvm/llvm-project/commit/823b1a582258f1417c648b3117ba08edc4855c68.diff LOG: [clang-installapi] Store dylib attributes in the order they are passed on the command line. (#139087) With the introduction of tbd-v5 holding rpaths, the order in which those attributes are passed to `clang-installapi` must be represented in tbd files. Previously, all dylib 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. This makes the order of all diagnostics related to load command comparisons stable. This approach resolves errors when building with reverse-iteration. Added: Modified: clang/include/clang/InstallAPI/DylibVerifier.h clang/lib/InstallAPI/DiagnosticBuilderWrappers.cpp clang/lib/InstallAPI/DiagnosticBuilderWrappers.h clang/lib/InstallAPI/DylibVerifier.cpp clang/tools/clang-installapi/ClangInstallAPI.cpp clang/tools/clang-installapi/Options.cpp Removed: ################################################################################ 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 diff erent // 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)}; } _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
