[llvm-install-name-tool] Add -id option

[sameerarora101]

Summary

Implement -id option for install-name-tool. Differences from cctool's
behavior:

• Does NOT throw an error if multiple -id options are specified. Instead, picks the last one.
• Throws an error in case empty id is specified.

[sameerarora101]

llvm/test/tools/llvm-objcopy/MachO/install-name-tool-id.test

12

NOTE: this behavior is different from cctool's. The cctool's version throws an error on specifying multiple -id options, but keeping @jhenderson comments in mind from -delete_rpath, we allowed for multiple -id options currently.

18

NOTE: this behavior is also different from cctool's. cctools allows for an empty id string (i.e -id ''). But we thought it would not be useful to allow for empty strings and so we throw an error right now.

[jhenderson]

llvm/test/tools/llvm-objcopy/MachO/install-name-tool-id.test

25

Am I right in thinking that you're just using the x86_64.yaml input because it doesn't have an LC_ID_DYLIB load command?

I have a personal preference to keep inputs local to their usage where possible, as it makes the test self-contained. To achieve that in this case, you can use yaml2obj's --docnum option to allow multiple YAML docs in the same test file. I'd do something like:

# RUN: yaml2obj %s --docnum=1 -o %t
<tests and checks using the first doc>

--- !mach-o
<yaml doc 1>

# RUN: yaml2obj %s --docnum=2 -o %t
<tests and checks using the second doc>

--- !mach-o
<yaml doc 2>

30–34

This is somewhat similar to the empty argument case. I'd move it to be adjacent in the test file.

llvm/tools/llvm-objcopy/CopyConfig.h

184–185

It's really that important, but I wonder if it's time to refactor out the Mach-O specific options into a MachOCopyConfig, a bit like the existing ELF config? Just something to think about, and not part of this patch.

llvm/tools/llvm-objcopy/InstallNameToolOpts.td

27

What is this KIND_SEPARATE about here and in the other options?

[sameerarora101]

llvm/test/tools/llvm-objcopy/MachO/install-name-tool-id.test

25

really nice, thanks!

llvm/tools/llvm-objcopy/CopyConfig.h

184–185

yup, I can try creating a diff for this.

llvm/tools/llvm-objcopy/InstallNameToolOpts.td

27

KIND_SEPARATE is used to specify an option which is followed by its value. Something like option <value>. I picked this definition from here https://github.com/llvm-mirror/llvm/blob/master/include/llvm/Option/OptParser.td.

[sameerarora101]

Updating test to contain the second binary.

[jhenderson]

LGTM, with one suggestion in the test.

llvm/test/tools/llvm-objcopy/MachO/install-name-tool-id.test

69–73

Can you get rid of the LoadCommands here completely?

llvm/tools/llvm-objcopy/CopyConfig.h

184–185

Just to be clear, in case it wasn't obvious, I meant "It's NOT really that important..." - see if it's worthwhile and why we do it for the ELF case.

[sameerarora101]

Removing unnecessary LC from test.

[sameerarora101]

Rebasing (for harbormaster)

[sameerarora101]

Rebasing

[sameerarora101]

Rebasing over latest origin/master

[smeenai]

LGTM

[thakis]

llvm/test/tools/llvm-objcopy/MachO/install-name-tool-id.test

14

This breaks check-llvm for anyone who has their LLVM checkout in a directory that has "/usr" anywhere on their checkout path, for example this bot: http://45.33.8.238/linux/21750/step_12.txt

[smeenai]

llvm/test/tools/llvm-objcopy/MachO/install-name-tool-id.test

14

Thanks for the report, fixed in rG5f56da3763ac

llvm/test/tools/llvm-objcopy/MachO/install-name-tool-id.test

+## This test checks updating a dynamic shared library ID in a MachO binary.
+
+# RUN: yaml2obj %s --docnum=1 -o %t
+
+## Specifying -id once:
+# RUN: llvm-install-name-tool -id /usr/lib/A_long_long_test %t
+# RUN: llvm-objdump -p %t | FileCheck %s --check-prefix=ID --implicit-check-not=/usr
+
+# ID: /usr/lib/A_long_long_test
+
+## Specifying -id more than once:
+# RUN: llvm-install-name-tool -id /usr/lib/B_long -id /usr/lib/K_long -id /usr/A_short  %t
+# RUN: llvm-objdump -p %t | FileCheck %s --check-prefix=ID-MULTIPLE --implicit-check-not=/usr
+
+# ID-MULTIPLE: /usr/A_short
+
+## Specifying -id with empty string:
+# RUN: not llvm-install-name-tool -id '' %t 2>&1 | \
+# RUN:   FileCheck %s --check-prefix=EMPTY
+
+# EMPTY: cannot specify an empty id
+
+## Missing id argument:
+# RUN: not llvm-install-name-tool %t -id 2>&1 | \
+# RUN:   FileCheck %s --check-prefix=MISSING
+
+# MISSING: missing argument to -id option
+
+## Shared dylib binary
+--- !mach-o
+FileHeader:
+  magic:           0xFEEDFACF
+  cputype:         0x01000007
+  cpusubtype:      0x00000003
+  filetype:        0x00000006
+  ncmds:           1
+  sizeofcmds:      56
+  flags:           0x00002000
+  reserved:        0x00000000
+LoadCommands:
+  - cmd:                        LC_ID_DYLIB
+    cmdsize:                    56
+    dylib:
+        name:                   24
+        timestamp:              2
+        current_version:        82115073
+        compatibility_version:  65536
+    PayloadString:              '/usr/lib/A'
+
+# RUN: yaml2obj %s --docnum=2 -o %t
+
+## Check that -id option has no effect if binary is not a shared dylib:
+# RUN: cp %t %t1
+# RUN: llvm-install-name-tool -id /usr/lib/J %t
+# RUN: cmp %t %t1
+
+## Executable binary
+--- !mach-o
+FileHeader:
+  magic:           0xFEEDFACF
+  cputype:         0x01000007
+  cpusubtype:      0x00000003
+  filetype:        0x00000001
+  ncmds:           0
+  sizeofcmds:      0
+  flags:           0x00002000
+  reserved:        0x00000000

llvm/tools/llvm-objcopy/CopyConfig.h

struct CopyConfig {
   // Repeated options
   std::vector<StringRef> AddSection;
   std::vector<StringRef> DumpSection;
   std::vector<StringRef> SymbolsToAdd;
   std::vector<StringRef> RPathToAdd;
   std::vector<std::pair<StringRef, StringRef>> RPathsToUpdate;
   DenseSet<StringRef> RPathsToRemove;
 
+  // install-name-tool's id option
+  Optional<StringRef> SharedLibId;
+
   // Section matchers
   NameMatcher KeepSection;
   NameMatcher OnlySection;
   NameMatcher ToRemove;
 
   // Symbol matchers
   NameMatcher SymbolsToGlobalize;
   NameMatcher SymbolsToKeep;

llvm/tools/llvm-objcopy/CopyConfig.cpp

if (It3 != Config.RPathToAdd.end())
       return createStringError(
           errc::invalid_argument,
           "cannot specify both -add_rpath %s and -rpath %s %s",
           It3->str().c_str(), Old.str().c_str(), New.str().c_str());
 
     Config.RPathsToUpdate.emplace_back(Old, New);
   }
 
+  if (auto *Arg = InputArgs.getLastArg(INSTALL_NAME_TOOL_id))
+    Config.SharedLibId = Arg->getValue();
+
   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 add_rpath : Option<["-", "--"], "add_rpath", KIND_SEPARATE>,
                 HelpText<"Add new rpath">;
 
 def delete_rpath: Option<["-", "--"], "delete_rpath", KIND_SEPARATE>,
                   HelpText<"Delete specified rpath">;
 
 def rpath: MultiArg<["-", "--"], "rpath", 2>,
            HelpText<"Change rpath path name">;
 
+def id : Option<["-","--"], "id", KIND_SEPARATE>,
+                 HelpText<"Change dynamic shared library id">;
+
 def version : Flag<["--"], "version">,
               HelpText<"Print the version and exit.">;

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

static Error handleArgs(const CopyConfig &Config, Object &Obj) {
 
   updateAndRemoveSymbols(Config, Obj);
 
   if (Config.StripAll)
     for (LoadCommand &LC : Obj.LoadCommands)
       for (std::unique_ptr<Section> &Sec : LC.Sections)
         Sec->Relocations.clear();
 
+  if (Config.SharedLibId) {
+    StringRef Id = Config.SharedLibId.getValue();
+    if (Id.empty())
+      return createStringError(errc::invalid_argument,
+                               "cannot specify an empty id");
+    for (LoadCommand &LC : Obj.LoadCommands) {
+      if (LC.MachOLoadCommand.load_command_data.cmd == MachO::LC_ID_DYLIB) {
+        updateLoadCommandPayloadString<MachO::dylib_command>(LC, Id);
+        break;
+      }
+    }
+  }
+
   for (const auto &Flag : Config.AddSection) {
     std::pair<StringRef, StringRef> SecPair = Flag.split("=");
     StringRef SecName = SecPair.first;
     StringRef File = SecPair.second;
     if (Error E = isValidMachOCannonicalName(SecName))
       return E;
     if (Error E = addSection(SecName, File, Obj))
       return E;