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

[Sameer Arora via Phabricator via llvm-commits]

sameerarora101 marked 6 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:
> sameerarora101 wrote:
> > sameerarora101 wrote:
> > > jhenderson wrote:
> > > > sameerarora101 wrote:
> > > > > 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)
> > > > `llvm-objcopy` (unlike `llvm-strip`) doesn't have a `-o` option either. It uses the following syntax:
> > > > 
> > > > ```
> > > > llvm-objcopy <input> [output]
> > > > ```
> > > > 
> > > > In other words, the second positional argument is the output file the input file will be copied to. If not specified, the input will be modified in place.
> > > > 
> > > > According to the help text, `llvm-install-name-tool` uses the same syntax, although I don't know if that's the case for Apple's version. Either way, assuming the help text is accurate, I think we should follow the same approach.
> > > Oh, it seems like the help text is wrong in case of  `llvm-install-name-tool`. If I run something like `./bin/llvm-install-name-tool -add_rpath @ZZ Dependent Dep2`, it throws the following error: `error: llvm-install-name-tool expects a single input file`. 
> > > 
> > > the code too sets `Config.InputFilename` and `Config.OutputFilename` to the same file:
> > > ```
> > >   if (Positional.size() > 1)
> > >     return createStringError(
> > >         errc::invalid_argument,
> > >         "llvm-install-name-tool expects a single input file");
> > >   Config.InputFilename = Positional[0];
> > >   Config.OutputFilename = Positional[0];
> > > ```
> > > Nevertheless, the current llvm's version is compatible with Apple's version as it too throws an error when an output file is specified. I can create a patch for the help text in a separate diff? (or, if preferable, make `install-name-tool` accept a output file?)
> > currently resolved by updating the help text here https://reviews.llvm.org/D81907. (I can change it to accept a output file if people prefer that way?) Please lemme know.
> Thanks for the fix. It's up to you if you want to add an option in a separate patch for emitting an output file (it seems like a logical improvement), but I wouldn't make it the same as llvm-objcopy's positional argument, but rather I'd follow llvm-strip's separate option style. I don't think you need to do that for this change to land though.
ok, got it!


================
Comment at: llvm/tools/llvm-objcopy/MachO/Object.cpp:42
+  return Error::success();
+}
+
----------------
alexshap wrote:
> alexshap wrote:
> > Object stores indices of some special load commands which need to be updated (in this method).
> > 
> >    for (const LoadCommand &LC : LoadCommand) {
> >         switch (LC.cmd) {
> >            ...
> >         }
> >    }
> > 
> i see, thanks, almost there. A few suggestions from me:
> 
> 1. I think the quadratic behavior here is unnecessary, I would simply use smth like
>       
>       for (size_t Index = 0, Size = LoadCommands.size(); Index < Size; ++Index) {
>            ...
>       }
> 
> 2. Let's factor out it into a helper method, e.g. updateLoadCommandsIndexes
got it, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81527