[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:
> 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.


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


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp:26-29
+  DenseSet<StringRef> RPathsToRemove;
+
+  RPathsToRemove.insert(Config.RPathsToRemove.begin(),
+                        Config.RPathsToRemove.end());
----------------
I believe you can construct the set upfront with the right elements:

```
DenseSet<StringRef> RPathsToRemove(Config.RPathsToRemove.begin(), Config.RPathsToRemove.end());
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81527