[PATCH] D81527: [llvm-install-name-tool] Add `delete_rpath` option

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-objcopy/MachO/install-name-tool-delete-rpath.test:1
+## This test checks deleting a LC_RPATH load command from a MachO binary.
+
----------------
Note to self: I still need to review the tests.


================
Comment at: llvm/tools/llvm-objcopy/CopyConfig.cpp:833-836
+    if (llvm::is_contained(Config.RPathsToRemove, RPath))
+      return createStringError(errc::invalid_argument,
+                               "-delete_rpath %s specified more than once",
+                               RPath.str().c_str());
----------------
This seems like an unnecessary limitation to me. I'd just expect it to ignore the later ones (you could use a set rather than a vector to store the set to remove in to ensure that).


================
Comment at: llvm/tools/llvm-objcopy/InstallNameToolOpts.td:22
+def delete_rpath: Option<["-", "--"], "delete_rpath", KIND_SEPARATE>,
+                  HelpText<"Delete old rpath">;
+
----------------
I'd get rid of "old" from the text. We don't say "old" for e.g. --remove-section in objcopy.


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp:26-45
+  LoadCommandPred RemovePred = [](const LoadCommand &) { return false; };
+  DenseSet<StringRef> RPathsToRemove;
+
+  if (!Config.RPathsToRemove.empty()) {
+    RPathsToRemove.insert(Config.RPathsToRemove.begin(),
+                          Config.RPathsToRemove.end());
+    RemovePred = [&RPathsToRemove](const LoadCommand &LC) {
----------------
This whole block seems unnecessarily overcomplicated to me.

1) Make Config.RPathsToRemove a set and then you don't have to copy things from one set to another - you can just use the config variable directly.
2) There's no need for the `if (!Config.RPathsToRemove.empty())` check, since it is safe to do everything here whether it's empty or not (the insert disappears, but would be safe, and defining the lambda is harmless and trivial.
3) By getting rid of the "if empty" check, you can also get rid of the `return false` lambda.
4) `trim(0)` - I assume you mean `trim('\0')`? If so, use the char directly rather than an integer literal, since you are dealing with an array of chars. This makes it easier to follow. I am assuming here that `LC.Payload.size()` includes a trailing null byte you want to get rid of. Do you also mean to get rid of any leading null bytes too? If not, you might be better off using `rtrim`.


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp:50-51
+
+  // If the Mach-O binary does not contains the old rpath path name specified in
+  // -delete_rpath it is an error.
+  for (StringRef RPath : Config.RPathsToRemove) {
----------------
I'd rephrase this comment slightly:

"Emit an error if the Mach-O binary does not contain an rpath path name specified in -delete_rpath."


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp:52
+  // -delete_rpath it is an error.
+  for (StringRef RPath : Config.RPathsToRemove) {
+    if (RPathsToRemove.count(RPath))
----------------
In every other remove operation within objcopy, we don't emit an error if a switch had no effect because the named section/symbol etc wasn't present. Do we really need this error message? Getting rid of it would allow us to further simplify logic above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81527