[clang-installapi] Store dylib attributes in the order they are passed

on the command line.

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-determinisic StringMap. Instead, hold them in custom container that
holds an underlying container to continue supporting searching by
attribute.

Author: cyndyishida

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.

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;
 }
 

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

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.");
       }

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,

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)};
 }