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

[Sameer Arora via Phabricator via llvm-commits]

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


================
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;
       }
----------------
jhenderson wrote:
> 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.
> }
> ```
I see now. Yup, this would be a nice change. I'll push a separate diff for this change. Thanks a lot 😊


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82613