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

[Sameer Arora via Phabricator via llvm-commits]

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


================
Comment at: llvm/test/tools/llvm-objcopy/MachO/install-name-tool-delete-rpath.test:13
+## Deleting single RPath entry:
+# RUN: llvm-install-name-tool -delete_rpath @executable_a/. %t
+# RUN: llvm-objdump -p %t | \
----------------
jhenderson wrote:
> It's generally more traditional for llvm-objcopy tests to use a separate output file rather than modifying in place. This means you don't have to repeatedly recreate the same input in later tests. In some cases, we even demonstrate that the input file hasn't been modified, though I'm not convinced by the necessity of that here.
> 
> Same applies for all test cases in this test.
Unlike `llvm-objcopy`, it seems like `install-name-tool` doesn't have an option (like `-o`) where the user can specify the output binary. It only expects an input file and thus can only change the binary in place. (It also doesn't print the result on the terminal when it succeeds so `>` operator can't be used)


================
Comment at: llvm/tools/llvm-objcopy/CopyConfig.cpp:833-836
+    if (llvm::is_contained(Config.RPathsToRemove, RPath))
+      return createStringError(errc::invalid_argument,
+                               "-delete_rpath %s specified more than once",
+                               RPath.str().c_str());
----------------
jhenderson wrote:
> smeenai wrote:
> > sameerarora101 wrote:
> > > jhenderson wrote:
> > > > This seems like an unnecessary limitation to me. I'd just expect it to ignore the later ones (you could use a set rather than a vector to store the set to remove in to ensure that).
> > > So currently, I have modeled the behavior according to Apples' `install_name_tool` and it throws this error in case the user specifies a `delete_rpath` entry twice. I can change it though if this is not we want?
> > Yeah. There's some tension here between following standard objcopy conventions vs. emulating the behavior of Apple's install_name_tool (which this is an LLVM version of). In general, we thought it was best to match install_name_tool's behavior as much as possible, although I don't feel too strongly about this particular error case.
> Not being an Apple developer, I don't feel like I'm best placed to say what the behaviour should be. I'd prefer @alexshap or other Apple developers to give their thoughts. How important is it to emulate error message behaviour? Are the errors actually useful?
> 
> One of the issues with prohibiting something being specified more than once is that people will occasionally be using response files or a build system or whatever where they can't easily change the existing commands that are specified, but can add to the end (I've run into this on one or two occasions). By following the more traditional approach of "last one wins", it's possible to workaround this difficulty, whereas this error makes it impossible.
> 
> My 2 pence are that this one is not particularly useful, whereas the other one I commented on might have some value for reasons you've already outlined.
Ok, so currently I have removed this error check and allowed for duplicate entries to be specified (`Config.RPathsToRemove` is a set). I can change it back if @alexshap or someone else feels that we should support it?


================
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:
> sameerarora101 wrote:
> > 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
> I assume something got messed up when you copied in that line, since you obviously don't trim a size value!
> 
> I would suggest that the older code should be improved (not in this patch though). When I read this code originally, I thought it was trying to trim 0 bytes or something before I confirmed that there is no such interface.
ok thanks. I have currently fixed it for above code in the current patch. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81527