[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.

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