[PATCH] D82051: [llvm-install-name-tool] Add -rpath option

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-objcopy/MachO/install-name-tool-rpath.test:34
+
+# RUN: llvm-objcopy %t %t1
+
----------------
Use `cp` rather than `llvm-objcopy` to copy the file.


================
Comment at: llvm/test/tools/llvm-objcopy/MachO/install-name-tool-rpath.test:38-39
+# RUN: not llvm-install-name-tool -rpath @executable_test/. @executable/. \
+# RUN:                            -rpath @executable_long_test/. @executable_long_longest/. %t 2>&1 | \
+# RUN:  FileCheck %s --check-prefix=RPATHS-FAIL
+
----------------
Nit: indent the FileCheck line by an additional space, matching e.g. lines 8 and 19.


================
Comment at: llvm/test/tools/llvm-objcopy/MachO/install-name-tool-rpath.test:67
+
+# COMBINED-DELETE: cannot specify both -delete_rpath @executable_d/. and -rpath @executable_d/. DD/.
+
----------------
You probably want to show the behaviour when different values are used too.


================
Comment at: llvm/test/tools/llvm-objcopy/MachO/install-name-tool-rpath.test:74
+
+# COMBINED-ADD: cannot specify both -add_rpath @executable_e/. and -rpath @executable_e/. EE/.
+
----------------
Ditto.


================
Comment at: llvm/test/tools/llvm-objcopy/MachO/install-name-tool-rpath.test:76-80
+## Missing a RPath argument:
+# RUN: not llvm-install-name-tool %t -rpath @executable_e/. 2>&1 | \
+# RUN:   FileCheck %s --check-prefix=MISSING
+
+# MISSING: missing argument to -rpath option
----------------
How do you say "RPath" - "ah-path" or something else? If "ah-path", (which is how I read it), the "a" before should be "an".

Also might be worth having another case here where `-rpath` has no arguments at all.


================
Comment at: llvm/test/tools/llvm-objcopy/MachO/install-name-tool-rpath.test:83
+## Check that binary is not modified after errors:
+# RUN: cmp %t %t1
+
----------------
It probably is best to do this after each individual error, rather than once at the end (just to make sure that one doesn't change the binary and the next change it back).


================
Comment at: llvm/tools/llvm-objcopy/CopyConfig.cpp:853
+
+    // Cannot specify duplicate -rpath entries
+    auto It1 = std::find_if(
----------------
sameerarora101 wrote:
> sameerarora101 wrote:
> > Unlike `-delete_rpath`, we thought it was necessary to check duplication in case of `-rpath`. This is because it would not be useful to do something like `-rpath A B .... -rpath A C` or something like `-rpath A B ... -rpath B C`. Additionally,  this also matches XCode's -rpath behavior.
> I can remove this check if people feel the need?
No, it makes reasonable sense to me. I guess if people ware used to XCode's behaviour, it's okay to follow it.


================
Comment at: llvm/tools/llvm-objcopy/CopyConfig.cpp:854-855
+    // Cannot specify duplicate -rpath entries
+    auto It1 = std::find_if(
+        adl_begin(Config.RPathsToUpdate), adl_end(Config.RPathsToUpdate),
+        [&](const std::pair<StringRef, StringRef> &RPair) {
----------------
There's an `llvm::find_if` which just takes the container. There's an `is_contained` method too which does `std::find` + the end check, which might be worth extending to allow it to take a lambda (something like `is_contained_if`). You might want to do that (in a prerequisite patch), to simplify a lot of this code.


================
Comment at: llvm/tools/llvm-objcopy/CopyConfig.cpp:860
+        });
+    if (It1 != adl_end(Config.RPathsToUpdate))
+      return createStringError(
----------------
`adl_end(Config.RPathsToUpdate)` -> `Config.RPathsToUpdate.end()` is more common syntax in LLVM.


================
Comment at: llvm/tools/llvm-objcopy/CopyConfig.cpp:867
+
+    auto IsValidWith = [&OldRPath, &NewRPath ]<class Iterator>(
+                           Iterator First, Iterator Last, const char *ErrorMsg)
----------------
sameerarora101 wrote:
> This highlights a warning :
> `warning: lambda templates are only available with -std=c++2a or -std=gnu++2a [-Wpedantic]`
> 
> Is this fine? Not sure what I should do about it?
LLVM is built with C++14 so this will fail the build on most build bots.

I'm actually struggling to follow the logic of this whole section, if I'm honest! If I follow it right, I think you could simplify it to something like below:

```
  if (is_contained(Config.RPathsToRemove), OldRPath)
    return creatStringError(...);
  if (is_contained(Config.RPathsToRemove), NewRPath)
    return creatStringError(...);
  if (is_contained(Config.RPathsToAdd), OldRPath)
    return creatStringError(...);
  if (is_contained(Config.RPathsToAdd), NewRPath)
    return creatStringError(...);
```

Alternatively, with an `is_contained_if` you could fold it down to just two cases, one for each of the two existing Config variables.


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp:124-125
+  LC->MachOLoadCommand.load_command_data.cmdsize = NewCmdsize;
+  LC->Payload.assign(NewCmdsize - sizeof(T), 0);
+  std::copy(NewName.begin(), NewName.end(), LC->Payload.begin());
+}
----------------
These two lines can be folded together I believe using `std::back_insertor` in the `std::copy` line, right?


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp:273
+
+    LoadCommand *ToBeModifiedLC = NULL;
+    for (LoadCommand &LC : Obj.LoadCommands) {
----------------
`NULL` -> `nullptr`

Same below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82051