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

Wed Jun 10 07:04:24 PDT 2020

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

================
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());
----------------
jhenderson wrote:
> 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).
So currently, I have modeled the behavior according to Apples' `install_name_tool` and it throws this error in case the user specifies a `delete_rpath` entry twice. I can change it though if this is not we want?

================
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))
----------------
alexshap wrote:
> jhenderson wrote:
> > 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.
> I think the intention here is to catch the case when e.g. there is a typo in the paths specified by the command-line arguments and report an error rather than silently leave the binary untouched. I can see pros and cons with the both approaches, if I am not mistaken Xcode's install-name-tool returns an error (similarly to what's done here).
yup, Apple's `install_name_tool` throws this error in case it can't find the rpath. It says in the specification too : 
```
-delete_rpath old
	      ... If the Mach-O binary does not contains the old rpath path name specified in -delete_rpath it is an error.
``` 

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81527