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

[Shoaib Meenai via Phabricator via llvm-commits]

smeenai 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());
----------------
sameerarora101 wrote:
> 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?
Yeah. There's some tension here between following standard objcopy conventions vs. emulating the behavior of Apple's install_name_tool (which this is an LLVM version of). In general, we thought it was best to match install_name_tool's behavior as much as possible, although I don't feel too strongly about this particular error case.


================
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))
----------------
sameerarora101 wrote:
> 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.
> ``` 
> 
I do feel strongly about this case, however :) I feel like not erroring in this case would be a pretty major divergence from Apple install_name_tool's behavior. Our goal is for llvm-install-name-tool to be a drop-in replacement for Apple's install_name_tool, and having similar error conditions is an important part of that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81527