[PATCH] D82812: [llvm-install-name-tool] Merge rpath with id/change

[Sameer Arora via Phabricator via llvm-commits]

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


================
Comment at: llvm/tools/llvm-objcopy/CopyConfig.cpp:886
           "cannot specify both -rpath %s %s and -rpath %s %s",
-          It1->first.str().c_str(), It1->second.str().c_str(),
-          Old.str().c_str(), New.str().c_str());
+          It1->getFirst().data(), It1->getSecond().data(), Old.data(),
+          New.data());
----------------
alexshap wrote:
> smeenai wrote:
> > The pointer returned by calling `.data()` on a StringRef [might not be null-terminated](https://llvm.org/doxygen/classllvm_1_1StringRef.html#a8ca8dc10ba312fe796d01b0f25b315f8), so I don't think this is safe.
> > 
> > There's a form of createStringError that takes a [twine](https://llvm.org/docs/ProgrammersManual.html#llvm-adt-twine-h) instead of a format string, which is more efficient than the intermediate allocations associated with `.str().c_str()`. Given that this is an error path, efficiency really doesn't matter though :) The twine form should get implicitly used if you do something like
> > 
> > ```
> > "cannot specify both -rpath " + It1->getFirst() + " " + It1->getSecond() + " and -rpath " + Old + " " + New
> > ```
> > 
> > You could also get fancy and use the `*` precision specifier to specify the string length manually, instead of constructing the intermediate `std::string`. See https://en.cppreference.com/w/cpp/io/c/fprintf for details about `*`. Again though, this is purely academic; efficiency doesn't really matter that much for error paths. In other words, just doing `.str().c_str()` as before should be fine.
> if I am not mistaken in this particular case they are null-terminated (see e.g. https://reviews.llvm.org/D81575),
> but perhaps it would be better not to rely on this.
I see, thanks! I have switched it to the twine format as suggested.


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp:65-66
   // specified in -delete_rpath.
   for (StringRef RPath : Config.RPathsToRemove) {
     if (RPathsToRemove.count(RPath))
       return createStringError(errc::invalid_argument,
----------------
smeenai wrote:
> Not related to this diff, but something I noticed: given that `RPathsToRemove` starts out as a copy of `Config.RPathsToRemove` and you're just removing elements from it, can't you just error out for any remaining entries in `RPathsToRemove` instead?
So the idea here was to raise an error for the **first** specified RPath that doesn't exist. For that purpose, we iterate through `Config.RPathsToRemove` and raise an error for the **first** RPath not deleted. 


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp:217
+
+  // Add new RPaths.
+  for (StringRef RPath : Config.RPathToAdd) {
----------------
alexshap wrote:
> smeenai wrote:
> > Nit: do we want to be adding new load commands in a function called `updateLoadCommands`? At least to me that function name seems like it should only be updating existing load commands, since we have a separate `removeLoadCommands` to handle removal. I'll leave it to the more experienced llvm-objcopy reviewers (@alexshap, @jhenderson) to decide if this is okay as-is or if we want a separate `addLoadCommands` function.
> so basically the idea was to group together logical pieces of handleArgs (to some reasonable extent).
> Besides error-reporting removeLoadCommands is ~10-12 lines of code, so I'd probably inline it into
> updateLoadCommands for consistency.
@alexshap Instead of inlining the whole `removeLoadCommands` inside `updateLoadCommands` I think it would cleaner if I just call

```
  // Remove LCs.
  if (Error E = removeLoadCommands(Config, Obj))
    return E;
```
from inside `updateLoadCommands`. This can allow for independent development of `removeLoadCommands` in future as well. What do you think? (I have updated the current diff with this change, however, I can update it again in case we want something else)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82812