[tools][llvm-objcopy] Enable --discard-all for MachO

[alexander-shaposhnikov]

Summary

In this diff we enable the option --discard-all for MachO.

Test plan: make check-all

[MaskRay]

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

72

Is this binutils's behavior? I cannot find a reference to N_EXT.

[alexander-shaposhnikov]

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

72

I was looking at how other tools in LLVM check if a macho symbol is global
https://llvm.org/doxygen/MachOObjectFile_8cpp_source.html#l01854
+ checked that on a simple examples cctools's strip -x does the same as llvm-strip.

[MaskRay]

llvm/test/tools/llvm-objcopy/MachO/discard-all.test

5

## for comments.

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

72

I think it is slightly better to use

if (Config.DiscardMode == DiscardType::All && !(N->n_type & MachO::N_EXT))
  return true;

Can you check if objcopy --discard-locals works?

If there are other checks in the future, we won't have to update return !static_cast<bool>(N->n_type & MachO::N_EXT);

179–180

Since --discard-locals has not been implemented, --discard-locals should still be rejected.

[alexander-shaposhnikov]

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

72

yes, there is an equivalent (e.g. in cctools strip) for the option -X, -X is an alias to --discard-locals,
it's also documented here https://www.unix.com/man-page/osx/1/strip/, that option is not implemented in llvm-objcopy for MachO yet, but it's not hard to add (in a separate diff).

[jhenderson]

Looks good from my point of view, but I don't know enough about the Mach-O to verify the behaviour of the option.

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

178–179

Nit: clang-format.

[MaskRay]

Looks good from my point of view. I also don't know enough about Mach-O, though...

[smeenai]

LGTM from the Mach-O perspective.

llvm/test/tools/llvm-objcopy/MachO/discard-all.test

12

This is verifying that it produces identical output for two different runs, right? Not that it didn't modify the input?

[alexander-shaposhnikov]

llvm/test/tools/llvm-objcopy/MachO/discard-all.test

12

yup

llvm/test/tools/llvm-objcopy/MachO/discard-all.test

+# RUN: yaml2obj %s -o %t
+# RUN: cp %t %t1
+# RUN: llvm-objcopy --discard-all %t %t2
+## Verify that llvm-objcopy has not modified the input.
+# RUN: cmp %t %t1
+# RUN: llvm-readobj --symbols %t2 | FileCheck %s
+
+# RUN: llvm-objcopy -x %t %t3
+## Verify that llvm-objcopy has not modified the input.
+# RUN: cmp %t %t1
+# RUN: cmp %t2 %t3
+
+## Verify that llvm-strip modifies the symbol table the same way.
+# RUN: cp %t %t4
+# RUN: llvm-strip --discard-all %t4
+# RUN: cmp %t2 %t4
+# RUN: llvm-strip -x %t
+# RUN: cmp %t2 %t
+
+# CHECK:       Symbols [
+# CHECK-NEXT:    Symbol {
+# CHECK-NEXT:      Name: _b (1)
+# CHECK-NEXT:      Extern
+# CHECK-NEXT:      Type: Undef (0x0)
+# CHECK-NEXT:      Section:  (0x0)
+# CHECK-NEXT:      RefType: UndefinedNonLazy (0x0)
+# CHECK-NEXT:      Flags [ (0x220)
+# CHECK-NEXT:        AltEntry (0x200)
+# CHECK-NEXT:        NoDeadStrip (0x20)
+# CHECK-NEXT:      ]
+# CHECK-NEXT:      Value: 0x4
+# CHECK-NEXT:    }
+# CHECK-NEXT:  ]
+
+--- !mach-o
+FileHeader:
+  magic:           0xFEEDFACF
+  cputype:         0x01000007
+  cpusubtype:      0x00000003
+  filetype:        0x00000001
+  ncmds:           4
+  sizeofcmds:      360
+  flags:           0x00002000
+  reserved:        0x00000000
+LoadCommands:
+  - cmd:             LC_SEGMENT_64
+    cmdsize:         232
+    segname:         ''
+    vmaddr:          0
+    vmsize:          4
+    fileoff:         392
+    filesize:        0
+    maxprot:         7
+    initprot:        7
+    nsects:          2
+    flags:           0
+    Sections:
+      - sectname:        __text
+        segname:         __TEXT
+        addr:            0x0000000000000000
+        size:            0
+        offset:          0x00000188
+        align:           0
+        reloff:          0x00000000
+        nreloc:          0
+        flags:           0x80000000
+        reserved1:       0x00000000
+        reserved2:       0x00000000
+        reserved3:       0x00000000
+        content:         ''
+      - sectname:        __bss
+        segname:         __DATA
+        addr:            0x0000000000000000
+        size:            4
+        offset:          0x00000000
+        align:           2
+        reloff:          0x00000000
+        nreloc:          0
+        flags:           0x00000001
+        reserved1:       0x00000000
+        reserved2:       0x00000000
+        reserved3:       0x00000000
+  - cmd:             LC_BUILD_VERSION
+    cmdsize:         24
+    platform:        1
+    minos:           658944
+    sdk:             658944
+    ntools:          0
+  - cmd:             LC_SYMTAB
+    cmdsize:         24
+    symoff:          392
+    nsyms:           2
+    stroff:          424
+    strsize:         8
+  - cmd:             LC_DYSYMTAB
+    cmdsize:         80
+    ilocalsym:       0
+    nlocalsym:       1
+    iextdefsym:      1
+    nextdefsym:      0
+    iundefsym:       1
+    nundefsym:       1
+    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:          4
+      n_type:          0x0E
+      n_sect:          2
+      n_desc:          32
+      n_value:         0
+    - n_strx:          1
+      n_type:          0x01
+      n_sect:          0
+      n_desc:          544
+      n_value:         4
+  StringTable:
+    - ''
+    - _b
+    - _a
+    - ''
+...

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

for (SymbolEntry &Sym : Obj.SymTable) {
     auto I = Config.SymbolsToRename.find(Sym.Name);
     if (I != Config.SymbolsToRename.end())
       Sym.Name = std::string(I->getValue());
   }
 
   auto RemovePred = [Config](const std::unique_ptr<SymbolEntry> &N) {
     if (N->Referenced)
       return false;
-    return Config.StripAll;
+    if (Config.StripAll)
+      return true;
+    if (Config.DiscardMode == DiscardType::All && !(N->n_type & MachO::N_EXT))
+      return true;
+    return false;
   };
 
   Obj.SymTable.removeSymbols(RemovePred);
 }
 
 static LoadCommand buildRPathLoadCommand(StringRef Path) {
   LoadCommand LC;
   MachO::rpath_command RPathLC;
if (Config.AllowBrokenLinks || !Config.BuildIdLinkDir.empty() ||
       !Config.SymbolsToWeaken.empty() || !Config.SymbolsToKeepGlobal.empty() ||
       !Config.SectionsToRename.empty() ||
       !Config.UnneededSymbolsToRemove.empty() ||
       !Config.SetSectionAlignment.empty() || !Config.SetSectionFlags.empty() ||
       Config.ExtractDWO || Config.KeepFileSymbols || Config.LocalizeHidden ||
       Config.PreserveDates || Config.StripAllGNU || Config.StripDWO ||
       Config.StripNonAlloc || Config.StripSections || Config.Weaken ||
       Config.DecompressDebugSections || Config.StripNonAlloc ||
       Config.StripSections || Config.StripUnneeded ||
-      Config.DiscardMode != DiscardType::None || !Config.SymbolsToAdd.empty() ||
-      Config.EntryExpr) {
+      Config.DiscardMode == DiscardType::Locals ||
+      !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);