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

[James Henderson via Phabricator via llvm-commits]

jhenderson 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 | \
----------------
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.


================
Comment at: llvm/test/tools/llvm-objcopy/MachO/install-name-tool-delete-rpath.test:8
+# RUN: llvm-objdump -p %t | \
+# RUN:   FileCheck %s --check-prefix=RPATHS --implicit-check-not={{path[[:space:]]@executable_[a]}}
+
----------------
sameerarora101 wrote:
> jhenderson wrote:
> > You could probably make this `--implicit-check-not` even simpler, for example simply `@executable` or `path` (but double-check the whole output for the second one, since it's possible it includes the file path somewhere else, and it's reasonable that somebody's file path will include "path" in a directory name somewhere).
> > 
> > Same goes below for the other cases.
> > 
> > Couple of other notes, but won't need dealing with, if you change as I request:
> > # `[a]` is equivalent to simply `a` in the regex.
> > # `[a]` is actually unnecessary entirely, since even without it, the check will still work - it will show that `path @executable_` doesn't appear in the output anywhere other than the lines checked by `RPATHS`. Similarly `[abc]` and `[abcd]` below are unnecessary.
> nice, thanks😊
You can also get rid of the `{{` and `}}` from the pattern here and below, since the string is no longer a regex.


================
Comment at: llvm/test/tools/llvm-objcopy/MachO/install-name-tool-delete-rpath.test:45
+
+## Check that removing load commands updates index for special LCs
+# RUN: yaml2obj %s -o %t
----------------
Nit: missing full stop.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81527