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

[Alexander Shaposhnikov via Phabricator via llvm-commits]

alexshap added inline comments.


================
Comment at: llvm/tools/llvm-objcopy/CopyConfig.cpp:839
+    // Cannot add & delete the same rpath at the same time.
+    if (llvm::is_contained(Config.RPathToAdd, RPath))
+      return createStringError(
----------------
since we are already in the namespace llvm you can simply use is_contained


================
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) {
----------------
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;
     }  
     

    




================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp:52
+  // -delete_rpath it is an error.
+  for (StringRef RPath : Config.RPathsToRemove) {
+    if (RPathsToRemove.count(RPath))
----------------
jhenderson wrote:
> In every other remove operation within objcopy, we don't emit an error if a switch had no effect because the named section/symbol etc wasn't present. Do we really need this error message? Getting rid of it would allow us to further simplify logic above.
I think the intention here is to catch the case when e.g. there is a typo in the paths specified by the command-line arguments and report an error rather than silently leave the binary untouched. I can see pros and cons with the both approaches, if I am not mistaken Xcode's install-name-tool returns an error (similarly to what's done here).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81527