[clang-installapi] Store dylib attributes in the order they are passed on the command line. #139087
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
|
@llvm/pr-subscribers-clang Author: Cyndy Ishida (cyndyishida) ChangesWith the introduction of tbd-v5 holding rpaths, the order in which attributes are passed to Resolves errors when building with reverse-iteration. Full diff: https://github.com/llvm/llvm-project/pull/139087.diff 6 Files Affected:
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)};
}
|
zixu-w
approved these changes
May 8, 2025
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/174/builds/17479 Here is the relevant piece of the build log for the reference |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
With the introduction of tbd-v5 holding rpaths, the order in which those attributes are passed to
clang-installapimust be represented in tbd files. Previously, all dylib attributes were stored in a non-deterministicStringMap. 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.