[llvm-objcopy] Add -prepend_rpath install-name-tool option

[keith]

Summary

This prepends a rpath as the first rpath in the binary. This is useful
when you want a rpath to take precedence over others without having to
delete all rpaths from a binary, or change each one in order.

Author: Keith Smiley <[e-mail]>

[kastiglione]

Name suggestion: -prepend_rpath or -insert_initial_rpath. By itself, "insert" feels like it should take a position to say where it insert.

[keith]

Rename to -prepend_rpath

[alexander-shaposhnikov]

LGTM, thanks!

[alexander-shaposhnikov]

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

242

tiny nit: would be good to add a comment that unlike the case where we add new load commands at the end of Obj.LoadCommands here the indexes have been invalidated and need to be updated.

[keith]

Add comment about recalculation

[alexander-shaposhnikov]

LG

[keith]

Thanks! I don't have merge access can you help me land this?

[alexander-shaposhnikov]

According to https://llvm.org/docs/DeveloperPolicy.html#commit-messages your name + e-mail are necessary (to correctly specify the author of the changes).
I'd wait for a day to see if there are any comments from other reviewers, if everything is okay I'll commit it.

[keith]

Update author

[keith]

I believe my author info was correct, but I've updated it using the command mentioned in the doc. Thanks!

[jhenderson]

Mostly looks fine, but lots of small issues, so requesting changes to make sure this doesn't get landed before they are addressed.

Please add the new option to the CommandGuide documentation.

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

14–16

Nit: I find it slightly easier to follow tests which use the -NEXT pattern, if the lines without -NEXT are indented slightly to line up.

18

22

Nit: comments should end with '.' (or ':' would work too)

27

31–32

Here and below, I have a personal preference for continuations with new commands to have the '|' on the previous line and then the next line to be slightly indented, as in the suggested edit. When reading the previous line, this shows that the next line is a new command and that there are no more arguments to the current line's command, whilst the indentation shows that the next line is still a continuation.

If for some reason you'd prefer not to do that, I'd recommend at least adding a couple of spaces before the '|' for readability (I just find it easier to follow the test that way).

36–39

This isn't a test case that's needed here - the behaviour for "no input file" is not specific to the new option, and is (or should be at least) tested elsewhere.

55

I'm not sure what is meant by "id" here? I think this option replaces it, IIRC?

67

llvm/tools/llvm-objcopy/CopyConfig.cpp

910–918

I'd delete this comment and the blank line before, so that the comment for RPathToAdd clearly applies to both blocks.

953

This comment isn't relevant to the code you've added. Please fix. It's also missing a trailing full stop.

llvm/tools/llvm-objcopy/InstallNameToolOpts.td

22

I think this more clearly expresses what is happening. "Prepend" to me implied it was changing the string in the existing rpath.

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

233–235

Please test this whole error message, rather than a small part of it. In particular, showing that the RPath is in the message.

242

[jhenderson]

In D89605#2340135, @keith wrote:

I believe my author info was correct, but I've updated it using the command mentioned in the doc. Thanks!

To my knowledge, we can't view your author details from the commit you have locally. You'll either need to post them in a comment or email them directly to someone (e.g. @alexshap).

[alexander-shaposhnikov]

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

36–39

@jhenderson is right, this is covered by https://github.com/llvm/llvm-project-staging/blob/staging/apple/llvm/test/tools/llvm-objcopy/tool-help-message.test (see NO-INPUT-FILES, MULTIPLE-INPUT-FILES, UNKNOWN-ARG).

[keith]

Address review comments

[keith]

Thanks for the review!

[jhenderson]

LGTM, with one suggestion, which could be deferred to a follow-up if you want.

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

34

Reading this message, I think we should quote the rpath value, to make it clearer. Same might apply to similar messages, but doesn't need addressing in this patch.

[keith]

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

34

I can address this in a follow up for all subcommands if that's ok

[keith]

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

34

Follow up is here! https://reviews.llvm.org/D90080

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

+## This test checks prepending a new LC_RPATH load command to a MachO binary.
+
+# RUN: yaml2obj %p/Inputs/i386.yaml -o %t.i386
+# RUN: llvm-install-name-tool -add_rpath @executable_path/. %t.i386
+# RUN: llvm-install-name-tool -prepend_rpath first_rpath %t.i386
+# RUN: llvm-objdump -p %t.i386 | FileCheck --check-prefix=NEW-RPATH %s
+
+# RUN: yaml2obj %p/Inputs/x86_64.yaml -o %t.x86_64
+# RUN: llvm-install-name-tool -add_rpath @executable_path/. %t.x86_64
+# RUN: llvm-install-name-tool -prepend_rpath first_rpath %t.x86_64
+# RUN: llvm-objdump -p %t.x86_64 | FileCheck --check-prefix=NEW-RPATH %s
+
+# NEW-RPATH: cmd LC_RPATH
+# NEW-RPATH-NEXT: cmdsize
+# NEW-RPATH-NEXT: first_rpath
+
+# NEW-RPATH: cmd LC_RPATH
+# NEW-RPATH-NEXT: cmdsize
+# NEW-RPATH-NEXT: @executable_path/.
+
+## Prepend with dylib loads
+# RUN: yaml2obj %p/Inputs/strip-all.yaml -o %t.dylib
+# RUN: llvm-install-name-tool -prepend_rpath first_rpath %t.dylib
+# RUN: llvm-objdump -p %t.dylib | FileCheck --check-prefix=DYLIB %s
+
+# DYLIB: cmd LC_RPATH
+# DYLIB-NEXT: cmdsize
+# DYLIB-NEXT: first_rpath
+
+# RUN: not llvm-install-name-tool -prepend_rpath first_rpath %t.i386 2>&1 \
+# RUN: | FileCheck --check-prefix=DUPLICATE-RPATH %s
+
+# DUPLICATE-RPATH: duplicate load command
+
+# RUN: not llvm-install-name-tool -prepend_rpath @executable_path/. 2>&1 \
+# RUN: | FileCheck --check-prefix=NO-INPUT %s
+
+# NO-INPUT: no input file specified
+
+## Prepend same RPATH twice:
+# RUN: not llvm-install-name-tool -prepend_rpath @executable_X \
+# RUN:                            -prepend_rpath @executable_X %t.i386 2>&1 \
+# RUN: | FileCheck --check-prefix=DOUBLE %s
+
+# DOUBLE: duplicate load command
+
+## Prepend and delete RPATH:
+# RUN: not llvm-install-name-tool -prepend_rpath foo \
+# RUN:                            -delete_rpath foo %t.i386 2>&1 \
+# RUN: | FileCheck --check-prefix=DELETE %s
+
+# DELETE: cannot specify both -prepend_rpath foo and -delete_rpath foo
+
+## Prepend and id RPATH:
+# RUN: not llvm-install-name-tool -prepend_rpath foo \
+# RUN:                            -rpath foo bar %t.i386 2>&1 \
+# RUN: | FileCheck --check-prefix=ID %s
+
+# ID: cannot specify both -prepend_rpath foo and -rpath foo bar
+
+## Check that cmdsize accounts for NULL terminator.
+# RUN: yaml2obj %p/Inputs/x86_64.yaml -o %t.x86_64
+# RUN: llvm-install-name-tool -prepend_rpath abcd %t.x86_64
+# RUN: llvm-objdump -p %t.x86_64 | FileCheck %s --check-prefix=RPATH-SIZE
+
+# RPATH-SIZE: cmd LC_RPATH
+# RPATH-SIZE-NEXT: cmdsize 24
+# RPATH-SIZE-NEXT: path abcd

llvm/tools/llvm-objcopy/CopyConfig.h

struct CopyConfig {
   DiscardType DiscardMode = DiscardType::None;
   Optional<StringRef> NewSymbolVisibility;
 
   // Repeated options
   std::vector<StringRef> AddSection;
   std::vector<StringRef> DumpSection;
   std::vector<StringRef> SymbolsToAdd;
   std::vector<StringRef> RPathToAdd;
+  std::vector<StringRef> RPathToPrepend;
   DenseMap<StringRef, StringRef> RPathsToUpdate;
   DenseMap<StringRef, StringRef> InstallNamesToUpdate;
   DenseSet<StringRef> RPathsToRemove;
 
   // install-name-tool's id option
   Optional<StringRef> SharedLibId;
 
   // Section matchers

llvm/tools/llvm-objcopy/CopyConfig.cpp

outs() << "llvm-install-name-tool, compatible with cctools "
               "install_name_tool\n";
     cl::PrintVersionMessage();
     exit(0);
   }
 
   for (auto Arg : InputArgs.filtered(INSTALL_NAME_TOOL_add_rpath))
     Config.RPathToAdd.push_back(Arg->getValue());
 
+  for (auto *Arg : InputArgs.filtered(INSTALL_NAME_TOOL_prepend_rpath))
+    Config.RPathToPrepend.push_back(Arg->getValue());
+
   for (auto Arg : InputArgs.filtered(INSTALL_NAME_TOOL_delete_rpath)) {
     StringRef RPath = Arg->getValue();
 
     // Cannot add and delete the same rpath at the same time.
     if (is_contained(Config.RPathToAdd, RPath))
       return createStringError(
           errc::invalid_argument,
           "cannot specify both -add_rpath %s and -delete_rpath %s",
           RPath.str().c_str(), RPath.str().c_str());
 
+    // Cannot add and delete the same rpath at the same time.
+    if (is_contained(Config.RPathToPrepend, RPath))
+      return createStringError(
+          errc::invalid_argument,
+          "cannot specify both -prepend_rpath %s and -delete_rpath %s",
+          RPath.str().c_str(), RPath.str().c_str());
+
     Config.RPathsToRemove.insert(RPath);
   }
 
   for (auto *Arg : InputArgs.filtered(INSTALL_NAME_TOOL_rpath)) {
     StringRef Old = Arg->getValue(0);
     StringRef New = Arg->getValue(1);
 
     auto Match = [=](StringRef RPath) { return RPath == Old || RPath == New; };
 
for (auto *Arg : InputArgs.filtered(INSTALL_NAME_TOOL_rpath)) {
 
     // Cannot specify the same rpath under both -add_rpath and -rpath
     auto It3 = find_if(Config.RPathToAdd, Match);
     if (It3 != Config.RPathToAdd.end())
       return createStringError(errc::invalid_argument,
                                "cannot specify both -add_rpath " + *It3 +
                                    " and -rpath " + Old + " " + New);
 
+    // Cannot specify the same rpath under both -add_rpath and -rpath
+    auto It4 = find_if(Config.RPathToPrepend, Match);
+    if (It4 != Config.RPathToPrepend.end())
+      return createStringError(errc::invalid_argument,
+                               "cannot specify both -prepend_rpath " + *It4 +
+                                   " and -rpath " + Old + " " + New);
+
     Config.RPathsToUpdate.insert({Old, New});
   }
 
   if (auto *Arg = InputArgs.getLastArg(INSTALL_NAME_TOOL_id)) {
     Config.SharedLibId = Arg->getValue();
     if (Config.SharedLibId->empty())
       return createStringError(errc::invalid_argument,
                                "cannot specify an empty id");

llvm/tools/llvm-objcopy/InstallNameToolOpts.td

 include "llvm/Option/OptParser.td"
 
 def help : Flag<["--"], "help">;
 def h : Flag<["-"], "h">, Alias<help>;
 
 def add_rpath : Option<["-", "--"], "add_rpath", KIND_SEPARATE>,
                 HelpText<"Add new rpath">;
 
+def prepend_rpath : Option<["-", "--"], "prepend_rpath", KIND_SEPARATE>,
+                    HelpText<"Prepend 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">;

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

for (StringRef RPath : Config.RPathToAdd) {
     if (RPaths.count(RPath) != 0)
       return createStringError(errc::invalid_argument,
                                "rpath " + RPath +
                                    " would create a duplicate load command");
     RPaths.insert(RPath);
     Obj.LoadCommands.push_back(buildRPathLoadCommand(RPath));
   }
 
+  for (StringRef RPath : Config.RPathToPrepend) {
+    if (RPaths.count(RPath) != 0)
+      return createStringError(errc::invalid_argument,
+                               "rpath " + RPath +
+                                   " would create a duplicate load command");
+
+    RPaths.insert(RPath);
+    Obj.LoadCommands.insert(Obj.LoadCommands.begin(),
+                            buildRPathLoadCommand(RPath));
+  }
+
+  if (!Config.RPathToPrepend.empty())
+    Obj.updateLoadCommandIndexes();
+
   return Error::success();
 }
 
 static Error dumpSectionToFile(StringRef SecName, StringRef Filename,
                                Object &Obj) {
   for (LoadCommand &LC : Obj.LoadCommands)
     for (const std::unique_ptr<Section> &Sec : LC.Sections) {
       if (Sec->CanonicalName == SecName) {