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

[Kellie Medlin via Phabricator via llvm-commits]

Ktwu added inline comments.


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp:275
 
+  std::set<std::string> OriginalRPaths;
+  auto FindPair =
----------------
sameerarora101 wrote:
> 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?
> 
> What do you think?

Definitely do that in a separate diff -- it's much better to keep refactoring changes like that confined to their own diff instead of mixed up with new feature work.


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp:296
+    case MachO::LC_RPATH: {
+      StringRef CurrentRPath = getPayloadString(LC);
+      OriginalRPaths.insert(CurrentRPath.str());
----------------
sameerarora101 wrote:
> 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.
Ah, my pseudocode is bad, sorry! You already fixed the crux of the problem I was talking about with the DenseSet<StringRef; no need to worry about my idea.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82812