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

[Sameer Arora via Phabricator via llvm-commits]

sameerarora101 marked 8 inline comments as done.
sameerarora101 added inline comments.


================
Comment at: llvm/tools/llvm-objcopy/CopyConfig.cpp:852
+    StringRef NewRPath = Arg->getValue(1);
+
+    // Cannot specify duplicate -rpath entries
----------------
alexshap wrote:
> 
>   # 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); });
> ```
nice, thanks! 😊 


================
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);
----------------
alexshap wrote:
> 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;
>     ...
> }
> ```
So it turns out the way to update LC name follows the same logic (as above) for all the options `-rpath`, `-id`, and `-change` for `llvm-install-name-tool`. Here is how I was thinking about using it :

`-rpath` : `updateLoadCommandName<MachO::rpath_command>(&LC, NewRPath);`,

`-id` : `updateLoadCommandName<MachO::dylib_command>(&LC, NewID);`

`-change` : `updateLoadCommandName<MachO::dylib_command>(&LC, NewInstallName);`

This allowed me to refactor the common logic of updating the name payload of any load command (instead of making it specific for rpath). Would this not be ok?


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp:269
 
+  for (std::pair<StringRef, StringRef> RPair : Config.RPathsToUpdate) {
+    StringRef OldRPath = RPair.first;
----------------
alexshap wrote:
> ```
> StringRef Old, New;
> for (std::tie(Old, New) : Config.RPathsToUpdate) {
>     ...
> }
> 
> ```
`for (std::tie(Old, New) : Config.RPathsToUpdate)` is perhaps not allowed : it says `for range declaration must declare a variable`. 

I have updated it to
```
StringRef OldRPath, NewRPath;
for (auto &RPair : Config.RPathsToUpdate) {
   std::tie(OldRPath, NewRPath) = RPair;
   ...
}
```
I hope this works?


================
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()),
----------------
alexshap wrote:
> 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);
> }
> ```
Ok, I refactored getting the RPath construction into a separate function and updated it to be used by `add_rpath`, `delete_rpath` and `rpath` options.

For the reorganization, we need to check that `New` doesn't exists for **any** of the LC_RPATH commands. Please correct me in case I am wrong, but it seems that the above suggested code finds an LC_RPATH that matches `Old`. However, above, we throw an error only when either we can't find such a command (`It == Obj.LoadCommands.end()`) or if the command's rpath equals the `New` value(`getRPath(*It) == New`). But we need to throw the latter error in case **any** command's value equals `New`. Here is what I have done now:

```
StringRef Old, New;
  for (auto &RPair : Config.RPathsToUpdate) {
    std::tie(Old, New) = RPair;

    auto Match = [&Obj] (StringRef RPath) {
      return find_if(Obj.LoadCommands, [=](const LoadCommand &LC) {
        return LC.MachOLoadCommand.load_command_data.cmd == MachO::LC_RPATH &&
               getRPath(LC) == RPath;
      });
    };

    auto NewIt = Match(New);
    if (NewIt != Obj.LoadCommands.end())
      return Error

    auto OldIt = Match(Old);
    if (OldIt == Obj.LoadCommands.end())
        return Error

    updateLoadCommandName<MachO::rpath_command>(&(*OldIt), New);
  }

```
Would this work? Thanks.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82051