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

[Sameer Arora via Phabricator via llvm-commits]

sameerarora101 marked 5 inline comments 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) {
----------------
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


================
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:
> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81527