[llvm-objcopy][MachO] Implement --only-section

[alexander-shaposhnikov]

llvm/tools/llvm-objcopy/CopyConfig.h

104

Name.conains(',')

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

28

return !is_contained(Config.OnlySection, Sec.CanonicalName)

[alexander-shaposhnikov]

llvm/test/tools/llvm-objcopy/MachO/only-section.test

3

0. I think we need to make this test more robust and verify that the content of those sections is correct as well as the load commands, etc
we also need tests:

1. where the specified arguments are invalid (to test error messages)
2. what happens when the specified section name is not present in the input binary
3. where all the sections should stay in place (i.e. --only-section lists all the available sections) -> the output binary should be "the same" (unless there is some ambiguity, i.e. the values of the padding bytes might be different, but I'd add a small test (as small as possible))

[seiya]

• Addressed some review comments.

[seiya]

llvm/test/tools/llvm-objcopy/MachO/only-section.test

3

0. I think we need to make this test more robust and verify that the content of those sections is correct as well as the load commands, etc

AFAIK, yaml2macho.cpp fills sections with dummy data (0xDEADBEEFu) so I'll write another patch to allow specifying arbitrary binary data in a MachO YAML.

[seiya]

• Addressed a review comment.

[seiya]

Re-upload the diff: the previous update was mistake.

[seiya]

• Improved the test cases.

[seiya]

• Added some newlines for readability.

[jhenderson]

This goes for all the new functionality you are adding, but make sure to review the llvm-strip and llvm-objcopy documentation (llvm/docs/CommandGuide) for any changes that need to be made. For example, if any of the switches are currently listed as "ELF specific" they need to be moved into the general switch section.

[rupprecht]

llvm/tools/llvm-objcopy/CopyConfig.cpp

318

These errors should also include the full Name string so the user knows specifically which flag is the culprit.

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

35–46

Aside: we still have a lot of ELF-specific stuff in CopyConfig, which should theoretically be relatively platform-agnostic -- for example, the --add-symbol parsing code creates functors that assign the symbol binding to ELF::STB_* values. We should think about how to properly separate ELF/MachO/COFF specific things there.

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

15–17

SymTable/StrTable are explicitly referenced in MachO/Object.h, so if either of those are matched by the predicate, those tables should be cleared out too. (And there should be test cases to verify this).

Also, you might want to use stable_sort to remove sections -- see ELF's implementation.

Although specific to --only-section (see replaceAndRemoveSections, ELFObjcopy.cpp), StrTable/SymTab should not be implicitly removed, so it's not really observable with just this patch.

[seiya]

• Updated CommandGuide.
• Addressed a review comment.

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

35–46

I think so too. What complicates the problem is that we don't know the file format until we parse the command line options and decode the input file. How about splitting the command-line parsing into two stages?

CopyConfig.h

struct UnparsedNewSymbolInfo {
  StringRef Bind; // Don't parse it for now.
  // ...
};

class CopyConfig {
  // Format-independent options.
  StringRef InputFilename;
  FileFormat InputFormat;
  MachineInfo BinaryArch;

  // ... 

  // Format-specific options.
  UnparsedNewSymbolInfo NewSymbolInfo;
};

ELFCopyConfig.h

struct NewSymbolInfo {
  uint8_t Type = ELF::STB_GLOBAL;
  // ...
};

class ELFCopyConfig : public CopyConfig {
  ELFCopyConfig(CopyConfig &Config) {
    // We should cache the result in case there're many input files to process.
    parse(Config);
  }
};

ELFObjcopy.cpp

Error executeObjcopyOnBinary(const CopyConfig &UnparsedConfig, ...) {
  ELFCopyConfig Config(UnparsedConfig);
  // ...
}

In the first stage (CopyConfig), we parse only format-independent options. In second stage (ELFCopyConfig), we parse format-specific options (the Bind field, in this example).

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

15–17

SymTable/StrTable are explicitly referenced in MachO/Object.h, so if either of those are matched by the predicate, those tables should be cleared out too. (And there should be test cases to verify this).

Unlike ELF, in MachO, SymTable/StrTable are not sections, but are load commands. Thus, we never remove them by this removeSections.

Also, you might want to use stable_sort to remove sections -- see ELF's implementation.

Perhaps you meant stable_partition?

[seiya]

Refactored a bit.

[jhenderson]

llvm/test/tools/llvm-objcopy/MachO/only-section.test

7

Error case 1? Where are error case 2, 3...?

[seiya]

Fix a review comment.

llvm/test/tools/llvm-objcopy/MachO/only-section.test

7

It's certainly confusing. Removed 1.

[jhenderson]

llvm/test/tools/llvm-objcopy/MachO/only-section.test

16

"Specify __TEXT,__text" -> "Specify one section."
"only the" -> "only that"

59–60

You say "without warning" here, but don't actually check for a warning, which you probably should do, if you're going to mention it in the comment.

[seiya]

Updated CommandGuide.

[rupprecht]

LGTM, once a MachO reviewer signs off. No action needed for my two comments.

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

35–46

Aside: woah, didn't realize phab could display cool tabbed codeblocks like that.

Yes, that's a decent sketch for how it might work. CopyConfig itself should be returning uninterpreted StringRefs instead of ELF/MachO-specific things.

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

15–17

Yeah, stable_partition, good catch.

In retrospect, I think the use of ELF/removeSections actually may not need to use stable_partition, and switching to remove_if may be faster. IIUC the difference is that stable_partition guarantees the things past the partition point are also in stable order, whereas remove_if does not (and is faster for not having to). So disregard my original suggestion.

[seiya]

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

35–46

OK, I'll propose it in a separate patch.

(I've been wanting to try this feature. It's neat.)

[seiya]

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

15–17

As you say, it seems that we should use remove_if instead. According to this blog post (lacks the details of the experiment environment though), remove_if is faster than stable_partiton. I think normally the number of sections are small, so using remove_if improve the performance not so much though.

[seiya]

I'll submit another patch to refactor CopyConfig as described in https://reviews.llvm.org/D65541#inline-594247

[seiya]

Add MachOConfig.cpp to isolate command-line validation logic from MachOObjCopy.cpp.

[jhenderson]

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

23–24

I know this was there before, but I'm not sure I understand this TODO. I think it's saying that support should be added to GNU objcopy for something? If so, I don't think you want this TODO in the code. TODOs in LLVM code should be about LLVM changes that should be made at some point.

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

13–14

I don't think you need these two includes here.

23–24

Is this clang-formatted? I thought it should be struct MachOCopyConfig {}; but could be wrong easily.

It might be worth a simple comment saying why there's an empty struct. I initially thought it was an incorrect attempt to forward declare it!

[seiya]

Addressed review comments.

[seiya]

llvm/test/tools/llvm-objcopy/MachO/only-section.test

59–60

Removed "without warning" from the comment since the behavior is not so surprising.

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

23–24

Now I noticed that comment does not make sense. This TODO is for LLVM code, not GNU objcopy. Rephrased the comment.

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

23–24

It might be worth a simple comment saying why there's an empty struct. I initially thought it was an incorrect attempt to forward declare it!

Good point! Added a comment.

[jhenderson]

I don't know about any Mach-O specific details, but everything else looks good to me. Best get somebody else with Mach-O expertise to confirm.

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

42

CannonicalName -> CanonicalName

[seiya]

Addressed a review comment.

[seiya]

Friendly ping: I'd like someone who is familiar with Mach-O to review this patch.

[seiya]

@alexshap Could you review this patch? It's accepted but I'd like someone who's familiar with Mach-O to confirm (a ping in case you missed changes in this patch).

[alexander-shaposhnikov]

llvm/tools/llvm-objcopy/CopyConfig.cpp

338

khm, maybe there is a named constant for this ? (somewhere in llvm::MachO:: )

[alexander-shaposhnikov]

@seiya - thanks for all your work, I'm wondering - can you commit this change / is there anything blocking this diff ?

[seiya]

Thank you for your review!

In D65541#1711938, @alexshap wrote:

@seiya - thanks for all your work, I'm wondering - can you commit this change / is there anything blocking this diff ?

There're no diffs that blocks this patch. I'll commit this patch along with D66280 and D66281.

Other Mach-O support patches for llvm-objcopy needs addressing some review comments. I'll let you know once I update them.

llvm/tools/llvm-objcopy/CopyConfig.cpp

338

I've git-grep'ed through llvm::MachO and lld but there were no constant for it (lld hard-codes the value).

[seiya]

Before committing this patch, I noticed that we need to consider how llvm-objcopy should handle wildcards in Mach-O support.

First I thought that validating the segment/section names would be helpful for users, for example:

$ llvm-objcopy llvm-objcopy --only-section __text foo
error: invalid section name '__text' (should be formatted as '<segment name>,<section name>')

However, the validation (check that the option value is formatted as <segment name>,<section name>) does not make sense when taking into account wildcards. In GNU objcopy, it seems it does not care about the segment/section name separator (.) at all. In the following example, GNU objcopy accepts both __TEXT* and __TEXT.* and outputs identical outputs.

$ gobjcopy llvm-objcopy --only-section '__TEXT*' foo foo2
$ gobjcopy llvm-objcopy --only-section '__TEXT.*' foo foo3
$ cmp foo2 foo3
$ echo $?
0

In this change, I've removed the validation to support the GNU objcopy's wildcard behavior for Mach-O objects as described above and added a test case for wildcard.

[seiya]

Ping @alexshap (and also @jhenderson and @rupprecht would want to confirm before landing). After this patch got accepted, in order to merge it, I added wildcard support and a test in this diff.

[seiya]

Updated a test.

[rupprecht]

LGTM with the wildcard changes, thanks!

llvm/docs/CommandGuide/llvm-objcopy.rst

 
  Print a summary of command line options.
 
 .. option:: --only-section <section>, -j
 
  Remove all sections from the output, except for sections named ``<section>``.
  Can be specified multiple times to keep multiple sections.
 
+ For MachO objects, ``<section>`` must be formatted as
+ ``<segment name>,<section name>``.
+
 .. option:: --regex
 
  If specified, symbol and section names specified by other switches are treated
  as extended POSIX regular expression patterns.
 
 .. option:: --remove-section <section>, -R
 
  Remove the specified section from the output. Can be specified multiple times

llvm/test/tools/llvm-objcopy/MachO/only-section.test

+## Show that if --only-section is given, llvm-objcopy removes all sections
+## except ones specified in the option.
+
+# RUN: yaml2obj %s > %t
+
+## Error case: Invalid section name.
+# RUN: not llvm-objcopy --only-section __text %t %t2 2>&1 \
+# RUN:   | FileCheck %s --check-prefix=BAD-SECTION-NAME
+# BAD-SECTION-NAME: error: invalid section name '__text' (should be formatted as '<segment name>,<section name>')
+
+## Specify all sections. The output file should be the same as the input.
+# RUN: llvm-objcopy -j __TEXT,__text -j __DATA,__data -j __TEXT,__const %t %t3
+# RUN: cmp %t %t3
+
+## Specify one section. The output file should contain only that section.
+# RUN: llvm-objcopy --only-section __TEXT,__text %t %t4
+# RUN: llvm-readobj --sections --section-data --macho-segment %t4 \
+# RUN:   | FileCheck %s --check-prefix=ONLY-TEXT-SECTION
+
+# ONLY-TEXT-SECTION:      Sections [
+# ONLY-TEXT-SECTION-NEXT:   Section {
+# ONLY-TEXT-SECTION-NEXT:     Index: 0
+# ONLY-TEXT-SECTION-NEXT:     Name: __text (5F 5F 74 65 78 74 00 00 00 00 00 00 00 00 00 00)
+# ONLY-TEXT-SECTION-NEXT:     Segment: __TEXT (5F 5F 54 45 58 54 00 00 00 00 00 00 00 00 00 00)
+# ONLY-TEXT-SECTION-NEXT:     Address: 0x0
+# ONLY-TEXT-SECTION-NEXT:     Size: 0x4
+# ONLY-TEXT-SECTION-NEXT:     Offset: 184
+# ONLY-TEXT-SECTION-NEXT:     Alignment: 0
+# ONLY-TEXT-SECTION-NEXT:     RelocationOffset: 0x0
+# ONLY-TEXT-SECTION-NEXT:     RelocationCount: 0
+# ONLY-TEXT-SECTION-NEXT:     Type: Regular (0x0)
+# ONLY-TEXT-SECTION-NEXT:     Attributes [ (0x800004)
+# ONLY-TEXT-SECTION-NEXT:       PureInstructions (0x800000)
+# ONLY-TEXT-SECTION-NEXT:       SomeInstructions (0x4)
+# ONLY-TEXT-SECTION-NEXT:     ]
+# ONLY-TEXT-SECTION-NEXT:     Reserved1: 0x0
+# ONLY-TEXT-SECTION-NEXT:     Reserved2: 0x0
+# ONLY-TEXT-SECTION-NEXT:     Reserved3: 0x0
+# ONLY-TEXT-SECTION-NEXT:     SectionData (
+# ONLY-TEXT-SECTION-NEXT:       0000: AABBCCDD                             |....|
+# ONLY-TEXT-SECTION-NEXT:     )
+# ONLY-TEXT-SECTION-NEXT:   }
+# ONLY-TEXT-SECTION-NEXT: ]
+# ONLY-TEXT-SECTION-NEXT: Segment {
+# ONLY-TEXT-SECTION-NEXT:   Cmd: LC_SEGMENT_64
+# ONLY-TEXT-SECTION-NEXT:   Name:
+# ONLY-TEXT-SECTION-NEXT:   Size: 152
+# ONLY-TEXT-SECTION-NEXT:   vmaddr: 0x0
+# ONLY-TEXT-SECTION-NEXT:   vmsize: 0x4
+# ONLY-TEXT-SECTION-NEXT:   fileoff: 184
+# ONLY-TEXT-SECTION-NEXT:   filesize: 4
+# ONLY-TEXT-SECTION-NEXT:   maxprot: rwx
+# ONLY-TEXT-SECTION-NEXT:   initprot: rwx
+# ONLY-TEXT-SECTION-NEXT:   nsects: 1
+# ONLY-TEXT-SECTION-NEXT:   flags: 0x0
+# ONLY-TEXT-SECTION-NEXT: }
+
+## Remove all sections if the specified section name is not present in the input.
+# RUN: llvm-objcopy --only-section __TEXT,__nonexistent %t %t5 2>&1
+# RUN: llvm-readobj --sections --section-data --macho-segment %t5 \
+# RUN:   | FileCheck %s --check-prefix=NONEXISTENT-SECTION
+
+# NONEXISTENT-SECTION:      Sections [
+# NONEXISTENT-SECTION-NEXT: ]
+# NONEXISTENT-SECTION-NEXT: Segment {
+# NONEXISTENT-SECTION-NEXT:   Cmd: LC_SEGMENT_64
+# NONEXISTENT-SECTION-NEXT:   Name:
+# NONEXISTENT-SECTION-NEXT:   Size: 72
+# NONEXISTENT-SECTION-NEXT:   vmaddr: 0x0
+# NONEXISTENT-SECTION-NEXT:   vmsize: 0x0
+# NONEXISTENT-SECTION-NEXT:   fileoff: 104
+# NONEXISTENT-SECTION-NEXT:   filesize: 0
+# NONEXISTENT-SECTION-NEXT:   maxprot: rwx
+# NONEXISTENT-SECTION-NEXT:   initprot: rwx
+# NONEXISTENT-SECTION-NEXT:   nsects: 0
+# NONEXISTENT-SECTION-NEXT:   flags: 0x0
+# NONEXISTENT-SECTION-NEXT: }
+
+--- !mach-o
+FileHeader:
+  magic:           0xFEEDFACF
+  cputype:         0x01000007
+  cpusubtype:      0x00000003
+  filetype:        0x00000001
+  ncmds:           1
+  sizeofcmds:      312
+  flags:           0x00002000
+  reserved:        0x00000000
+LoadCommands:
+  - cmd:             LC_SEGMENT_64
+    cmdsize:         312
+    segname:         ''
+    vmaddr:          0
+    vmsize:          12
+    fileoff:         344
+    filesize:        12
+    maxprot:         7
+    initprot:        7
+    nsects:          3
+    flags:           0
+    Sections:
+      - sectname:        __text
+        segname:         __TEXT
+        addr:            0x0000000000000000
+        content:         'AABBCCDD'
+        size:            4
+        offset:          344
+        align:           0
+        reloff:          0x00000000
+        nreloc:          0
+        flags:           0x80000400
+        reserved1:       0x00000000
+        reserved2:       0x00000000
+        reserved3:       0x00000000
+      - sectname:        __data
+        segname:         __DATA
+        addr:            0x0000000000000004
+        content:         'DDAADDAA'
+        size:            4
+        offset:          348
+        align:           0
+        reloff:          0x00000000
+        nreloc:          0
+        flags:           0x00000000
+        reserved1:       0x00000000
+        reserved2:       0x00000000
+        reserved3:       0x00000000
+      - sectname:        __const
+        segname:         __TEXT
+        addr:            0x0000000000000008
+        content:         'EEFFEEFF'
+        size:            4
+        offset:          352
+        align:           0
+        reloff:          0x00000000
+        nreloc:          0
+        flags:           0x00000000
+        reserved1:       0x00000000
+        reserved2:       0x00000000
+        reserved3:       0x00000000

llvm/tools/llvm-objcopy/CMakeLists.txt

add_llvm_tool(llvm-objcopy
   llvm-objcopy.cpp
   COFF/COFFObjcopy.cpp
   COFF/Object.cpp
   COFF/Reader.cpp
   COFF/Writer.cpp
   ELF/ELFConfig.cpp
   ELF/ELFObjcopy.cpp
   ELF/Object.cpp
+  MachO/MachOConfig.cpp
   MachO/MachOObjcopy.cpp
   MachO/MachOReader.cpp
   MachO/MachOWriter.cpp
   MachO/MachOLayoutBuilder.cpp
   MachO/Object.cpp
   DEPENDS
   ObjcopyOptsTableGen
   StripOptsTableGen
   )
 
 add_llvm_tool_symlink(llvm-strip llvm-objcopy)
 
 if(LLVM_INSTALL_BINUTILS_SYMLINKS)
   add_llvm_tool_symlink(objcopy llvm-objcopy)
   add_llvm_tool_symlink(strip llvm-objcopy)
 endif()

llvm/tools/llvm-objcopy/CopyConfig.h

 //===- CopyConfig.h -------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_TOOLS_LLVM_OBJCOPY_COPY_CONFIG_H
 #define LLVM_TOOLS_LLVM_OBJCOPY_COPY_CONFIG_H
 
 #include "ELF/ELFConfig.h"
+#include "MachO/MachOConfig.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/BitmaskEnum.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Object/ELFTypes.h"
 #include "llvm/Support/Allocator.h"
class NameOrRegex {
   StringRef Name;
   // Regex is shared between multiple CopyConfig instances.
   std::shared_ptr<Regex> R;
 
 public:
   NameOrRegex(StringRef Pattern, bool IsRegex);
   bool operator==(StringRef S) const { return R ? R->match(S) : Name == S; }
   bool operator!=(StringRef S) const { return !operator==(S); }
+  Error isMachOCannonicalName() const;
 };
 
 // Matcher that checks symbol or section names against the command line flags
 // provided for that option.
 class NameMatcher {
   std::vector<NameOrRegex> Matchers;
-
 public:
   void addMatcher(NameOrRegex Matcher) {
     Matchers.push_back(std::move(Matcher));
   }
   bool matches(StringRef S) const { return is_contained(Matchers, S); }
   bool empty() const { return Matchers.empty(); }
+  const std::vector<NameOrRegex> &matchers() const { return Matchers; }
 };
 
 // Configuration for copying/stripping a single file.
 struct CopyConfig {
   // Format-specific options to be initialized lazily when needed.
   Optional<elf::ELFCopyConfig> ELF;
+  Optional<macho::MachOCopyConfig> MachO;
 
   // Main input/output options
   StringRef InputFilename;
   FileFormat InputFormat;
   StringRef OutputFilename;
   FileFormat OutputFormat;
 
   // Only applicable when --output-format!=binary (e.g. elf64-x86-64).
Error parseELFConfig() {
     if (!ELF) {
       Expected<elf::ELFCopyConfig> ELFConfig = elf::parseConfig(*this);
       if (!ELFConfig)
         return ELFConfig.takeError();
       ELF = *ELFConfig;
     }
     return Error::success();
   }
+
+  // parseMachOConfig performs Mach-O-specific command-line parsing. Fills
+  // `MachO` on success or returns an Error otherwise.
+  Error parseMachOConfig() {
+    if (!MachO) {
+      Expected<macho::MachOCopyConfig> MachOConfig = macho::parseConfig(*this);
+      if (!MachOConfig)
+        return MachOConfig.takeError();
+      MachO = *MachOConfig;
+    }
+    return Error::success();
+  }
 };
 
 // Configuration for the overall invocation of this tool. When invoked as
 // objcopy, will always contain exactly one CopyConfig. When invoked as strip,
 // will contain one or more CopyConfigs.
 struct DriverConfig {
   SmallVector<CopyConfig, 1> CopyConfigs;
   BumpPtrAllocator Alloc;
 };
 
+// isValidMachOCannonicalName returns success if Name is a MachO cannonical name
+// ("<segment>,<section>") and lengths of both segment and section names are
+// valid.
+Error isValidMachOCannonicalName(StringRef Name);
+
 // ParseObjcopyOptions returns the config and sets the input arguments. If a
 // help flag is set then ParseObjcopyOptions will print the help messege and
 // exit.
 Expected<DriverConfig> parseObjcopyOptions(ArrayRef<const char *> ArgsArr);
 
 // ParseStripOptions returns the config and sets the input arguments. If a
 // help flag is set then ParseStripOptions will print the help messege and
 // exit. ErrorCallback is used to handle recoverable errors. An Error returned

llvm/tools/llvm-objcopy/CopyConfig.cpp

if (!IsRegex) {
     return;
   }
 
   SmallVector<char, 32> Data;
   R = std::make_shared<Regex>(
       ("^" + Pattern.ltrim('^').rtrim('$') + "$").toStringRef(Data));
 }
 
+Error NameOrRegex::isMachOCannonicalName() const {
+  return R ? Error::success() : isValidMachOCannonicalName(Name);
+}
+
 static Error addSymbolsToRenameFromFile(StringMap<StringRef> &SymbolsToRename,
                                         BumpPtrAllocator &Alloc,
                                         StringRef Filename) {
   StringSaver Saver(Alloc);
   SmallVector<StringRef, 16> Lines;
   auto BufOrErr = MemoryBuffer::getFile(Filename);
   if (!BufOrErr)
     return createFileError(Filename, BufOrErr.getError());
template <class T> static ErrorOr<T> getAsInteger(StringRef Val) {
   return Result;
 }
 
 static void printHelp(const opt::OptTable &OptTable, raw_ostream &OS,
                       StringRef ToolName) {
   OptTable.PrintHelp(OS, (ToolName + " input [output]").str().c_str(),
                      (ToolName + " tool").str().c_str());
   // TODO: Replace this with libOption call once it adds extrahelp support.
   // The CommandLine library has a cl::extrahelp class to support this,
   // but libOption does not have that yet.
   OS << "\nPass @FILE as argument to read options from FILE.\n";
 }
 
+// isValidMachOCannonicalName returns success if Name is a MachO cannonical name
+// ("<segment>,<section>") and lengths of both segment and section names are
+// valid.
+Error isValidMachOCannonicalName(StringRef Name) {
+  if (Name.count(',') != 1)
+    return createStringError(errc::invalid_argument,
+                             "invalid section name '%s' (should be formatted "
+                             "as '<segment name>,<section name>')",
+                             Name.str().c_str());
+
+  std::pair<StringRef, StringRef> Pair = Name.split(',');
+  if (Pair.first.size() > 16)
+    return createStringError(errc::invalid_argument,
+                             "too long segment name: '%s'",
+                             Pair.first.str().c_str());
+  if (Pair.second.size() > 16)
+    return createStringError(errc::invalid_argument,
+                             "too long section name: '%s'",
+                             Pair.second.str().c_str());
+  return Error::success();
+}
+
 // ParseObjcopyOptions returns the config and sets the input arguments. If a
 // help flag is set then ParseObjcopyOptions will print the help messege and
 // exit.
 Expected<DriverConfig> parseObjcopyOptions(ArrayRef<const char *> ArgsArr) {
   DriverConfig DC;
   ObjcopyOptTable T;
   unsigned MissingArgumentIndex, MissingArgumentCount;
   llvm::opt::InputArgList InputArgs =

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

+//===- MachOConfig.h --------------------------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_TOOLS_OBJCOPY_MACHOCONFIG_H
+#define LLVM_TOOLS_OBJCOPY_MACHOCONFIG_H
+
+#include "llvm/Support/Error.h"
+
+namespace llvm {
+namespace objcopy {
+struct CopyConfig;
+
+namespace macho {
+
+/// Mach-O specific command line options. Currently it's empty and is used to
+/// indicate that we have validated options in macho::parseConfig().
+struct MachOCopyConfig {};
+
+Expected<MachOCopyConfig> parseConfig(const CopyConfig &Config);
+
+} // namespace macho
+} // namespace objcopy
+} // namespace llvm
+
+#endif

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

+//===- MachOConfig.cpp ----------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "MachOConfig.h"
+#include "CopyConfig.h"
+#include "llvm/Support/Error.h"
+
+namespace llvm {
+namespace objcopy {
+namespace macho {
+
+Expected<MachOCopyConfig> parseConfig(const CopyConfig &Config) {
+  MachOCopyConfig MachOConfig;
+
+  // Validate section names.
+  //
+  // TODO: Support GNU objcopy's section renaming rules for compatibility (see
+  // http://lists.llvm.org/pipermail/llvm-dev/2019-May/132570.html).
+  if (!Config.OnlySection.empty()) {
+    for (const NameOrRegex &NR : Config.OnlySection.matchers())
+      if (Error E = NR.isMachOCannonicalName())
+        return std::move(E);
+  }
+
+  return MachOConfig;
+}
+
+} // end namespace macho
+} // end namespace objcopy
+} // end namespace llvm

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

 #include "llvm/Support/Errc.h"
 #include "llvm/Support/Error.h"
 
 namespace llvm {
 namespace objcopy {
 namespace macho {
 
 using namespace object;
+using SectionPred = std::function<bool(const Section &Sec)>;
+
+static void removeSections(const CopyConfig &Config, Object &Obj) {
+  SectionPred RemovePred = [](const Section &) { return false; };
+
+  if (!Config.OnlySection.empty()) {
+    RemovePred = [&Config, RemovePred](const Section &Sec) {
+      return !Config.OnlySection.matches(Sec.CannonicalName);
+    };
+  }
+
+  return Obj.removeSections(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.OnlySection.empty() ||
-      !Config.SymbolsToGlobalize.empty() || !Config.SymbolsToKeep.empty() ||
-      !Config.SymbolsToLocalize.empty() || !Config.SymbolsToWeaken.empty() ||
-      !Config.SymbolsToKeepGlobal.empty() || !Config.SectionsToRename.empty() ||
-      !Config.SymbolsToRename.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.UnneededSymbolsToRemove.empty() ||
       !Config.SetSectionFlags.empty() || !Config.ToRemove.empty() ||
       Config.ExtractDWO || Config.KeepFileSymbols || Config.LocalizeHidden ||
       Config.PreserveDates || 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);
   return Error::success();
 }
 
 Error executeObjcopyOnBinary(const CopyConfig &Config,
                              object::MachOObjectFile &In, Buffer &Out) {
   MachOReader Reader(In);
   std::unique_ptr<Object> O = Reader.create();
   if (!O)

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

 template <typename SectionType>
 Section constructSectionCommon(SectionType Sec) {
   Section S;
   S.Sectname =
       StringRef(Sec.sectname, strnlen(Sec.sectname, sizeof(Sec.sectname)))
           .str();
   S.Segname =
       StringRef(Sec.segname, strnlen(Sec.segname, sizeof(Sec.sectname))).str();
+  S.CannonicalName = (Twine(S.Segname) + "," + S.Sectname).str();
   S.Addr = Sec.addr;
   S.Size = Sec.size;
   S.Offset = Sec.offset;
   S.Align = Sec.align;
   S.RelOff = Sec.reloff;
   S.NReloc = Sec.nreloc;
   S.Flags = Sec.flags;
   S.Reserved1 = Sec.reserved1;

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

struct MachHeader {
   uint32_t Flags;
   uint32_t Reserved = 0;
 };
 
 struct RelocationInfo;
 struct Section {
   std::string Sectname;
   std::string Segname;
+  // CannonicalName is a string formatted as “<Segname>,<Sectname>".
+  std::string CannonicalName;
   uint64_t Addr;
   uint64_t Size;
   uint32_t Offset;
   uint32_t Align;
   uint32_t RelOff;
   uint32_t NReloc;
   uint32_t Flags;
   uint32_t Reserved1;
struct Object {
   /// The index of LC_DYLD_INFO or LC_DYLD_INFO_ONLY load command if present.
   Optional<size_t> DyLdInfoCommandIndex;
   /// The index LC_DYSYMTAB load comamnd if present.
   Optional<size_t> DySymTabCommandIndex;
   /// The index LC_DATA_IN_CODE load comamnd if present.
   Optional<size_t> DataInCodeCommandIndex;
   /// The index LC_FUNCTION_STARTS load comamnd if present.
   Optional<size_t> FunctionStartsCommandIndex;
+
+  void removeSections(function_ref<bool(const Section &)> ToRemove);
 };
 
 } // end namespace macho
 } // end namespace objcopy
 } // end namespace llvm
 
 #endif // LLVM_OBJCOPY_MACHO_OBJECT_H

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

 #include "Object.h"
 #include "../llvm-objcopy.h"
 
 namespace llvm {
 namespace objcopy {
 namespace macho {
 
 const SymbolEntry *SymbolTable::getSymbolByIndex(uint32_t Index) const {
   assert(Index < Symbols.size() && "invalid symbol index");
   return Symbols[Index].get();
 }
 
+void Object::removeSections(function_ref<bool(const Section &)> ToRemove) {
+  for (LoadCommand &LC : LoadCommands)
+    LC.Sections.erase(std::remove_if(std::begin(LC.Sections),
+                                     std::end(LC.Sections), ToRemove),
+                      std::end(LC.Sections));
+}
+
 } // end namespace macho
 } // end namespace objcopy
 } // end namespace llvm

llvm/tools/llvm-objcopy/llvm-objcopy.cpp

 static Error executeObjcopyOnBinary(CopyConfig &Config, object::Binary &In,
                                     Buffer &Out) {
   if (auto *ELFBinary = dyn_cast<object::ELFObjectFileBase>(&In)) {
     if (Error E = Config.parseELFConfig())
       return E;
     return elf::executeObjcopyOnBinary(Config, *ELFBinary, Out);
   } else if (auto *COFFBinary = dyn_cast<object::COFFObjectFile>(&In))
     return coff::executeObjcopyOnBinary(Config, *COFFBinary, Out);
-  else if (auto *MachOBinary = dyn_cast<object::MachOObjectFile>(&In))
+  else if (auto *MachOBinary = dyn_cast<object::MachOObjectFile>(&In)) {
+    if (Error E = Config.parseMachOConfig())
+      return E;
     return macho::executeObjcopyOnBinary(Config, *MachOBinary, Out);
-  else
+  } else
     return createStringError(object_error::invalid_file_type,
                              "unsupported object file format");
 }
 
 static Error executeObjcopyOnArchive(CopyConfig &Config, const Archive &Ar) {
   std::vector<NewArchiveMember> NewArchiveMembers;
   Error Err = Error::success();
   for (const Archive::Child &Child : Ar.children(Err)) {