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

[Sameer Arora via Phabricator via llvm-commits]

sameerarora101 marked 4 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:
> > 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?)


================
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]}}
+
----------------
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😊


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81527