[PATCH] D82051: [llvm-install-name-tool] Add -rpath option

[Alexander Shaposhnikov via Phabricator via llvm-commits]

alexshap requested changes to this revision.
alexshap added inline comments.
This revision now requires changes to proceed.


================
Comment at: llvm/tools/llvm-objcopy/CopyConfig.cpp:852
+    StringRef NewRPath = Arg->getValue(1);
+
+    // Cannot specify duplicate -rpath entries
----------------

  # llvm::find_if - since the code is already inside the namespace "llvm" the prefix is unnecessary. 

  # StringRef can be passed by value 

  # You can reuse your code a tiny bit further + I would probably rename some things here:

```
auto Match  = [=](StringRef RPath) { return RPath == Old || RPath == New; };
    
auto It1 = find_if(Config.RPathsToUpdate, [](const std::pair<StringRef, StringRef> & OldNew) { 
                                      return Match(OldNew.first) || Match(OldNew.second); });
```


================
Comment at: llvm/tools/llvm-objcopy/CopyConfig.cpp:888
+
+    Config.RPathsToUpdate.push_back(std::make_pair(OldRPath, NewRPath));
+  }
----------------
Config.RPathsToUpdate.emplace_back(Old, New);


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp:120
+template <typename T>
+static void updateLoadCommandName(LoadCommand *LC, StringRef NewName) {
+  uint32_t NewCmdsize = alignTo(sizeof(T) + NewName.size() + 1, 8);
----------------
Different load commands have very different payloads, so i think this function is a bit misleading.
What about this:
```
static void updateRPathLoadCommand(LoadCommand &LC, StringRef NewRPath) {
    assert( ....  ); // make sure that LC is indeed LC_RPATH;
    ...
}
```


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp:269
 
+  for (std::pair<StringRef, StringRef> RPair : Config.RPathsToUpdate) {
+    StringRef OldRPath = RPair.first;
----------------
```
StringRef Old, New;
for (std::tie(Old, New) : Config.RPathsToUpdate) {
    ...
}

```


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp:276
+      if (LC.MachOLoadCommand.load_command_data.cmd == MachO::LC_RPATH) {
+        StringRef CurrentRPath =
+            StringRef(reinterpret_cast<char *>(LC.Payload.data()),
----------------
If I'm not mistaken there are multiple places where this construction is used, so i think we need to factor out it into a helper function.
```
     StringRef getRPath(const LoadCommand &LC) {
         assert(LC.MachOLoadCommand.load_command_data.cmd == MachO::LC_RPATH &&
                   "LC_RPATH is expected");
         return StringRef(reinterpret_cast<char *>(LC.Payload.data()),
                                   LC.Payload.size()).rtrim('\0');
     }
```

Plus I would slightly reorganize the code (see below):

```
StringRef Old, New;
for (std::tie(Old, New) : Config.RPathsToUpdate) {
     auto It = find_if(Obj.LoadCommands, [](const LoadCommand &LC) { 
                              return LC.MachOLoadCommand.load_command_data.cmd == MachO::LC_RPATH &&
                                         getRPath(LC) == Old); });
     if (It == Obj.LoadCommands.end())
         ...
     if (getRPath(*It) == New)
        ...
     updateRPathLoadCommand(*It, New);
}
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82051/new/

https://reviews.llvm.org/D82051