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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/tools/llvm-objcopy/CopyConfig.cpp:853-855
+    auto StringChecker = [&](const StringRef &Str) {
+      return Str == OldRPath || Str == NewRPath;
+    };
----------------
Move this down to where it's used for the first time.


================
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) {
----------------
sameerarora101 wrote:
> 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?
Yeah, that's right. I didn't realise you wanted to use the element, so `find_if` looks good.


================
Comment at: llvm/tools/llvm-objcopy/CopyConfig.cpp:867
+
+    auto IsValidWith = [&OldRPath, &NewRPath ]<class Iterator>(
+                           Iterator First, Iterator Last, const char *ErrorMsg)
----------------
sameerarora101 wrote:
> 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?
Thanks. I find the new code much easier to follow.


================
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());
+}
----------------
sameerarora101 wrote:
> 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?
That error message doesn't really give me enough info to understand what's going on. Did you perhaps write one too many bytes somewhere/not write the null byte? Regarding the `copy`, you'd also need to `clear` beforehand, to avoid it just appending the data to the end, so actually `assign` is probably want you want anyway, but I believe you may need to push a null byte to the end too, i.e.
```
LC->Payload.assign(NewName.begin(), NewName.end());
LC->Payload.push_back('\0');
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82051