[clang] [clang-installapi] Store dylib attributes in the order they are passed on the command line. (PR #139087)
via cfe-commits
cfe-commits at lists.llvm.org
Thu May 8 07:45:59 PDT 2025
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
More information about the cfe-commits
mailing list