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

[Sameer Arora via Phabricator via llvm-commits]

sameerarora101 marked 16 inline comments as done.
sameerarora101 added inline comments.


================
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/.
+
----------------
jhenderson wrote:
> You probably want to show the behaviour when different values are used too.
With different values it works fine - added the test now, thanks!


================
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
----------------
jhenderson wrote:
> 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.
yup, it should be "an", thanks 😄(added the test too)


================
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) {
----------------
jhenderson wrote:
> 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.
oh thanks, `llvm::find_if` helps. But, I don't want `is_contained` because it just returns true or false. I also need access to the element itself in case `true` is returned (error msg use the element). Therefore, I am explicitly checking whether `It != RPaths.end()` and using the iterator to access the element if the above condition is true. 

If  I use `is_contained`, I would have to again iterate through the vector to find the element, right?


================
Comment at: llvm/tools/llvm-objcopy/CopyConfig.cpp:867
+
+    auto IsValidWith = [&OldRPath, &NewRPath ]<class Iterator>(
+                           Iterator First, Iterator Last, const char *ErrorMsg)
----------------
jhenderson wrote:
> 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.
ok, i got rid of the lambda. No warnings are thrown (the code is little cleaner too perhaps). 

For the pattern,
```
if (is_contained(Config.RPathsToRemove), OldRPath)
  return creatStringError(...);
```
the issue is again that I need access to the element (if `is_contained` is true) to construct the error msg which means iterating the vector again. Instead I am following this pattern:

```
It = find_if(Config.RPathsToRemove, StringChecker)
if (It != Config.RPathsToRemove.end())
  return createStringError(..<use It here>..);

It = find_if(Config.RPathsToAdd, StringChecker)
if (It != Config.RPathsToAdd.end())
  return createStringError(..<use It here>..);
```

Is this fine?


================
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());
+}
----------------
jhenderson wrote:
> These two lines can be folded together I believe using `std::back_insertor` in the `std::copy` line, right?
So I tried folding the 2 lines into `std::copy(NewName.begin(), NewName.end(), std::back_insertor(LC->Payload));` but it fails the rpath test giving the following error: `truncated or malformed object (load command 1 extends past end of file)`.

Surprisingly, even `LC->Payload.assign(NewName.begin(), NewName.end());` didn't work (giving the same error as above).

However, the initial approach seems to work lol, not sure what's the difference in behavior from these new approaches?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82051