[llvm-objcopy][MachO] Implement --redefine-sym and --redefine-syms

[MaskRay]

Summary

Similar to D46029 (ELF) and D70036 (COFF), but for MachO.

[jhenderson]

llvm/test/tools/llvm-objcopy/MachO/redefine-symbol.s

3

Any reason you're not using yaml2obj for this?

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

53–57

It looks like there's currently no way to remove individual symbols by name in llvm-objcopy for Mach-O, but I wanted to make sure that this is the right order, i.e. symbols are renamed before they are checked for removal, to make sure things don't break in the future.

For example does objcopy --redefine-sym foo=bar -N foo test.o leave a symbol called bar?

[MaskRay]

Add a test that checks --redefine-sym executes before --strip-symbol/-N.

-N is currently a no-op. This prevents us from making a mistake when -N is implemented.

[MaskRay]

Improve --strip-symbol tests

[MaskRay]

llvm/test/tools/llvm-objcopy/MachO/redefine-symbol.s

3

A YAML test file would be verbose:

--- !mach-o
FileHeader:
  magic:           0xFEEDFACF
  cputype:         0x01000007
  cpusubtype:      0x00000003
  filetype:        0x00000001
  ncmds:           3
  sizeofcmds:      336
  flags:           0x00000000
  reserved:        0x00000000
LoadCommands:
  - cmd:             LC_SEGMENT_64
    cmdsize:         232
    segname:         ''
    vmaddr:          0
    vmsize:          0
    fileoff:         368
    filesize:        0
    maxprot:         7
    initprot:        7
    nsects:          2
    flags:           0
    Sections:
      - sectname:        __text
        segname:         __TEXT
        addr:            0x0000000000000000
        size:            0
        offset:          0x00000170
        align:           0
        reloff:          0x00000000
        nreloc:          0
        flags:           0x80000000
        reserved1:       0x00000000
        reserved2:       0x00000000
        reserved3:       0x00000000
        content:         ''
      - sectname:        __const
        segname:         __TEXT
        addr:            0x0000000000000000
        size:            0
        offset:          0x00000170
        align:           0
        reloff:          0x00000000
        nreloc:          0
        flags:           0x00000000
        reserved1:       0x00000000
        reserved2:       0x00000000
        reserved3:       0x00000000
        content:         ''
  - cmd:             LC_SYMTAB
    cmdsize:         24
    symoff:          368
    nsyms:           2
    stroff:          400
    strsize:         12
  - cmd:             LC_DYSYMTAB
    cmdsize:         80
    ilocalsym:       0
    nlocalsym:       0
    iextdefsym:      0
    nextdefsym:      2
    iundefsym:       2
    nundefsym:       0
    tocoff:          0
    ntoc:            0
    modtaboff:       0
    nmodtab:         0
    extrefsymoff:    0
    nextrefsyms:     0
    indirectsymoff:  0
    nindirectsyms:   0
    extreloff:       0
    nextrel:         0
    locreloff:       0
    nlocrel:         0
LinkEditData:
  NameList:
    - n_strx:          1
      n_type:          0x0F
      n_sect:          2
      n_desc:          0
      n_value:         0
    - n_strx:          6
      n_type:          0x0F
      n_sect:          1
      n_desc:          0
      n_value:         0
  StringTable:
    - ''
    - _foo
    - _func
...

Most fields of MachOYAML::Section (see lib/ObjectYAML/MachOYAML.cpp:276) are required. I feel that replicating what we do for ELF may not be very good for MachO. We can probably change the assembly test to YAML when the YAML files used by yaml2obj/obj2yaml become more succinct.

[jhenderson]

This looks good to me, but not being a Mach-O expert, I can't say for certain that there isn't anything else to be aware of, so perhaps wait a bit to give @seiya or @alexshap or another Mach-O expert a chance to chime in, unless you're confident that there is nothing else to worry about.

llvm/test/tools/llvm-objcopy/MachO/redefine-symbol.s

+# REQUIRES: x86-registered-target
+
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-macos %s -o %t
+
+## A non-existent symbol does not error.
+# RUN: llvm-objcopy --redefine-sym _func=_cnuf1234 --redefine-sym _foo=_ba --redefine-sym=_notexist= %t %t2 2>&1 | count 0
+# RUN: llvm-readobj --symbols %t2 | FileCheck %s
+
+# RUN: echo '_func _cnuf1234 #rename func' > %t.rename.txt
+# RUN: echo '  _foo _ba ' >> %t.rename.txt
+# RUN: echo '_notexist _notexist' >> %t.rename.txt
+# RUN: llvm-objcopy --redefine-syms %t.rename.txt %t %t3 2>&1 | count 0
+# RUN: cmp %t2 %t3
+
+# CHECK:      Symbol {
+# CHECK-NEXT:   Name: _ba (1)
+# CHECK-NEXT:   Extern
+# CHECK-NEXT:   Type: Section (0xE)
+# CHECK-NEXT:   Section: __const (0x2)
+# CHECK-NEXT:   RefType: UndefinedNonLazy (0x0)
+# CHECK-NEXT:   Flags [ (0x0)
+# CHECK-NEXT:   ]
+# CHECK-NEXT:   Value: 0x0
+# CHECK-NEXT: }
+# CHECK-NEXT: Symbol {
+# CHECK-NEXT:   Name: _cnuf1234 (5)
+# CHECK-NEXT:   Extern
+# CHECK-NEXT:   Type: Section (0xE)
+# CHECK-NEXT:   Section: __text (0x1)
+# CHECK-NEXT:   RefType: UndefinedNonLazy (0x0)
+# CHECK-NEXT:   Flags [ (0x0)
+# CHECK-NEXT:   ]
+# CHECK-NEXT:   Value: 0x0
+# CHECK-NEXT: }
+
+.globl _func
+_func:
+
+.section __TEXT,__const
+.globl _foo
+_foo:

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

 
 static void markSymbols(const CopyConfig &Config, Object &Obj) {
   // Symbols referenced from the indirect symbol table must not be removed.
   for (IndirectSymbolEntry &ISE : Obj.IndirectSymTable.Symbols)
     if (ISE.Symbol)
       (*ISE.Symbol)->Referenced = true;
 }
 
-static void removeSymbols(const CopyConfig &Config, Object &Obj) {
+static void updateAndRemoveSymbols(const CopyConfig &Config, Object &Obj) {
+  for (SymbolEntry &Sym : Obj.SymTable) {
+    auto I = Config.SymbolsToRename.find(Sym.Name);
+    if (I != Config.SymbolsToRename.end())
+      Sym.Name = I->getValue();
+  }
+
   auto RemovePred = [Config](const std::unique_ptr<SymbolEntry> &N) {
     if (N->Referenced)
       return false;
     return Config.StripAll;
   };
 
   Obj.SymTable.removeSymbols(RemovePred);
 }
 
 static Error handleArgs(const CopyConfig &Config, Object &Obj) {
   if (Config.AllowBrokenLinks || !Config.BuildIdLinkDir.empty() ||
       Config.BuildIdLinkInput || Config.BuildIdLinkOutput ||
       !Config.SplitDWO.empty() || !Config.SymbolsPrefix.empty() ||
       !Config.AllocSectionsPrefix.empty() || !Config.AddSection.empty() ||
       !Config.DumpSection.empty() || !Config.KeepSection.empty() ||
       Config.NewSymbolVisibility || !Config.SymbolsToGlobalize.empty() ||
       !Config.SymbolsToKeep.empty() || !Config.SymbolsToLocalize.empty() ||
       !Config.SymbolsToWeaken.empty() || !Config.SymbolsToKeepGlobal.empty() ||
-      !Config.SectionsToRename.empty() || !Config.SymbolsToRename.empty() ||
+      !Config.SectionsToRename.empty() ||
       !Config.UnneededSymbolsToRemove.empty() ||
       !Config.SetSectionAlignment.empty() || !Config.SetSectionFlags.empty() ||
       !Config.ToRemove.empty() || Config.ExtractDWO || Config.KeepFileSymbols ||
       Config.LocalizeHidden || Config.PreserveDates || Config.StripAllGNU ||
       Config.StripDWO || Config.StripNonAlloc || Config.StripSections ||
       Config.Weaken || Config.DecompressDebugSections || Config.StripDebug ||
       Config.StripNonAlloc || Config.StripSections || Config.StripUnneeded ||
       Config.DiscardMode != DiscardType::None || !Config.SymbolsToAdd.empty() ||
       Config.EntryExpr) {
     return createStringError(llvm::errc::invalid_argument,
                              "option not supported by llvm-objcopy for MachO");
   }
 
   removeSections(Config, Obj);
 
   // Mark symbols to determine which symbols are still needed.
   if (Config.StripAll)
     markSymbols(Config, Obj);
 
-  removeSymbols(Config, Obj);
+  updateAndRemoveSymbols(Config, Obj);
 
   if (Config.StripAll)
     for (LoadCommand &LC : Obj.LoadCommands)
       for (Section &Sec : LC.Sections)
         Sec.Relocations.clear();
 
   return Error::success();
 }

llvm/tools/llvm-objcopy/MachO/Object.h

struct SymbolEntry {
   }
 };
 
 /// The location of the symbol table inside the binary is described by LC_SYMTAB
 /// load command.
 struct SymbolTable {
   std::vector<std::unique_ptr<SymbolEntry>> Symbols;
 
+  using iterator = pointee_iterator<
+      std::vector<std::unique_ptr<SymbolEntry>>::const_iterator>;
+
+  iterator begin() const { return iterator(Symbols.begin()); }
+  iterator end() const { return iterator(Symbols.end()); }
+
   const SymbolEntry *getSymbolByIndex(uint32_t Index) const;
   SymbolEntry *getSymbolByIndex(uint32_t Index);
   void removeSymbols(
       function_ref<bool(const std::unique_ptr<SymbolEntry> &)> ToRemove);
 };
 
 struct IndirectSymbolEntry {
   // The original value in an indirect symbol table. Higher bits encode extra