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

[seiya]

• Removed trailing whitespaces.

[jhenderson]

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

2

Remove the "if" before llvm-objcopy

8

You also need a test case for what happens if %t.data doesn't exist.

124

Nit: no new line at EOF.

[rupprecht]

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

75–83

This boilerplate should be handled in CopyConfig instead of copied in every platform-specific implementation, can you add a FIXME to consolidate it?

87

SecName is actually the pair of <segment>,<section>, so shouldn't SecName be replaced with Pair.second? I think the test case you have shows this.

103

If the segment can't be created, it seems like it's not at all related to the file content, so either the filename here should be the file we're trying to add a segment to, or it should be a regular StringError.

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

47–49

Is there a utility you could extract to convert from possibly-null-delimited segment/section names to StringRefs? This pattern is used above with the strncpy and also in MachOReader

[seiya]

• Addressed review comments.

[seiya]

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

87

Oops, I didn't noticed that. Thanks!

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

47–49

Added extractSegmentName for this. I didn't updated strncpy above because it's for filling the segment name, not extracting a possibly-null-terminated string.

[seiya]

Refactored a bit.

[seiya]

• Added a newline at the end of file.

[jhenderson]

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

16

No -> {{[Nn]}}o

The error message is platform-dependent. On some platforms it's "No" and on others it's "no".

[seiya]

Fixed a review comment.

[seiya]

Update CommandGuide.

[jhenderson]

No more comments from me, thanks!

[seiya]

CommandGuide: Removed remove "the" before "<section>".

[rupprecht]

Don't think I have any comments besides these two, so LGTM once they're addressed & you get a MachO reviewer to be happy w/ this.

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

33 ↗

(On Diff #216130)

sizeof(Sec.sectname) should be sizeof(Sec.segname) here?
(They're the same size, fortunately)

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

76–81

A common use of GNU objcopy is to run with --add-section .note.GNU-stack=/dev/null, which I imagine corresponds to a 0-sized section. I think you might want to wrap OwnedContentData in Optional<>. (Assuming a zero-sized section is valid in Mach-O). Additionally, can you add a test for --add-section w/ /dev/null?

The ELF code doesn't use Optional because it takes a different approach by having subclasses for different types of sections, so OwnedDataSection is different from Section -- I don't know if that makes sense here

[seiya]

Addressed review comments and isolate some changes into D71331.

[seiya]

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

76–81

Replaced with Optional and added a test case for /dev/null.

[seiya]

Added a newline at EOF.

[jhenderson]

There seems to be some stuff to do with 64-bit versus normal segments in here, which I don't fully follow due to it being Mach-O, but your test only seems to handle the 64-bit segment type. Should it be extended?

[seiya]

Added 32-bit object support.

[seiya]

In D66283#1779295, @jhenderson wrote:

There seems to be some stuff to do with 64-bit versus normal segments in here, which I don't fully follow due to it being Mach-O, but your test only seems to handle the 64-bit segment type. Should it be extended?

I forgot to implement 32-bit support. The latest patch includes 32-bit support and tests for it.

[jhenderson]

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

5

It probably makes sense to call %t %t.64bit.

5–6

By the way, there seems to be a degree of consensus that -o should be preferred to a shell redirection with yaml2obj. No need to modify old tests, but if you're adding new ones/updating them, it's probably good to make this change too.

20

%t.out1 -> %t.out1.64bit or something along those lines

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

45–49

This and the above block are very similar. You might want to consider pulling them into some kind of function to reduce the duplication.

[seiya]

• Merge D71331
• Use StringSaver as suggested in D71331#1780934
• Address review comments

[jhenderson]

LGTM, from a style and testing point of view, but a Mach-O expert should confirm they are happy too.

[alexander-shaposhnikov]

I've added one minor comment, but otherwise this looks good to me, thanks

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

60

static

llvm/docs/CommandGuide/llvm-objcopy.rst

 
 .. option:: --add-section <section=file>
 
  Add a section named ``<section>`` with the contents of ``<file>`` to the
  output. For ELF objects the section will be of type `SHT_NOTE`, if the name
  starts with ".note". Otherwise, it will have type `SHT_PROGBITS`. Can be
  specified multiple times to add multiple sections.
 
+ For MachO objects, ``<section>`` must be formatted as
+ ``<segment name>,<section name>``.
+
 .. option:: --binary-architecture <arch>, -B
 
  Ignored for compatibility.
 
 .. option:: --disable-deterministic-archives, -U
 
  Use real values for UIDs, GIDs and timestamps when updating archive member
  headers.

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

+## Show that llvm-objcopy adds a new section into the object if
+## --add-section is given.
+
+# RUN: yaml2obj %s > %t
+# RUN: echo -n abcdefg > %t.data
+
+## Error case 1: Nonexistent input file is specified by --add-section.
+# RUN: not llvm-objcopy --add-section __TEXT,__text=%t.missing %t %t.nonexistent-file 2>&1 \
+# RUN:   | FileCheck %s -DINPUT=%t -DSECTION_DATA_FILE=%t.missing --check-prefix=NONEXSITENT-FILE
+# NONEXSITENT-FILE: error: '[[INPUT]]': '[[SECTION_DATA_FILE]]': {{[Nn]}}o such file or directory
+
+## Error case 2: Too long segment name.
+# RUN: not llvm-objcopy --add-section __TOOOOOOOOO_LONG,__text=%t.data %t %t.too-long-seg-name 2>&1 \
+# RUN:   | FileCheck %s -DINPUT=%t --check-prefix=TOO-LONG-SEG-NAME
+# TOO-LONG-SEG-NAME: error: '[[INPUT]]': too long segment name: '__TOOOOOOOOO_LONG'
+
+## Case 1: Add a new section into an existing segment.
+# RUN: llvm-objcopy --add-section __TEXT,__text=%t.data %t %t.out1
+# RUN: llvm-readobj --sections --section-data %t.out1 \
+# RUN:   | FileCheck %s --check-prefixes=COMMON,CASE1
+
+## Case 2: Add a new section into a nonexistent segment.
+# RUN: llvm-objcopy --add-section __FOO,__bar=%t.data %t %t.out2
+# RUN: llvm-readobj --sections --section-data --macho-segment %t.out2 \
+# RUN:   | FileCheck %s --check-prefixes=COMMON,CASE2
+
+## Case 3: Add a new section with /dev/null.
+# RUN: llvm-objcopy --add-section __TEXT,__text=/dev/null %t %t.devnull
+# RUN: llvm-readobj --sections --section-data %t.devnull \
+# RUN:   | FileCheck %s --check-prefixes=COMMON,DEVNULL
+
+--- !mach-o
+FileHeader:
+  magic:           0xFEEDFACF
+  cputype:         0x01000007
+  cpusubtype:      0x00000003
+  filetype:        0x00000001
+  ncmds:           1
+  sizeofcmds:      152
+  flags:           0x00002000
+  reserved:        0x00000000
+LoadCommands:
+  - cmd:             LC_SEGMENT_64
+    cmdsize:         152
+    segname:         ''
+    vmaddr:          0
+    vmsize:          4
+    fileoff:         184
+    filesize:        4
+    maxprot:         7
+    initprot:        7
+    nsects:          1
+    flags:           0
+    Sections:
+      - sectname:        __text
+        segname:         __TEXT
+        addr:            0x0000000000000000
+        content:         'AABBCCDD'
+        size:            4
+        offset:          184
+        align:           0
+        reloff:          0x00000000
+        nreloc:          0
+        flags:           0x80000400
+        reserved1:       0x00000000
+        reserved2:       0x00000000
+        reserved3:       0x00000000
+
+# COMMON:       Index: 0
+# COMMON-NEXT:  __text (5F 5F 74 65 78 74 00 00 00 00 00 00 00 00 00 00)
+# COMMON-NEXT:  Segment: __TEXT (5F 5F 54 45 58 54 00 00 00 00 00 00 00 00 00 00)
+# COMMON:       Index: 1
+# CASE1-NEXT:   __text (5F 5F 74 65 78 74 00 00 00 00 00 00 00 00 00 00)
+# CASE1-NEXT:   Segment: __TEXT (5F 5F 54 45 58 54 00 00 00 00 00 00 00 00 00 00)
+# CASE2-NEXT:   Name: __bar (5F 5F 62 61 72 00 00 00 00 00 00 00 00 00 00 00)
+# CASE2-NEXT:   Segment: __FOO (5F 5F 46 4F 4F 00 00 00 00 00 00 00 00 00 00 00)
+# DEVNULL-NEXT: __text (5F 5F 74 65 78 74 00 00 00 00 00 00 00 00 00 00)
+# DEVNULL-NEXT: Segment: __TEXT (5F 5F 54 45 58 54 00 00 00 00 00 00 00 00 00 00)
+# COMMON-NEXT:  Address: 0x0
+# CASE1-NEXT:   Size: 0x7
+# CASE2-NEXT:   Size: 0x7
+# DEVNULL-NEXT: Size: 0x0
+# COMMON-NEXT:  Offset: 340
+# COMMON-NEXT:  Alignment: 0
+# COMMON-NEXT:  RelocationOffset: 0x0
+# COMMON-NEXT:  RelocationCount: 0
+# COMMON-NEXT:  Type: Regular (0x0)
+# COMMON-NEXT:  Attributes [ (0x0)
+# COMMON-NEXT:  ]
+# COMMON-NEXT:  Reserved1: 0x0
+# COMMON-NEXT:  Reserved2: 0x0
+# COMMON-NEXT:  Reserved3: 0x0
+# COMMON-NEXT:  SectionData (
+# CASE1-NEXT:    0000: 61626364 656667                      |abcdefg|
+# CASE2-NEXT:    0000: 61626364 656667                      |abcdefg|
+# COMMON-NEXT:  )
+
+# CASE2:      Segment {
+# CASE2-NEXT:   Cmd: LC_SEGMENT_64
+# CASE2-NEXT:   Name:
+# CASE2-NEXT:   Size: 152
+# CASE2-NEXT:   vmaddr: 0x0
+# CASE2-NEXT:   vmsize: 0x4
+# CASE2-NEXT:   fileoff: 336
+# CASE2-NEXT:   filesize: 4
+# CASE2-NEXT:   maxprot: rwx
+# CASE2-NEXT:   initprot: rwx
+# CASE2-NEXT:   nsects: 1
+# CASE2-NEXT:   flags: 0x0
+# CASE2-NEXT: }
+# CASE2-NEXT: Segment {
+# CASE2-NEXT:   Cmd: LC_SEGMENT_64
+# CASE2-NEXT:   Name: __FOO
+# CASE2-NEXT:   Size: 152
+# CASE2-NEXT:   vmaddr: 0x0
+# CASE2-NEXT:   vmsize: 0x7
+# CASE2-NEXT:   fileoff: 340
+# CASE2-NEXT:   filesize: 7
+# CASE2-NEXT:   maxprot: ---
+# CASE2-NEXT:   initprot: ---
+# CASE2-NEXT:   nsects: 1
+# CASE2-NEXT:   flags: 0x0
+# CASE2-NEXT: }
 No newline at end of file

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

auto RemovePred = [Config](const std::unique_ptr<SymbolEntry> &N) {
     if (N->Referenced)
       return false;
     return Config.StripAll;
   };
 
   Obj.SymTable.removeSymbols(RemovePred);
 }
 
 static LoadCommand buildRPathLoadCommand(StringRef Path) {
   LoadCommand LC;
   MachO::rpath_command RPathLC;
   RPathLC.cmd = MachO::LC_RPATH;
   RPathLC.path = sizeof(MachO::rpath_command);
   RPathLC.cmdsize = alignTo(sizeof(MachO::rpath_command) + Path.size(), 8);
   LC.MachOLoadCommand.rpath_command_data = RPathLC;
   LC.Payload.assign(RPathLC.cmdsize - sizeof(MachO::rpath_command), 0);
   std::copy(Path.begin(), Path.end(), LC.Payload.begin());
   return LC;
 }
 
 static Error dumpSectionToFile(StringRef SecName, StringRef Filename,
                                Object &Obj) {
   for (LoadCommand &LC : Obj.LoadCommands)
     for (Section &Sec : LC.Sections) {
       if (Sec.CanonicalName == SecName) {
         Expected<std::unique_ptr<FileOutputBuffer>> BufferOrErr =
             FileOutputBuffer::create(Filename, Sec.getContents().size());
         if (!BufferOrErr)
           return BufferOrErr.takeError();
         std::unique_ptr<FileOutputBuffer> Buf = std::move(*BufferOrErr);
         llvm::copy(Sec.getContents(), Buf->getBufferStart());
 
         if (Error E = Buf->commit())
           return E;
         return Error::success();
       }
     }
 
   return createStringError(object_error::parse_failed, "section '%s' not found",
                            SecName.str().c_str());
 }
 
+static Error addSection(StringRef SecName, StringRef Filename, Object &Obj) {
+  ErrorOr<std::unique_ptr<MemoryBuffer>> BufOrErr =
+      MemoryBuffer::getFile(Filename);
+  if (!BufOrErr)
+    return createFileError(Filename, errorCodeToError(BufOrErr.getError()));
+  std::unique_ptr<MemoryBuffer> Buf = std::move(*BufOrErr);
+  ArrayRef<uint8_t> Content(
+      reinterpret_cast<const uint8_t *>(Buf->getBufferStart()),
+      Buf->getBufferSize());
+
+  std::pair<StringRef, StringRef> Pair = SecName.split(',');
+  StringRef TargetSegName = Pair.first;
+  Section Sec(TargetSegName, Pair.second);
+  Sec.setOwnedContents(Content);
+
+  // Add the a section into an existing segment.
+  for (LoadCommand &LC : Obj.LoadCommands) {
+    Optional<StringRef> SegName = LC.getSegmentName();
+    if (SegName && SegName == TargetSegName) {
+      LC.Sections.push_back(Sec);
+      return Error::success();
+    }
+  }
+
+  // There's no segment named TargetSegName. Create a new load command and
+  // Insert a new section into it.
+  LoadCommand &NewSegment = Obj.addSegment(TargetSegName);
+  NewSegment.Sections.push_back(Sec);
+  return Error::success();
+}
+
+// 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();
+}
+
 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.KeepSection.empty() || Config.NewSymbolVisibility ||
-      !Config.SymbolsToGlobalize.empty() || !Config.SymbolsToKeep.empty() ||
-      !Config.SymbolsToLocalize.empty() || !Config.SymbolsToWeaken.empty() ||
-      !Config.SymbolsToKeepGlobal.empty() || !Config.SectionsToRename.empty() ||
+      !Config.AllocSectionsPrefix.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.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() ||
static Error handleArgs(const CopyConfig &Config, Object &Obj) {
   for (const StringRef &Flag : Config.DumpSection) {
     std::pair<StringRef, StringRef> SecPair = Flag.split("=");
     StringRef SecName = SecPair.first;
     StringRef File = SecPair.second;
     if (Error E = dumpSectionToFile(SecName, File, Obj))
       return E;
   }
 
+  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;
+  }
+
   for (StringRef RPath : Config.RPathToAdd) {
     for (LoadCommand &LC : Obj.LoadCommands) {
       if (LC.MachOLoadCommand.load_command_data.cmd == MachO::LC_RPATH &&
           RPath == StringRef(reinterpret_cast<char *>(LC.Payload.data()),
                              LC.Payload.size())
                        .trim(0)) {
         return createStringError(errc::invalid_argument,
                                  "rpath " + RPath +

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

public:
 
   void setContentsRef(StringRef Data) { Content = Data; }
 
   void setOwnedContents(ArrayRef<uint8_t> Data) {
     OwnedContentData = Data.vec();
     Content =
         StringRef(reinterpret_cast<const char *>(OwnedContentData->data()),
                   OwnedContentData->size());
   }
 
   StringRef getContents() const {
     if (OwnedContentData)
       return StringRef(reinterpret_cast<const char *>(OwnedContentData->data()),
                        OwnedContentData->size());
     return Content;
   }
 
   MachO::SectionType getType() const {
     return static_cast<MachO::SectionType>(Flags & MachO::SECTION_TYPE);
   }
 
   bool isVirtualSection() const {
struct LoadCommand {
   // copied-over without digging into its structure.
   std::vector<uint8_t> Payload; 
 
   // Some load commands can contain (inside the payload) an array of sections,
   // though the contents of the sections are stored separately. The struct
   // Section describes only sections' metadata and where to find the
   // corresponding content inside the binary.
   std::vector<Section> Sections;
+
+  // Returns the segment name if the load command is a segment command.
+  Optional<StringRef> getSegmentName() const;
 };
 
 // A symbol information. Fields which starts with "n_" are same as them in the
 // nlist.
 struct SymbolEntry {
   std::string Name;
   bool Referenced = false;
   uint32_t Index;
struct Object {
   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);
   void addLoadCommand(LoadCommand LC);
+
+  /// Creates a new segment load command in the object and returns a reference
+  /// to the newly created load command. The caller should verify that SegName
+  /// is not too long (SegName.size() should be less than or equal to 16).
+  LoadCommand &addSegment(StringRef SegName);
+
+  bool is64Bit() const {
+    return Header.Magic == MachO::MH_MAGIC_64 ||
+           Header.Magic == MachO::MH_CIGAM_64;
+  }
 };
 
 } // end namespace macho
 } // end namespace objcopy
 } // end namespace llvm
 
 #endif // LLVM_OBJCOPY_MACHO_OBJECT_H

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

LC.Sections.erase(std::remove_if(std::begin(LC.Sections),
                                      std::end(LC.Sections), ToRemove),
                       std::end(LC.Sections));
 }
 
 void Object::addLoadCommand(LoadCommand LC) {
   LoadCommands.push_back(std::move(LC));
 }
 
+LoadCommand &Object::addSegment(StringRef SegName) {
+  assert(is64Bit() && "adding segment_command is not yet supported");
+
+  LoadCommand LC;
+  MachO::segment_command_64 &Seg = LC.MachOLoadCommand.segment_command_64_data;
+
+  assert(SegName.size() <= sizeof(Seg.segname) && "too long segment name");
+
+  memset(&Seg, 0, sizeof(MachO::segment_command_64));
+  Seg.cmd = MachO::LC_SEGMENT_64;
+  strncpy(Seg.segname, SegName.data(), SegName.size());
+  LoadCommands.push_back(LC);
+
+  return LoadCommands.back();
+}
+
+/// Extracts a segment name from a string which is possibly non-null-terminated.
+StringRef extractSegmentName(const char *SegName) {
+  return StringRef(SegName,
+                   strnlen(SegName, sizeof(MachO::segment_command::segname)));
+}
+
+Optional<StringRef> LoadCommand::getSegmentName() const {
+  const MachO::macho_load_command &MLC = MachOLoadCommand;
+  switch (MLC.load_command_data.cmd) {
+  case MachO::LC_SEGMENT:
+    return extractSegmentName(MLC.segment_command_data.segname);
+  case MachO::LC_SEGMENT_64:
+    return extractSegmentName(MLC.segment_command_64_data.segname);
+  default:
+    return None;
+  }
+}
+
 } // end namespace macho
 } // end namespace objcopy
 } // end namespace llvm