[install-name-tool] Add --delete_all_rpaths to llvm-install-name-tool

[thieta]

Summary

We had a need of deleting all the rpaths in binaries we ship (we later add just one) and instead of parsing the output of objdump -p and calling install-name-tool over and over this seemed like a easy solution to the problem. I know this tool was created to mirror upstream apple - but don't see any problem of it adding additional functionality IMHO.

This patch misses some documentation but I wanted to make sure that people thought that this was an alright thing to add.

Tagged a few reviewers that have worked with install-name-tool before.

[rupprecht]

Can you also update install-name-tool-delete-rpath.test with a test case for this?

This seems fine to me. In general for our binutils-like tools, we allow extra flags for useful behavior as long as they don't have short prefixes (that may be someday used by GNU binutils, and we may want to match), which you have avoided here.

llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp

145

tiny tiny nit: this extra blank line seems unnecessary.

[alexander-shaposhnikov]

LG

llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp

140

nit: for a single-line if we don't use braces (just for consistency with the other places)

[alexander-shaposhnikov]

(and yeah, we need a test for this option)

[MaskRay]

Since this is specific to the tool llvm-install-name-tool. You can change the subject to [llvm-install-name-tool] Add --delete_all_rpaths to make readers clear.

[MaskRay]

llvm/tools/llvm-objcopy/CopyConfig.h

233

--delete_all_rpaths

llvm/tools/llvm-objcopy/InstallNameToolOpts.td

24

Does this need "-" ?

[thieta]

Fixed all review feedback and added tests.

[thieta]

Marked comments as done.

[thieta]

The test fails doesn't seem related. But let me know if anyone thinks otherwise.

[alexander-shaposhnikov]

llvm/test/tools/llvm-objcopy/MachO/install-name-tool-delete-rpath.test

48 ↗

(On Diff #295976)

the line break appears to be unnecessary

llvm/tools/llvm-objcopy/InstallNameToolOpts.td

24

it would be good to have at least one test which checks -delete_all_rpaths (with single dash)

llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp

136

you can apply clang-format -style=llvm to reformat the code (here and above)

[thieta]

• made sure one call is using -- and one is using -
• clang-format'ed
• line breaks fix

If this is deemed done after this - can someone please commit it for me since I don't have commit access.

[thieta]

maked comments as done

[alexander-shaposhnikov]

Thanks, looks good, I'd wait for ~1 day to see if other reviewers have any comments, if everything is fine I can commit it for you

[jhenderson]

+1 to being okay to extend the tools with other useful options. Before we rush headlong into this option, I wonder whether we should instead allow the existing delete option to take a wildcard/glob pattern/regex thing. That way you could just delete e.g. "*" to remove all rpaths, or "*.foo" to delete all ending in ".foo" etc. From a compatibility concern, it might mean we need to add an option similar to e.g. --regex in llvm-objcopy which enables the pattern matching.

What do others think?

If people think this is being overly general, the proposed option in this patch is fine by me too.

[thieta]

I don't have a strong opinion on what's best - both will solve my problem, if people think it would be better to use a regex I can rework the patch to do that (as long as there is a good regex implementation in LLVM I can use already).

[alexander-shaposhnikov]

I don't have a strong opinion regarding this either, usually there are not so many rpaths.
To me the current version looks reasonable, if you'd like I think we can proceed, but it's up to you.

[rupprecht]

In D88674#2313985, @jhenderson wrote:

+1 to being okay to extend the tools with other useful options. Before we rush headlong into this option, I wonder whether we should instead allow the existing delete option to take a wildcard/glob pattern/regex thing. That way you could just delete e.g. "*" to remove all rpaths, or "*.foo" to delete all ending in ".foo" etc. From a compatibility concern, it might mean we need to add an option similar to e.g. --regex in llvm-objcopy which enables the pattern matching.

What do others think?

If people think this is being overly general, the proposed option in this patch is fine by me too.

Same here -- that seems like it would be a better patch.

In D88674#2314150, @thieta wrote:

I don't have a strong opinion on what's best - both will solve my problem, if people think it would be better to use a regex I can rework the patch to do that (as long as there is a good regex implementation in LLVM I can use already).

There is -- see NameOrPattern and NameMatcher in CopyConfig.h. You would also have to add --regex and/or --wildcard flags, which AIUI are not part of install_name_tool. So that would also be a flag incompatibility, but maybe a better one.

There is some validation that all RPathsToRemove are used, so you will have to handle that differently -- it might be easier to just drop that if wildcards/regexes are used.

[alexander-shaposhnikov]

@rupprecht - in general, I'm not against using regex, however, as I mentioned above, it seems like it's not a very frequent use case + there are not so many rpaths (this is quite different from matching symbol names or section names).
Adding more options (--regex) + the required plumbing + tests coverage has its price, honestly I'm not convinced that it's strictly necessary in this particular case (when e.g. a binary has 2 or 3 rpaths).

[alexander-shaposhnikov]

just want to add a few more words to explain what I meant: on the one hand this would provide more flexibility on the other hand this would introduce one more option + increase the code size.
If this flexibility is necessary (which is not clear) - yeah, we can add it.

[thieta]

Hey - I am keen on getting this done. Can we come to a consensus on what the best way is to handle this?

[jhenderson]

I'm okay to stick with this approach. I don't think it hurts to have such an option, even if we do decide in the future to add a wildcard/regex option. @rupprecht, are you happy?

llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp

139

Nit: comments should end with a full-stop. You've probably also prematurely wrapped this comment.

[thieta]

Can this be merged? @rupprecht ?

[rupprecht]

Sorry for the delay -- yep, going this route (for now, at least) is fine with me too.

Can you add this flag to llvm/docs/CommandGuide/llvm-install-name-tool.rst?

[thieta]

Added documentation in the rst file. Anyone with commit access - feel free to commit this for me.

[alexander-shaposhnikov]

@thieta - thanks for the patch and for your patience. According to https://llvm.org/docs/DeveloperPolicy.html#commit-messages we would need your name + e-mail (to correctly specify the author of the changes).

[mstorsjo]

In D88674#2326835, @thieta wrote:

Added documentation in the rst file. Anyone with commit access - feel free to commit this for me.

Also fwiw, I think you should ask for commit access.

[thieta]

In D88674#2326869, @alexshap wrote:

@thieta - thanks for the patch and for your patience. According to https://llvm.org/docs/DeveloperPolicy.html#commit-messages we would need your name + e-mail (to correctly specify the author of the changes).

Tobias Hieta <[e-mail]>

thanks!

[thieta]

In D88674#2326878, @mstorsjo wrote:

In D88674#2326835, @thieta wrote:

Added documentation in the rst file. Anyone with commit access - feel free to commit this for me.

Also fwiw, I think you should ask for commit access.

Yeah I should take care of that. Will send that email now.

[jhenderson]

Normally, I'd be happy to land patches for people, but I've got too much else on my plate currently, so if somebody else could, that would be great!

P.S. @thieta, if you get commit access before somebody gets that far, don't forget to include the line Differential Revision: https://reviews.llvm.org/D88674 in the commit message (with nothing else on that line) so that this patch auto-closes afterwards.

llvm/tools/llvm-objcopy/CopyConfig.h

struct CopyConfig {
   bool StripDWO = false;
   bool StripDebug = false;
   bool StripNonAlloc = false;
   bool StripSections = false;
   bool StripSwiftSymbols = false;
   bool StripUnneeded = false;
   bool Weaken = false;
   bool DecompressDebugSections = false;
+  // install-name-tool's delete_all_rpaths
+  bool RemoveAllRpaths = false;
+
   DebugCompressionType CompressionType = DebugCompressionType::None;
 
   // parseELFConfig performs ELF-specific command-line parsing. Fills `ELF` on
   // success or returns an Error otherwise.
   Error parseELFConfig() {
     if (!ELF) {
       Expected<elf::ELFCopyConfig> ELFConfig = elf::parseConfig(*this);
       if (!ELFConfig)

llvm/tools/llvm-objcopy/CopyConfig.cpp

parseInstallNameToolOptions(ArrayRef<const char *> ArgsArr) {
 
   if (auto *Arg = InputArgs.getLastArg(INSTALL_NAME_TOOL_id))
     Config.SharedLibId = Arg->getValue();
 
   for (auto *Arg : InputArgs.filtered(INSTALL_NAME_TOOL_change)) {
     Config.InstallNamesToUpdate.insert({Arg->getValue(0), Arg->getValue(1)});
   }
 
+  Config.RemoveAllRpaths = InputArgs.hasArg(INSTALL_NAME_TOOL_delete_all_rpaths);
+
   SmallVector<StringRef, 2> Positional;
   for (auto Arg : InputArgs.filtered(INSTALL_NAME_TOOL_UNKNOWN))
     return createStringError(errc::invalid_argument, "unknown argument '%s'",
                              Arg->getAsString(InputArgs).c_str());
   for (auto Arg : InputArgs.filtered(INSTALL_NAME_TOOL_INPUT))
     Positional.push_back(Arg->getValue());
   if (Positional.empty())
     return createStringError(errc::invalid_argument, "no input file specified");

llvm/tools/llvm-objcopy/InstallNameToolOpts.td

 def h : Flag<["-"], "h">, Alias<help>;
 
 def add_rpath : Option<["-", "--"], "add_rpath", KIND_SEPARATE>,
                 HelpText<"Add new rpath">;
 
 def delete_rpath: Option<["-", "--"], "delete_rpath", KIND_SEPARATE>,
                   HelpText<"Delete specified rpath">;
 
+def delete_all_rpaths: Flag<["--"], "delete_all_rpaths">,
+              HelpText<"Delete all rpath directives">;
+
 def rpath: MultiArg<["-", "--"], "rpath", 2>,
            HelpText<"Change rpath path name">;
 
 def id : Option<["-","--"], "id", KIND_SEPARATE>,
                  HelpText<"Change dynamic shared library id">;
 
 def change: MultiArg<["-", "--"], "change", 2>,
             HelpText<"Change dependent shared library install name">;
 
 def version : Flag<["--"], "version">,
               HelpText<"Print the version and exit.">;

llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp

static LoadCommand buildRPathLoadCommand(StringRef Path) {
   return LC;
 }
 
 static Error processLoadCommands(const CopyConfig &Config, Object &Obj) {
   // Remove RPaths.
   DenseSet<StringRef> RPathsToRemove(Config.RPathsToRemove.begin(),
                                      Config.RPathsToRemove.end());
 
-  LoadCommandPred RemovePred = [&RPathsToRemove](const LoadCommand &LC) {
+  LoadCommandPred RemovePred = [&RPathsToRemove, &Config](const LoadCommand &LC) {
     if (LC.MachOLoadCommand.load_command_data.cmd == MachO::LC_RPATH) {
+      // When removing all RPaths we don't need to care
+      // about what it contains
+      if (Config.RemoveAllRpaths) {
+        return true;
+      }
+
       StringRef RPath = getPayloadString(LC);
+
       if (RPathsToRemove.count(RPath)) {
         RPathsToRemove.erase(RPath);
         return true;
       }
     }
     return false;
   };