[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.

106

Nit: no new line at EOF.

[rupprecht]

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

68–76

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

80

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.

96

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

44–46

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

80

Oops, I didn't noticed that. Thanks!

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

44–46

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

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

38–42

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

53

static

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

+## Show that if llvm-objcopy adds a new section into the object if
+## --add-section is given.
+
+# RUN: yaml2obj %s > %t
+# RUN: echo -n abcdefg > %t.data
+
+# RUN: not llvm-objcopy --add-section __text=%t.data %t %t.invalid-section-name 2>&1 \
+# RUN:   | FileCheck %s -DINPUT=%t --check-prefix=INVALID-SECTION-NAME
+# INVALID-SECTION-NAME: error: '[[INPUT]]': invalid section name (should be formatted as '<segment name>,<section name>')
+
+## 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 an 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
+
+--- !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: 1
+# CASE1-NEXT:   Name: __TEXT,__text (5F 5F 54 45 58 54 2C 5F 5F 74 65 78 74 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: __FOO,__bar (5F 5F 46 4F 4F 2C 5F 5F 62 61 72 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)
+# COMMON-NEXT:  Address: 0x0
+# COMMON-NEXT:  Size: 0x7
+# 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 (
+# COMMON-NEXT:    0000: 61626364 656667                      |abcdefg|
+# COMMON-NEXT:  )
+
+## Make sure that new segment (__FOO) is created.
+# 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 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, SecName);
+  Sec.setOwnedContentData(Content);
+
+  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.
+  // TODO: 32-bit
+  ErrorOr<LoadCommand &> NewSegment = Obj.addSegment64(TargetSegName);
+  if (!NewSegment)
+    return createFileError(Filename, errorCodeToError(BufOrErr.getError()));
+  NewSegment->Sections.push_back(Sec);
+  return Error::success();
+}
+
 static Error validateOptions(const CopyConfig &Config) {
   // TODO: Support section renaming in GNU objcopy for compatibility (see
   // http://lists.llvm.org/pipermail/llvm-dev/2019-May/132570.html).
 
   if (!Config.OnlySection.empty()) {
     for (const NameOrRegex &NR : Config.OnlySection)
       if (Error E = NR.isMachOCannonicalName())
         return E;
   }
 
   if (!Config.ToRemove.empty()) {
     for (const NameOrRegex &NR : Config.ToRemove)
       if (Error E = NR.isMachOCannonicalName())
         return E;
   }
 
   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.DumpSection.empty() || !Config.KeepSection.empty() ||
-      !Config.SymbolsToGlobalize.empty() || !Config.SymbolsToKeep.empty() ||
-      !Config.SymbolsToLocalize.empty() || !Config.SymbolsToWeaken.empty() ||
-      !Config.SymbolsToKeepGlobal.empty() || !Config.SectionsToRename.empty() ||
-      !Config.SymbolsToRename.empty() ||
+      !Config.AllocSectionsPrefix.empty() || !Config.DumpSection.empty() ||
+      !Config.KeepSection.empty() || !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.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");
   }
 
   if (auto E = validateOptions(Config))
     return E;
 
   removeSections(Config, Obj);
   markSymbols(Config, Obj);
   removeSymbols(Config, Obj);
 
+  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;
+  }
+
   if (Config.StripAll)
     for (LoadCommand &LC : Obj.LoadCommands)
       for (Section &Sec : LC.Sections)
         Sec.Relocations.clear();
 
   return Error::success();
 }
 

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

void MachOReader::readHeader(Object &O) const {
   O.Header.FileType = MachOObj.getHeader().filetype;
   O.Header.NCmds = MachOObj.getHeader().ncmds;
   O.Header.SizeOfCmds = MachOObj.getHeader().sizeofcmds;
   O.Header.Flags = MachOObj.getHeader().flags;
 }
 
 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();
+  StringRef Sectname(Sec.sectname, strnlen(Sec.sectname, sizeof(Sec.sectname)));
+  StringRef Segname(Sec.segname, strnlen(Sec.segname, sizeof(Sec.sectname)));
+  Section S(Segname, Sectname);
   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/MachOWriter.cpp

 
 void MachOWriter::writeSections() {
   for (const auto &LC : O.LoadCommands)
     for (const auto &Sec : LC.Sections) {
       if (Sec.isVirtualSection())
         continue;
 
       assert(Sec.Offset && "Section offset can not be zero");
-      assert((Sec.Size == Sec.Content.size()) && "Incorrect section size");
-      memcpy(B.getBufferStart() + Sec.Offset, Sec.Content.data(),
-             Sec.Content.size());
+      assert((Sec.Size == Sec.getContent().size()) && "Incorrect section size");
+      memcpy(B.getBufferStart() + Sec.Offset, Sec.getContent().data(),
+             Sec.getContent().size());
       for (size_t Index = 0; Index < Sec.Relocations.size(); ++Index) {
         auto RelocInfo = Sec.Relocations[Index];
         if (!RelocInfo.Scattered) {
           auto *Info =
               reinterpret_cast<MachO::relocation_info *>(&RelocInfo.Info);
           Info->r_symbolnum = RelocInfo.Symbol->Index;
         }
 

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

struct MachHeader {
   uint32_t FileType;
   uint32_t NCmds;
   uint32_t SizeOfCmds;
   uint32_t Flags;
   uint32_t Reserved = 0;
 };
 
 struct RelocationInfo;
-struct Section {
+class Section {
+private:
+  std::vector<uint8_t> OwnedContentData;
+
+public:
   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;
-  uint32_t Reserved2;
-  uint32_t Reserved3;
-
+  uint64_t Addr = 0;
+  uint64_t Size = 0;
+  uint32_t Offset = 0;
+  uint32_t Align = 0;
+  uint32_t RelOff = 0;
+  uint32_t NReloc = 0;
+  uint32_t Flags = 0;
+  uint32_t Reserved1 = 0;
+  uint32_t Reserved2 = 0;
+  uint32_t Reserved3 = 0;
   StringRef Content;
+
   std::vector<RelocationInfo> Relocations;
 
+  Section(StringRef SegName, StringRef SectName)
+      : Sectname(SectName), Segname(SegName),
+        CannonicalName((Twine(SegName) + Twine(',') + SectName).str()) {}
+
+  Section(StringRef SegName, StringRef SectName, StringRef Content)
+      : Sectname(SectName), Segname(SegName),
+        CannonicalName((Twine(SegName) + Twine(',') + SectName).str()),
+        Content(Content) {}
+
+  void setOwnedContentData(ArrayRef<uint8_t> Data) {
+    OwnedContentData.assign(Data.begin(), Data.end());
+    Content = StringRef(reinterpret_cast<const char *>(OwnedContentData.data()),
+                        OwnedContentData.size());
+  }
+
+  StringRef getContent() const {
+    if (OwnedContentData.size() > 0)
+      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 {
     return (getType() == MachO::S_ZEROFILL ||
             getType() == MachO::S_GB_ZEROFILL ||
             getType() == MachO::S_THREAD_LOCAL_ZEROFILL);
struct LoadCommand {
   // copied-over without digging into its structure.
   ArrayRef<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 {
   /// 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);
+
+  /// Creates a new segment_command_64 in the object and returns a reference
+  /// to the newly created load command.
+  LoadCommand &addSegment64(StringRef SegName);
 };
 
 } // 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"
+#include "llvm/Support/Errc.h"
+#include "llvm/Support/ErrorOr.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 SymbolTable::removeSymbols(
     function_ref<bool(const std::unique_ptr<SymbolEntry> &)> ToRemove) {
   Symbols.erase(
       std::remove_if(std::begin(Symbols), std::end(Symbols), ToRemove),
       std::end(Symbols));
 }
 
 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));
 }
 
+LoadCommand &Object::addSegment64(StringRef SegName) {
+  LoadCommand LC;
+  MachO::segment_command_64 &Seg = LC.MachOLoadCommand.segment_command_64_data;
+  memset(&Seg, 0, sizeof(MachO::segment_command_64));
+  Seg.cmd = MachO::LC_SEGMENT_64;
+  assert(SegName.size() <= sizeof(Seg.segname) && "too long segment name");
+  strncpy(Seg.segname, SegName.data(), SegName.size());
+  LoadCommands.push_back(LC);
+  return LoadCommands.back();
+}
+
+Optional<StringRef> LoadCommand::getSegmentName() const {
+  const MachO::macho_load_command &MLC = MachOLoadCommand;
+  switch (MLC.load_command_data.cmd) {
+  case MachO::LC_SEGMENT:
+    return StringRef(MLC.segment_command_data.segname,
+                     strnlen(MLC.segment_command_data.segname,
+                             sizeof(MLC.segment_command_data.segname)));
+    break;
+  case MachO::LC_SEGMENT_64:
+    return StringRef(MLC.segment_command_64_data.segname,
+                     strnlen(MLC.segment_command_64_data.segname,
+                             sizeof(MLC.segment_command_64_data.segname)));
+    break;
+  default:
+    return None;
+  }
+}
+
 } // end namespace macho
 } // end namespace objcopy
 } // end namespace llvm