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

[Alexander Shaposhnikov via Phabricator via llvm-commits]

alexshap 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());
----------------
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.


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp:217
+
+  // Add new RPaths.
+  for (StringRef RPath : Config.RPathToAdd) {
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82812