[PATCH] D82812: [llvm-install-name-tool] Merge rpath with id/change

[Sameer Arora via Phabricator via llvm-commits]

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


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp:275
 
+  std::set<std::string> OriginalRPaths;
+  auto FindPair =
----------------
Ktwu wrote:
> alexshap wrote:
> > jhenderson wrote:
> > > sameerarora101 wrote:
> > > > NOTE: Cannot store `StringRef` in OriginlRPaths (i.e cannot have `set<StringRef> OriginalRPaths`). This is because the StringRef `CurrentRPath` is a reference to the string stored inside LC and thus when we change the payload (via `-rpath` option) `CurrentRPath` is also updated to hold a reference to the new string. Hence I need to store the actual strings inside the set.
> > > You probably don't want a `std::set` here. Have you reviewed [[ https://llvm.org/docs/ProgrammersManual.html#set-like-containers-std-set-smallset-setvector-etc | the Programmer's Manual ]] on what set type to use?
> > 1. I wanted to mention it on one of the previous diffs but looks like I forgot - it would be better to place all the details related to updating load commands into a separate helper function rather than directly into handleArgs.
> >    
> > ```
> > static Error updateLoadCommands(const CopyConfig &Config, Object &Obj)
> > ```
> > handleArgs would invoke this function (updateLoadCommands) similarly to removeSections, updateAndRemoveSymbols etc.
> > 
> > Probably it would be even better first to refactor the code this way (on a separate diff) before moving forward if you don't mind
> > (now handleArgs is accumulating the details it should not really be aware of)
> > 
> > 2. Please, take a look at StringSet, StringMap and their interfaces 
> > (in particular, if I am not mistaken they would help you avoid some str() calls below)
> >  
> > Also this probably suggests that Config.RPathsToUpdate, Config.RPathsToUpdate should really be StringMap-s.
> > Also this probably suggests that Config.RPathsToUpdate, Config.RPathsToUpdate should really be StringMap-s.
> 
> +1, although does StringMap have deterministic iteration? Maybe SetVector would work as well?
yup, I have replace it with a `DenseSet<StringRef>`


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp:275
 
+  std::set<std::string> OriginalRPaths;
+  auto FindPair =
----------------
sameerarora101 wrote:
> Ktwu wrote:
> > alexshap wrote:
> > > jhenderson wrote:
> > > > sameerarora101 wrote:
> > > > > NOTE: Cannot store `StringRef` in OriginlRPaths (i.e cannot have `set<StringRef> OriginalRPaths`). This is because the StringRef `CurrentRPath` is a reference to the string stored inside LC and thus when we change the payload (via `-rpath` option) `CurrentRPath` is also updated to hold a reference to the new string. Hence I need to store the actual strings inside the set.
> > > > You probably don't want a `std::set` here. Have you reviewed [[ https://llvm.org/docs/ProgrammersManual.html#set-like-containers-std-set-smallset-setvector-etc | the Programmer's Manual ]] on what set type to use?
> > > 1. I wanted to mention it on one of the previous diffs but looks like I forgot - it would be better to place all the details related to updating load commands into a separate helper function rather than directly into handleArgs.
> > >    
> > > ```
> > > static Error updateLoadCommands(const CopyConfig &Config, Object &Obj)
> > > ```
> > > handleArgs would invoke this function (updateLoadCommands) similarly to removeSections, updateAndRemoveSymbols etc.
> > > 
> > > Probably it would be even better first to refactor the code this way (on a separate diff) before moving forward if you don't mind
> > > (now handleArgs is accumulating the details it should not really be aware of)
> > > 
> > > 2. Please, take a look at StringSet, StringMap and their interfaces 
> > > (in particular, if I am not mistaken they would help you avoid some str() calls below)
> > >  
> > > Also this probably suggests that Config.RPathsToUpdate, Config.RPathsToUpdate should really be StringMap-s.
> > > Also this probably suggests that Config.RPathsToUpdate, Config.RPathsToUpdate should really be StringMap-s.
> > 
> > +1, although does StringMap have deterministic iteration? Maybe SetVector would work as well?
> yup, I have replace it with a `DenseSet<StringRef>`
1. ok, so i refactored the code for updating LCs into a separate function `updateLoadCommands` and call that from `handleArgs`.

> Also this probably suggests that Config.RPathsToUpdate, Config.RPathsToUpdate should really be StringMap-s.

I agree, but I think I should do that in a separate diff considering that it would require changing the way I parse options in `CopyConfig.cpp`. What do you think?



================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp:295
 
+    case MachO::LC_RPATH: {
+      StringRef CurrentRPath = getPayloadString(LC);
----------------
jhenderson wrote:
> Ktwu wrote:
> > nit: are these braces intentional? It's inconsistent with the other cases.
> I think the braces are required because of the declaration of `CurrentRPath` in this case statement.
yup, that is indeed the case.


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp:296
+    case MachO::LC_RPATH: {
+      StringRef CurrentRPath = getPayloadString(LC);
+      OriginalRPaths.insert(CurrentRPath.str());
----------------
Ktwu wrote:
> I think you have some options available depending on how clever you want to get with sets if you care about minimizing string cloning. Instead of tracking original rpaths, you can track operations on the config rpaths, for example (those strings shouldn't change unexpectedly beneath you, right)?
> 
> ```
> std::set<StringRef> UpdatedRPaths;
> 
> case Macho::LC_RPATH:
>   StringRef CurrentRPath = ...
>   auto FoundArg = ...
>   if (FoundArg != end) {
>     if (CurrentRPath in UpdatedRPaths) {
>       // duplicate scenario
>     }
>     if (CurrentRPath in RPathToAdd) {
>       // Also a dup scenario
>     }
>     updateLoadCommandPayloadString()
>     UpdatedRPaths.insert(FoundArg->first);
>   }
> ...
> 
> for (const auto &OldNew : Config.RPathsToUpdate) {
>   std::tie(Old, New) = OldNew;
>   if (Old not in UpdatedMap) {
>     // no load command scenario
>   }
>   // dup scenario handled above
> }
> 
> ...
> std::set<StringRef> AddedRPaths
> for (StringRef RPath : Config.RPathToAdd) {
>   if (RPath in AddedRPaths) {
>     // dup
>   }
>   AddedRPaths.insert(RPath);
> }
> ```
> 
> 
Thanks a lot for the clever tip, `UpdatedRPaths.insert(FoundArg->first)` was really helpful. However, I am not sure if I understood the logic completely (sorry if I missed something 😔). In particular, say I have 
2 LC:  `lcA` and `lcB` 
and 1 `-rpath` option :`lcB -> lcA`

We want this to throw an error but it is currently passing with the above logic.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82812