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

[Sameer Arora via Phabricator via llvm-commits]

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


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp:25
 
+StringRef getRPath(const LoadCommand &LC) {
+  assert(LC.MachOLoadCommand.load_command_data.cmd == MachO::LC_RPATH &&
----------------
alexshap wrote:
> nit: static
> 
> P.S. but, please, take a look at my comment below (line 278), let's try to understand the broader picture.
> It doesn't mean that something is wrong now but it's necessary to think about the best way to organize this code
> (to improve readability and avoid code duplication in the future).
ok thanks 😊 I have refactored this too we'll be needing it for `-change` option as well. I have renamed it to `getLoadCommandPayload`. Is this fine?


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp:278
+
+    auto Match = [&Obj](StringRef RPath) {
+      return find_if(Obj.LoadCommands, [=](const LoadCommand &LC) {
----------------
alexshap wrote:
> I'm not sure that Match is the best possible name here (it's more like findLoadCommand, but this requires more thinking), 
> but, more importantly,  it's necessary to find out how the case of dylib_command differs from rpath_command.
> Based on this information one will be able to refactor this code / make an informed decision - should it be generalized or not.
> 
> 
So it seems that this should not be generalized as `-id` and `-change` would not be making use of it. This is because:

- this current `Match` function takes in a `StringRef RPath` and checks for LC such that LC is of type `MachO::LC_RPATH` and matches this `RPath` passed in as argument.

- for `-id`: I need to search for a LC such that `LC.MachOLoadCommand.load_command_data.cmd == MachO::LC_ID_DYLIB`. Since there can only be one such LC, we don't need to pass in another `StringRef Name` as an argument. And so we wouldn't be using the `Match` function as we don't wish to pass in an argument to it.

- for `-change`: (currently I am matching the behavior of this option with that of XCode's). To match XCode's behavior, implementation of this option involves iterating over LCs **before** iterating over provided `(old, new)` install name pairs. More precisely,

here is what we are doing here in case of `-rpath`:
```
for (RPair : Config.RPathsToUpdate) {.                     // iterate over provided (old,new) pairs
  ...
  find_if( Obj.LoadCommands, ...)                          // iterate over all LCs to find the right LC
  ...
```

However, the iteration would be reversed in case of `-change` option as follows: 
```
for (LoadCommand &LC : Obj.LoadCommands) {.                       // iterate over all LCs first
  ...
  for (auto InstallNamePair : Config.InstallNamesToUpdate)        // for each LC now, find the (old,new) pair
  ...
```

You see how the order of for loops is reversed. This reversed iteration is necessary in case of `-change` option to match the behavior of XCode's `install_name_tool`. And because of this, we wouldn't be able to use the `Match` function above (as that requires taking in a name argument and iterating over LCs after). 

In light of this I have currently renamed `Match` to `FindRPathLC` (as it is rpath specific). What do you think? 

Also. in case we do end up changing `-change` option's behavior in a later diff, I can then perhaps refactor the common code out later? (For now though, it doesn't seems to be needing refactoring.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82051