[PATCH] D82613: [llvm-install-name-tool] Add -change option

[James Henderson via Phabricator via llvm-commits]

jhenderson accepted this revision.
jhenderson added a comment.
This revision is now accepted and ready to land.

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.



================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp:280-283
       if (LC.MachOLoadCommand.load_command_data.cmd == MachO::LC_ID_DYLIB) {
         updateLoadCommandPayload<MachO::dylib_command>(LC, Id);
         break;
       }
----------------
sameerarora101 wrote:
> jhenderson wrote:
> > 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?
> 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.
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.
}
```


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp: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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82613