<div dir="ltr"><div dir="ltr">Hi Sameer,<div><br></div><div>This broke the build :) There are a lot of complaining build bots you can take a look at.</div><div><br></div><div>I've temporarily reverted it here:</div><div><br></div><div>echristo@athyra ~/s/llvm-project> git push<br>To github.com:llvm/llvm-project.git<br> 1e495e10e6c..4029f8ede42 master -> master<br></div><div><br></div><div>Thanks! and sorry for the inconvenience</div><div><br></div><div>-eric</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Jul 6, 2020 at 3:15 PM Sameer Arora via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
Author: Sameer Arora<br>
Date: 2020-07-06T15:15:20-07:00<br>
New Revision: c143900a0851b2c7b7d52e4825c7f073b3474cf6<br>
<br>
URL: <a href="https://github.com/llvm/llvm-project/commit/c143900a0851b2c7b7d52e4825c7f073b3474cf6" rel="noreferrer" target="_blank">https://github.com/llvm/llvm-project/commit/c143900a0851b2c7b7d52e4825c7f073b3474cf6</a><br>
DIFF: <a href="https://github.com/llvm/llvm-project/commit/c143900a0851b2c7b7d52e4825c7f073b3474cf6.diff" rel="noreferrer" target="_blank">https://github.com/llvm/llvm-project/commit/c143900a0851b2c7b7d52e4825c7f073b3474cf6.diff</a><br>
<br>
LOG: [llvm-install-name-tool] Merge install-name options<br>
<br>
This diff merges all options for llvm-install-name-tool under a single<br>
function processLoadCommands. Also adds another test case for -add_rpath<br>
option.<br>
<br>
Test plan: make check-all<br>
<br>
Reviewed by: jhenderson, alexshap, smeenai, Ktwu<br>
<br>
Differential Revision: <a href="https://reviews.llvm.org/D82812" rel="noreferrer" target="_blank">https://reviews.llvm.org/D82812</a><br>
<br>
Added: <br>
<br>
<br>
Modified: <br>
llvm/test/tools/llvm-objcopy/MachO/install-name-tool-add-rpath.test<br>
llvm/tools/llvm-objcopy/CopyConfig.cpp<br>
llvm/tools/llvm-objcopy/CopyConfig.h<br>
llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp<br>
<br>
Removed: <br>
<br>
<br>
<br>
################################################################################<br>
diff --git a/llvm/test/tools/llvm-objcopy/MachO/install-name-tool-add-rpath.test b/llvm/test/tools/llvm-objcopy/MachO/install-name-tool-add-rpath.test<br>
index 1435c6b744c8..7b21fdc2e03c 100644<br>
--- a/llvm/test/tools/llvm-objcopy/MachO/install-name-tool-add-rpath.test<br>
+++ b/llvm/test/tools/llvm-objcopy/MachO/install-name-tool-add-rpath.test<br>
@@ -22,6 +22,13 @@<br>
<br>
# NO-INPUT: no input file specified<br>
<br>
+## Add same RPATH twice:<br>
+# RUN: not llvm-install-name-tool -add_rpath @executable_X \<br>
+# RUN: -add_rpath @executable_X %t.i386 2>&1 \<br>
+# RUN: | FileCheck --check-prefix=DOUBLE %s<br>
+<br>
+# DOUBLE: duplicate load command<br>
+<br>
## Check that cmdsize accounts for NULL terminator.<br>
# RUN: yaml2obj %p/Inputs/x86_64.yaml -o %t.x86_64<br>
# RUN: llvm-install-name-tool -add_rpath abcd %t.x86_64<br>
<br>
diff --git a/llvm/tools/llvm-objcopy/CopyConfig.cpp b/llvm/tools/llvm-objcopy/CopyConfig.cpp<br>
index f93406f371d0..1fde54dd290a 100644<br>
--- a/llvm/tools/llvm-objcopy/CopyConfig.cpp<br>
+++ b/llvm/tools/llvm-objcopy/CopyConfig.cpp<br>
@@ -874,42 +874,39 @@ parseInstallNameToolOptions(ArrayRef<const char *> ArgsArr) {<br>
auto Match = [=](StringRef RPath) { return RPath == Old || RPath == New; };<br>
<br>
// Cannot specify duplicate -rpath entries<br>
- auto It1 = find_if(Config.RPathsToUpdate,<br>
- [&Match](const std::pair<StringRef, StringRef> &OldNew) {<br>
- return Match(OldNew.first) || Match(OldNew.second);<br>
- });<br>
+ auto It1 = find_if(<br>
+ Config.RPathsToUpdate,<br>
+ [&Match](const DenseMap<StringRef, StringRef>::value_type &OldNew) {<br>
+ return Match(OldNew.getFirst()) || Match(OldNew.getSecond());<br>
+ });<br>
if (It1 != Config.RPathsToUpdate.end())<br>
- return createStringError(<br>
- errc::invalid_argument,<br>
- "cannot specify both -rpath %s %s and -rpath %s %s",<br>
- It1->first.str().c_str(), It1->second.str().c_str(),<br>
- Old.str().c_str(), New.str().c_str());<br>
+ return createStringError(errc::invalid_argument,<br>
+ "cannot specify both -rpath " + It1->getFirst() +<br>
+ " " + It1->getSecond() + " and -rpath " +<br>
+ Old + " " + New);<br>
<br>
// Cannot specify the same rpath under both -delete_rpath and -rpath<br>
auto It2 = find_if(Config.RPathsToRemove, Match);<br>
if (It2 != Config.RPathsToRemove.end())<br>
- return createStringError(<br>
- errc::invalid_argument,<br>
- "cannot specify both -delete_rpath %s and -rpath %s %s",<br>
- It2->str().c_str(), Old.str().c_str(), New.str().c_str());<br>
+ return createStringError(errc::invalid_argument,<br>
+ "cannot specify both -delete_rpath " + *It2 +<br>
+ " and -rpath " + Old + " " + New);<br>
<br>
// Cannot specify the same rpath under both -add_rpath and -rpath<br>
auto It3 = find_if(Config.RPathToAdd, Match);<br>
if (It3 != Config.RPathToAdd.end())<br>
- return createStringError(<br>
- errc::invalid_argument,<br>
- "cannot specify both -add_rpath %s and -rpath %s %s",<br>
- It3->str().c_str(), Old.str().c_str(), New.str().c_str());<br>
+ return createStringError(errc::invalid_argument,<br>
+ "cannot specify both -add_rpath " + *It3 +<br>
+ " and -rpath " + Old + " " + New);<br>
<br>
- Config.RPathsToUpdate.emplace_back(Old, New);<br>
+ Config.RPathsToUpdate.insert({Old, New});<br>
}<br>
<br>
if (auto *Arg = InputArgs.getLastArg(INSTALL_NAME_TOOL_id))<br>
Config.SharedLibId = Arg->getValue();<br>
<br>
for (auto *Arg : InputArgs.filtered(INSTALL_NAME_TOOL_change)) {<br>
- Config.InstallNamesToUpdate.emplace_back(Arg->getValue(0),<br>
- Arg->getValue(1));<br>
+ Config.InstallNamesToUpdate.insert({Arg->getValue(0), Arg->getValue(1)});<br>
}<br>
<br>
SmallVector<StringRef, 2> Positional;<br>
<br>
diff --git a/llvm/tools/llvm-objcopy/CopyConfig.h b/llvm/tools/llvm-objcopy/CopyConfig.h<br>
index ce119dee5bff..1341dd674c7b 100644<br>
--- a/llvm/tools/llvm-objcopy/CopyConfig.h<br>
+++ b/llvm/tools/llvm-objcopy/CopyConfig.h<br>
@@ -178,8 +178,8 @@ struct CopyConfig {<br>
std::vector<StringRef> DumpSection;<br>
std::vector<StringRef> SymbolsToAdd;<br>
std::vector<StringRef> RPathToAdd;<br>
- std::vector<std::pair<StringRef, StringRef>> RPathsToUpdate;<br>
- std::vector<std::pair<StringRef, StringRef>> InstallNamesToUpdate;<br>
+ DenseMap<StringRef, StringRef> RPathsToUpdate;<br>
+ DenseMap<StringRef, StringRef> InstallNamesToUpdate;<br>
DenseSet<StringRef> RPathsToRemove;<br>
<br>
// install-name-tool's id option<br>
<br>
diff --git a/llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp b/llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp<br>
index 3844b6f62de6..9d0c36630258 100644<br>
--- a/llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp<br>
+++ b/llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp<br>
@@ -42,35 +42,6 @@ static StringRef getPayloadString(const LoadCommand &LC) {<br>
.rtrim('\0');<br>
}<br>
<br>
-static Error removeLoadCommands(const CopyConfig &Config, Object &Obj) {<br>
- DenseSet<StringRef> RPathsToRemove(Config.RPathsToRemove.begin(),<br>
- Config.RPathsToRemove.end());<br>
-<br>
- LoadCommandPred RemovePred = [&RPathsToRemove](const LoadCommand &LC) {<br>
- if (LC.MachOLoadCommand.load_command_data.cmd == MachO::LC_RPATH) {<br>
- StringRef RPath = getPayloadString(LC);<br>
- if (RPathsToRemove.count(RPath)) {<br>
- RPathsToRemove.erase(RPath);<br>
- return true;<br>
- }<br>
- }<br>
- return false;<br>
- };<br>
-<br>
- if (Error E = Obj.removeLoadCommands(RemovePred))<br>
- return E;<br>
-<br>
- // Emit an error if the Mach-O binary does not contain an rpath path name<br>
- // specified in -delete_rpath.<br>
- for (StringRef RPath : Config.RPathsToRemove) {<br>
- if (RPathsToRemove.count(RPath))<br>
- return createStringError(errc::invalid_argument,<br>
- "no LC_RPATH load command with path: %s",<br>
- RPath.str().c_str());<br>
- }<br>
- return Error::success();<br>
-}<br>
-<br>
static Error removeSections(const CopyConfig &Config, Object &Obj) {<br>
SectionPred RemovePred = [](const std::unique_ptr<Section> &) {<br>
return false;<br>
@@ -157,6 +128,103 @@ static LoadCommand buildRPathLoadCommand(StringRef Path) {<br>
return LC;<br>
}<br>
<br>
+static Error processLoadCommands(const CopyConfig &Config, Object &Obj) {<br>
+ // Remove RPaths.<br>
+ DenseSet<StringRef> RPathsToRemove(Config.RPathsToRemove.begin(),<br>
+ Config.RPathsToRemove.end());<br>
+<br>
+ LoadCommandPred RemovePred = [&RPathsToRemove](const LoadCommand &LC) {<br>
+ if (LC.MachOLoadCommand.load_command_data.cmd == MachO::LC_RPATH) {<br>
+ StringRef RPath = getPayloadString(LC);<br>
+ if (RPathsToRemove.count(RPath)) {<br>
+ RPathsToRemove.erase(RPath);<br>
+ return true;<br>
+ }<br>
+ }<br>
+ return false;<br>
+ };<br>
+<br>
+ if (Error E = Obj.removeLoadCommands(RemovePred))<br>
+ return E;<br>
+<br>
+ // Emit an error if the Mach-O binary does not contain an rpath path name<br>
+ // specified in -delete_rpath.<br>
+ for (StringRef RPath : Config.RPathsToRemove) {<br>
+ if (RPathsToRemove.count(RPath))<br>
+ return createStringError(errc::invalid_argument,<br>
+ "no LC_RPATH load command with path: %s",<br>
+ RPath.str().c_str());<br>
+ }<br>
+<br>
+ DenseSet<StringRef> RPaths;<br>
+<br>
+ // Get all existing RPaths.<br>
+ for (LoadCommand &LC : Obj.LoadCommands) {<br>
+ if (LC.MachOLoadCommand.load_command_data.cmd == MachO::LC_RPATH)<br>
+ RPaths.insert(getPayloadString(LC));<br>
+ }<br>
+<br>
+ // Throw errors for invalid RPaths.<br>
+ for (const auto &OldNew : Config.RPathsToUpdate) {<br>
+ StringRef Old, New;<br>
+ std::tie(Old, New) = OldNew;<br>
+ if (RPaths.count(Old) == 0)<br>
+ return createStringError(errc::invalid_argument,<br>
+ "no LC_RPATH load command with path: " + Old);<br>
+ if (RPaths.count(New) != 0)<br>
+ return createStringError(errc::invalid_argument,<br>
+ "rpath " + New +<br>
+ " would create a duplicate load command");<br>
+ }<br>
+<br>
+ // Update load commands.<br>
+ for (LoadCommand &LC : Obj.LoadCommands) {<br>
+ switch (LC.MachOLoadCommand.load_command_data.cmd) {<br>
+ case MachO::LC_ID_DYLIB:<br>
+ if (Config.SharedLibId) {<br>
+ StringRef Id = Config.SharedLibId.getValue();<br>
+ if (Id.empty())<br>
+ return createStringError(errc::invalid_argument,<br>
+ "cannot specify an empty id");<br>
+ updateLoadCommandPayloadString<MachO::dylib_command>(LC, Id);<br>
+ }<br>
+ break;<br>
+<br>
+ case MachO::LC_RPATH: {<br>
+ StringRef RPath = getPayloadString(LC);<br>
+ StringRef NewRPath = Config.RPathsToUpdate.lookup(RPath);<br>
+ if (!NewRPath.empty())<br>
+ updateLoadCommandPayloadString<MachO::rpath_command>(LC, NewRPath);<br>
+ break;<br>
+ }<br>
+<br>
+ // TODO: Add LC_REEXPORT_DYLIB, LC_LAZY_LOAD_DYLIB, and LC_LOAD_UPWARD_DYLIB<br>
+ // here once llvm-objcopy supports them.<br>
+ case MachO::LC_LOAD_DYLIB:<br>
+ case MachO::LC_LOAD_WEAK_DYLIB:<br>
+ StringRef InstallName = getPayloadString(LC);<br>
+ StringRef NewInstallName =<br>
+ Config.InstallNamesToUpdate.lookup(InstallName);<br>
+ if (!NewInstallName.empty())<br>
+ updateLoadCommandPayloadString<MachO::dylib_command>(LC,<br>
+ NewInstallName);<br>
+ break;<br>
+ }<br>
+ }<br>
+<br>
+ // Add new RPaths.<br>
+ for (StringRef RPath : Config.RPathToAdd) {<br>
+ if (RPaths.count(RPath) != 0)<br>
+ return createStringError(errc::invalid_argument,<br>
+ "rpath " + RPath +<br>
+ " would create a duplicate load command");<br>
+ RPaths.insert(RPath);<br>
+ Obj.addLoadCommand(buildRPathLoadCommand(RPath));<br>
+ }<br>
+<br>
+ return Error::success();<br>
+}<br>
+<br>
static Error dumpSectionToFile(StringRef SecName, StringRef Filename,<br>
Object &Obj) {<br>
for (LoadCommand &LC : Obj.LoadCommands)<br>
@@ -273,34 +341,6 @@ static Error handleArgs(const CopyConfig &Config, Object &Obj) {<br>
for (std::unique_ptr<Section> &Sec : LC.Sections)<br>
Sec->Relocations.clear();<br>
<br>
- for (LoadCommand &LC : Obj.LoadCommands) {<br>
- switch (LC.MachOLoadCommand.load_command_data.cmd) {<br>
- case MachO::LC_ID_DYLIB:<br>
- if (Config.SharedLibId) {<br>
- StringRef Id = Config.SharedLibId.getValue();<br>
- if (Id.empty())<br>
- return createStringError(errc::invalid_argument,<br>
- "cannot specify an empty id");<br>
- updateLoadCommandPayloadString<MachO::dylib_command>(LC, Id);<br>
- }<br>
- break;<br>
-<br>
- // TODO: Add LC_REEXPORT_DYLIB, LC_LAZY_LOAD_DYLIB, and LC_LOAD_UPWARD_DYLIB<br>
- // here once llvm-objcopy supports them.<br>
- case MachO::LC_LOAD_DYLIB:<br>
- case MachO::LC_LOAD_WEAK_DYLIB:<br>
- StringRef Old, New;<br>
- StringRef CurrentInstallName = getPayloadString(LC);<br>
- for (const auto &InstallNamePair : Config.InstallNamesToUpdate) {<br>
- std::tie(Old, New) = InstallNamePair;<br>
- if (CurrentInstallName == Old) {<br>
- updateLoadCommandPayloadString<MachO::dylib_command>(LC, New);<br>
- break;<br>
- }<br>
- }<br>
- }<br>
- }<br>
-<br>
for (const auto &Flag : Config.AddSection) {<br>
std::pair<StringRef, StringRef> SecPair = Flag.split("=");<br>
StringRef SecName = SecPair.first;<br>
@@ -311,45 +351,9 @@ static Error handleArgs(const CopyConfig &Config, Object &Obj) {<br>
return E;<br>
}<br>
<br>
- if (Error E = removeLoadCommands(Config, Obj))<br>
+ if (Error E = processLoadCommands(Config, Obj))<br>
return E;<br>
<br>
- StringRef Old, New;<br>
- for (const auto &OldNew : Config.RPathsToUpdate) {<br>
- std::tie(Old, New) = OldNew;<br>
-<br>
- auto FindRPathLC = [&Obj](StringRef RPath) {<br>
- return find_if(Obj.LoadCommands, [=](const LoadCommand &LC) {<br>
- return LC.MachOLoadCommand.load_command_data.cmd == MachO::LC_RPATH &&<br>
- getPayloadString(LC) == RPath;<br>
- });<br>
- };<br>
-<br>
- auto NewIt = FindRPathLC(New);<br>
- if (NewIt != Obj.LoadCommands.end())<br>
- return createStringError(errc::invalid_argument,<br>
- "rpath " + New +<br>
- " would create a duplicate load command");<br>
-<br>
- auto OldIt = FindRPathLC(Old);<br>
- if (OldIt == Obj.LoadCommands.end())<br>
- return createStringError(errc::invalid_argument,<br>
- "no LC_RPATH load command with path: " + Old);<br>
-<br>
- updateLoadCommandPayloadString<MachO::rpath_command>(*OldIt, New);<br>
- }<br>
-<br>
- for (StringRef RPath : Config.RPathToAdd) {<br>
- for (LoadCommand &LC : Obj.LoadCommands) {<br>
- if (LC.MachOLoadCommand.load_command_data.cmd == MachO::LC_RPATH &&<br>
- RPath == getPayloadString(LC)) {<br>
- return createStringError(errc::invalid_argument,<br>
- "rpath " + RPath +<br>
- " would create a duplicate load command");<br>
- }<br>
- }<br>
- Obj.addLoadCommand(buildRPathLoadCommand(RPath));<br>
- }<br>
return Error::success();<br>
}<br>
<br>
<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div></div>