[llvm-install-name-tool] Add -change option

[sameerarora101]

Summary

Implement -change option for install-name-tool. The behavior exactly
matches that of cctools. Depends on D82410.

[sameerarora101]

llvm/test/tools/llvm-objcopy/MachO/install-name-tool-change.test

2

NOTE: I have currently modeled -change option's behavior exactly as that of cctools. We thought it would be better to keep the same behavior in order to maintain consistency with the usage of the tools.

[sameerarora101]

Rebasing (for harbormaster)

[jhenderson]

llvm/test/tools/llvm-objcopy/MachO/install-name-tool-change.test

68–69

To avoid any possible confusion, I'd recommend changing the output paths here to not match those in the input. The fact that they match suggests that's important otherwise.

llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp

274–296

I think it might make sense to move this logic inside the loop below - it probably doesn't make sense to loop over the load commands multiple times if you specify multiple options. The same might apply in relation the rpath stuff too.

What do you think?

[sameerarora101]

llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp

274–296

Yup, I moved the -id logic in the same loop as -change now.

However, I don't think we can do the same for rpath stuff because of all the error checking we need to perform. For eg., in case of -add_rpath, for each rpath to be added, we need to iterate through all LCs anyway so as to make sure we aren't duplicating rpaths. Similarly, in case of -delete_rpath we need to iterate through the whole list to throw an error in case we aren't able to find the rpath to be deleted.

But yeah, in case of -id and -change there is no such error checking that requires checking over all LCs, so I was able to put them in the same loop easily.

[sameerarora101]

Putting -id and -change in the same loop

[sameerarora101]

Rebasing

[jhenderson]

LGTM. As mentioned inline, I think you could do a little extra refactoring to improve things further, but I'd leave that to a subsequent patch.

llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp

274–296

I think it might be possible with some extra refactoring, but I'd recommend deferring that to a separate patch. What I'm thinking in semi-pseudo code terms is roughly:

std::unordered_set<StringRef> RPaths; // Pick your container of choice here. Also useful for the add rpath loop.
std::unordered_set<StirngRef> OriginalRPaths;
...
case MachO::LC_RPATH:
  StringRef Current = getLoadCommandPayloadString(LC);
  auto FoundArg = find_if(Config.RPathsToUpdate, IsFirstMatching);
  OriginalRPaths.insert(Current);
  if (!RPaths.insert(Current).second) // If already in RPaths...
    // report duplicate rpath error.
  if (FoundArg != Config.RPathsToUpdate.end())
    updateLoadCommandPayloadString<MachO::rpath_command>(Current, FoundArg->second);
...
// After loop.
for (const auto &RPair : Config.RPathsToUpdate) {
  if (OriginalRPaths.count(RPair.first) == 0)
    // Did not find rpath error.
}

296

To avoid confusion in the future, if this switch gets extended with more cases below, I'd be inclined to put a break here too.

[sameerarora101]

llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp

274–296

I see now. Yup, this would be a nice change. I'll push a separate diff for this change. Thanks a lot 😊

[sameerarora101]

rebasing over latest origin/master

[sameerarora101]

resolving clang-tidy

[sameerarora101]

llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp

274–296

Ok, merged -rpath option inside the same loop as -id and -change here https://reviews.llvm.org/D82812. I refactored common logic too. In fact, it even simplified -add_rpath a little. Thanks a lot 😄 and please review!

[smeenai]

LGTM

llvm/test/tools/llvm-objcopy/MachO/install-name-tool-change.test

+## This test checks updating a dependent shared library install name in a MachO binary.
+
+# RUN: yaml2obj %s -o %t
+
+## Specifying -change once:
+# RUN: cp %t %t.copy
+# RUN: llvm-install-name-tool -change /usr/dylib/LOAD /usr/long/long/dylib/LOAD %t.copy
+# RUN: llvm-objdump -p %t.copy | FileCheck %s --check-prefix=CHANGE --implicit-check-not=/usr
+
+# CHANGE: /usr/long/long/dylib/LOAD
+# CHANGE: /usr/dylib/WEAK
+
+## Specifying -change multiple times:
+# RUN: cp %t %t.copy
+# RUN: llvm-install-name-tool -change /usr/dylib/WEAK /usr/sh/WEAK \
+# RUN:                        -change /usr/dylib/LOAD /usr/sh/LOAD  %t.copy
+# RUN: llvm-objdump -p %t.copy | FileCheck %s --check-prefix=CHANGE-MULTIPLE --implicit-check-not=/usr
+
+# CHANGE-MULTIPLE: /usr/sh/LOAD
+# CHANGE-MULTIPLE: /usr/sh/WEAK
+
+## Changing same dependent library name multiple times:
+# RUN: cp %t %t.copy
+# RUN: llvm-install-name-tool -change /usr/dylib/LOAD /usr/LOAD \
+# RUN:                        -change /usr/dylib/LOAD /usr/XXXX %t.copy
+# RUN: llvm-objdump -p %t.copy | FileCheck %s --check-prefix=CHANGE-REPEAT --implicit-check-not=/usr
+
+# CHANGE-REPEAT: /usr/LOAD
+# CHANGE-REPEAT: /usr/dylib/WEAK
+
+## Specifying dependent library names in a chain:
+# RUN: cp %t %t.copy
+# RUN: llvm-install-name-tool -change /usr/dylib/LOAD /usr/XX/LOAD \
+# RUN:                        -change /usr/XX/LOAD /usr/YY/LOAD %t.copy
+# RUN: llvm-objdump -p %t.copy | FileCheck %s --check-prefix=CHANGE-CHAIN --implicit-check-not=/usr
+
+# CHANGE-CHAIN: /usr/XX/LOAD
+# CHANGE-CHAIN: /usr/dylib/WEAK
+
+## Changing multiple dependent library names where one exists and the other doesn't:
+# RUN: cp %t %t.copy
+# RUN: llvm-install-name-tool -change /usr/dylib/LOAD /usr/JOJO/LOAD \
+# RUN:                        -change /usr/BIZARRE /usr/KOKO/LOAD %t.copy
+# RUN: llvm-objdump -p %t.copy | FileCheck %s --check-prefix=CHANGE-SWITCH --implicit-check-not=/usr
+
+# CHANGE-SWITCH: /usr/JOJO/LOAD
+# CHANGE-SWITCH: /usr/dylib/WEAK
+
+## Changing to a common dependent library name:
+# RUN: cp %t %t.copy
+# RUN: llvm-install-name-tool -change /usr/dylib/LOAD /usr/COMMON \
+# RUN:                        -change /usr/dylib/WEAK /usr/COMMON %t.copy
+# RUN: llvm-objdump -p %t.copy | FileCheck %s --check-prefix=CHANGE-COMMON --implicit-check-not=/usr
+
+# CHANGE-COMMON: /usr/COMMON
+# CHANGE-COMMON: /usr/COMMON
+
+## Change all common dependent library names at once:
+# RUN: llvm-install-name-tool -change /usr/COMMON /usr/ONCE %t.copy
+# RUN: llvm-objdump -p %t.copy | FileCheck %s --check-prefix=CHANGE-ONCE --implicit-check-not=/usr
+
+# CHANGE-ONCE: /usr/ONCE
+# CHANGE-ONCE: /usr/ONCE
+
+## Check that -change option has no effect if the binary doesn't contain old install name:
+# RUN: cp %t %t1
+# RUN: llvm-install-name-tool -change /usr/JOJO/LOAD /usr/XX/LOAD \
+# RUN:                        -change /usr/KOKO/WEAK /usr/YY/WEAK %t
+# RUN: cmp %t %t1
+
+## Missing a -change argument:
+# RUN: not llvm-install-name-tool %t -change /usr/ONCE 2>&1 | \
+# RUN:   FileCheck %s --check-prefix=MISSING
+
+## Missing both -change arguments:
+# RUN: not llvm-install-name-tool %t -change 2>&1 | \
+# RUN:   FileCheck %s --check-prefix=MISSING
+
+# MISSING: missing argument to -change option
+
+--- !mach-o
+FileHeader:
+  magic:           0xFEEDFACF
+  cputype:         0x01000007
+  cpusubtype:      0x00000003
+  filetype:        0x00000001
+  ncmds:           2
+  sizeofcmds:      80
+  flags:           0x00002000
+  reserved:        0x00000000
+LoadCommands:
+  - cmd:                        LC_LOAD_DYLIB
+    cmdsize:                    40
+    dylib:
+        name:                   24
+        timestamp:              2
+        current_version:        82115073
+        compatibility_version:  65536
+    PayloadString:              '/usr/dylib/LOAD'
+  - cmd:                        LC_LOAD_WEAK_DYLIB
+    cmdsize:                    40
+    dylib:
+        name:                   24
+        timestamp:              2
+        current_version:        82115073
+        compatibility_version:  65536
+    PayloadString:              '/usr/dylib/WEAK'

llvm/tools/llvm-objcopy/CopyConfig.h

struct CopyConfig {
   Optional<StringRef> NewSymbolVisibility;
 
   // Repeated options
   std::vector<StringRef> AddSection;
   std::vector<StringRef> DumpSection;
   std::vector<StringRef> SymbolsToAdd;
   std::vector<StringRef> RPathToAdd;
   std::vector<std::pair<StringRef, StringRef>> RPathsToUpdate;
+  std::vector<std::pair<StringRef, StringRef>> InstallNamesToUpdate;
   DenseSet<StringRef> RPathsToRemove;
 
   // install-name-tool's id option
   Optional<StringRef> SharedLibId;
 
   // Section matchers
   NameMatcher KeepSection;
   NameMatcher OnlySection;

llvm/tools/llvm-objcopy/CopyConfig.cpp

if (It3 != Config.RPathToAdd.end())
           It3->str().c_str(), Old.str().c_str(), New.str().c_str());
 
     Config.RPathsToUpdate.emplace_back(Old, New);
   }
 
   if (auto *Arg = InputArgs.getLastArg(INSTALL_NAME_TOOL_id))
     Config.SharedLibId = Arg->getValue();
 
+  for (auto *Arg : InputArgs.filtered(INSTALL_NAME_TOOL_change)) {
+    Config.InstallNamesToUpdate.emplace_back(Arg->getValue(0),
+                                             Arg->getValue(1));
+  }
+
   SmallVector<StringRef, 2> Positional;
   for (auto Arg : InputArgs.filtered(INSTALL_NAME_TOOL_UNKNOWN))
     return createStringError(errc::invalid_argument, "unknown argument '%s'",
                              Arg->getAsString(InputArgs).c_str());
   for (auto Arg : InputArgs.filtered(INSTALL_NAME_TOOL_INPUT))
     Positional.push_back(Arg->getValue());
   if (Positional.empty())
     return createStringError(errc::invalid_argument, "no input file specified");

llvm/tools/llvm-objcopy/InstallNameToolOpts.td

def delete_rpath: Option<["-", "--"], "delete_rpath", KIND_SEPARATE>,
                   HelpText<"Delete specified rpath">;
 
 def rpath: MultiArg<["-", "--"], "rpath", 2>,
            HelpText<"Change rpath path name">;
 
 def id : Option<["-","--"], "id", KIND_SEPARATE>,
                  HelpText<"Change dynamic shared library id">;
 
+def change: MultiArg<["-", "--"], "change", 2>,
+            HelpText<"Change dependent shared library install name">;
+
 def version : Flag<["--"], "version">,
               HelpText<"Print the version and exit.">;

llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp

static Error handleArgs(const CopyConfig &Config, Object &Obj) {
 
   updateAndRemoveSymbols(Config, Obj);
 
   if (Config.StripAll)
     for (LoadCommand &LC : Obj.LoadCommands)
       for (std::unique_ptr<Section> &Sec : LC.Sections)
         Sec->Relocations.clear();
 
+  for (LoadCommand &LC : Obj.LoadCommands) {
+    switch (LC.MachOLoadCommand.load_command_data.cmd) {
+    case MachO::LC_ID_DYLIB:
       if (Config.SharedLibId) {
         StringRef Id = Config.SharedLibId.getValue();
         if (Id.empty())
           return createStringError(errc::invalid_argument,
                                    "cannot specify an empty id");
-    for (LoadCommand &LC : Obj.LoadCommands) {
-      if (LC.MachOLoadCommand.load_command_data.cmd == MachO::LC_ID_DYLIB) {
         updateLoadCommandPayload<MachO::dylib_command>(LC, Id);
-        break;
+      }
+
+    case MachO::LC_LOAD_DYLIB:
+    case MachO::LC_LOAD_WEAK_DYLIB:
+      StringRef Old, New;
+      StringRef CurrentInstallName = getLoadCommandPayload(LC);
+      for (const auto &InstallNamePair : Config.InstallNamesToUpdate) {
+        std::tie(Old, New) = InstallNamePair;
+        if (CurrentInstallName == Old) {
+          updateLoadCommandPayload<MachO::dylib_command>(LC, New);
+          break;
+        }
       }
     }
   }
 
   for (const auto &Flag : Config.AddSection) {
     std::pair<StringRef, StringRef> SecPair = Flag.split("=");
     StringRef SecName = SecPair.first;
     StringRef File = SecPair.second;
     if (Error E = isValidMachOCannonicalName(SecName))