[PATCH] D81527: [llvm-install-name-tool] Add `delete_rpath` option

[Sameer Arora via Phabricator via llvm-commits]

sameerarora101 marked an inline comment as done.
sameerarora101 added inline comments.


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp:26-45
+  LoadCommandPred RemovePred = [](const LoadCommand &) { return false; };
+  DenseSet<StringRef> RPathsToRemove;
+
+  if (!Config.RPathsToRemove.empty()) {
+    RPathsToRemove.insert(Config.RPathsToRemove.begin(),
+                          Config.RPathsToRemove.end());
+    RemovePred = [&RPathsToRemove](const LoadCommand &LC) {
----------------
sameerarora101 wrote:
> sameerarora101 wrote:
> > alexshap wrote:
> > > jhenderson wrote:
> > > > This whole block seems unnecessarily overcomplicated to me.
> > > > 
> > > > 1) Make Config.RPathsToRemove a set and then you don't have to copy things from one set to another - you can just use the config variable directly.
> > > > 2) There's no need for the `if (!Config.RPathsToRemove.empty())` check, since it is safe to do everything here whether it's empty or not (the insert disappears, but would be safe, and defining the lambda is harmless and trivial.
> > > > 3) By getting rid of the "if empty" check, you can also get rid of the `return false` lambda.
> > > > 4) `trim(0)` - I assume you mean `trim('\0')`? If so, use the char directly rather than an integer literal, since you are dealing with an array of chars. This makes it easier to follow. I am assuming here that `LC.Payload.size()` includes a trailing null byte you want to get rid of. Do you also mean to get rid of any leading null bytes too? If not, you might be better off using `rtrim`.
> > > After some thinking it seems to me that having the original list of specified rpaths can be useful since it enables us to report the **first** option causing the issue. If we drop that error reporting then indeed it makes sense to simply use StringSet for storing RPATHs in CopyConfig and use that set directly.
> > > 
> > > 2., 3. - I also think that !Config.RPathsToRemove.empty() is unnecessary
> > > 
> > > If I am not mistaken there is a tiny bug in buildRPathLoadCommand function (I've just spotted it) - would be good to fix separately.
> > > 
> > >    RPathLC.cmdsize = alignTo(sizeof(MachO::rpath_command) + Path.size(), 8); 
> > > 
> > > should be replaced with
> > > 
> > >    RPathLC.cmdsize = alignTo(sizeof(MachO::rpath_command) + Path.size() + 1, 8);
> > > 
> > > this is what LD64 does (see ld64/src/ld/HeaderAndLoadCommands.hpp) : 
> > >     
> > >     template <typename A>
> > >     uint8_t* HeaderAndLoadCommandsAtom<A>::copyRPathLoadCommand(uint8_t* p, const char* path) const
> > >     {
> > >         uint32_t sz = alignedSize(sizeof(macho_rpath_command<P>) + strlen(path) + 1);
> > >         macho_rpath_command<P>* cmd = (macho_rpath_command<P>*)p;
> > >         cmd->set_cmd(LC_RPATH);
> > >         cmd->set_cmdsize(sz);
> > >         cmd->set_path_offset();
> > >         strcpy((char*)&p[sizeof(macho_rpath_command<P>)], path);
> > >         return p + sz;
> > >      }  
> > >      
> > > 
> > >     
> > > 
> > > 
> > 4 - for using `trim(0)`, I was looking at `  RPath == StringRef(reinterpret_cast<char *>(LC.Payload.data()),LC.Payload.size().trim(0))`  at line 264 under `Config.RPathToAdd`. Is that not how I should be doing it? Thanks
> @alexshap - ya, I can push a fix for it separately.
pushed the fix at https://reviews.llvm.org/D81575


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81527