[llvm-objcopy][MachO][NFC] Allow section to own its contents

[seiya]

Summary

This patch allows section to have owned section contents instead of a reference. This feature is necessary when we want to create a new section in the object (e.g., --add-section).

I'll commit this patch along with D66283 since this patch does not include test cases for newly added code.

[jhenderson]

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

42

Since you're storing the owned content as a std::vector<uint8_t>, it may make more sense for Content to be an ArrayRef rather than a StringRef.

[seiya]

Addressed review comments.

[alexander-shaposhnikov]

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

43

frankly speaking i am somehow not sure this is the best possible solution.
Having these two fields + sort of duplication of logic / code size increase ..

Maybe we can consider some alternatives ?

The first thing which comes on my mind -
use StringSaver NewSectionsContents
(for example, it can be a field of Object)
and keep using ArrayRef / StringRef inside Section.
But there are other options as well.

What do you think ? (maybe I'm missing something)

[jhenderson]

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

43

A StringSaver sounds like an interesting idea. I think it's worth exploring.

[seiya]

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

43

Sounds good to me too. I'll try it out.

[seiya]

Merged into D66283: I think it does not have to be a separate patch because using StringSaver simplified what I wanted to do in this patch.

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

for (auto &LC : O.LoadCommands) {
     for (auto &Sec : LC.Sections) {
       if (IsObjectFile) {
         if (Sec.isVirtualSection()) {
           Sec.Offset = 0;
         } else {
           uint64_t PaddingSize =
               offsetToAlignment(SegFileSize, Align(1ull << Sec.Align));
           Sec.Offset = SegOffset + SegFileSize + PaddingSize;
-          Sec.Size = Sec.Content.size();
+          Sec.Size = Sec.getContents().size();
           SegFileSize += PaddingSize + Sec.Size;
         }
         VMSize = std::max(VMSize, Sec.Addr + Sec.Size);
       } else {
         if (Sec.isVirtualSection()) {
           Sec.Offset = 0;
           VMSize += Sec.Size;
         } else {
           uint32_t SectOffset = Sec.Addr - SegmentVmAddr;
           Sec.Offset = SegOffset + SectOffset;
-          Sec.Size = Sec.Content.size();
+          Sec.Size = Sec.getContents().size();
           SegFileSize = std::max(SegFileSize, SectOffset + Sec.Size);
           VMSize = std::max(VMSize, SegFileSize);
         }
       }
     }
 
     if (IsObjectFile) {
       Offset += SegFileSize;

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

 }
 
 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.Content.size());
+            FileOutputBuffer::create(Filename, Sec.getContents().size());
         if (!BufferOrErr)
           return BufferOrErr.takeError();
         std::unique_ptr<FileOutputBuffer> Buf = std::move(*BufferOrErr);
-        llvm::copy(Sec.Content, Buf->getBufferStart());
+        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",

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();
+  StringRef Segname(Sec.segname, strnlen(Sec.segname, sizeof(Sec.segname)));
+  StringRef Sectname(Sec.sectname, strnlen(Sec.sectname, sizeof(Sec.sectname)));
+  Section S(Segname, Sectname);
   S.CanonicalName = (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;
for (; reinterpret_cast<const void *>(Curr) < End; Curr++) {
 
     Expected<object::SectionRef> SecRef =
         MachOObj.getSection(NextSectionIndex++);
     if (!SecRef)
       reportError(MachOObj.getFileName(), SecRef.takeError());
 
     if (Expected<ArrayRef<uint8_t>> E =
             MachOObj.getSectionContents(SecRef->getRawDataRefImpl()))
-      S.Content =
-          StringRef(reinterpret_cast<const char *>(E->data()), E->size());
+      S.setContentsRef(*E);
     else
       reportError(MachOObj.getFileName(), E.takeError());
 
     S.Relocations.reserve(S.NReloc);
     for (auto RI = MachOObj.section_rel_begin(SecRef->getRawDataRefImpl()),
               RE = MachOObj.section_rel_end(SecRef->getRawDataRefImpl());
          RI != RE; ++RI) {
       RelocationInfo R;

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.getContents().size()) && "Incorrect section size");
+      memcpy(B.getBufferStart() + Sec.Offset, Sec.getContents().data(),
+             Sec.getContents().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

 //===- Object.h - Mach-O object file model ----------------------*- 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_OBJCOPY_MACHO_OBJECT_H
 #define LLVM_OBJCOPY_MACHO_OBJECT_H
 
+#include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/BinaryFormat/MachO.h"
 #include "llvm/MC/StringTableBuilder.h"
 #include "llvm/ObjectYAML/DWARFYAML.h"
 #include "llvm/Support/YAMLTraits.h"
 #include <cstdint>
 #include <string>
struct MachHeader {
   uint32_t NCmds;
   uint32_t SizeOfCmds;
   uint32_t Flags;
   uint32_t Reserved = 0;
 };
 
 struct RelocationInfo;
 struct Section {
+private:
+  Optional<std::vector<uint8_t>> OwnedContents;
+  ArrayRef<uint8_t> ContentsRef;
+
+public:
   std::string Sectname;
   std::string Segname;
   // CanonicalName is a string formatted as “<Segname>,<Sectname>".
   std::string CanonicalName;
-  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),
+        CanonicalName((Twine(SegName) + Twine(',') + SectName).str()) {}
+
+  Section(StringRef SegName, StringRef SectName, ArrayRef<uint8_t> Content)
+      : ContentsRef(Content), Sectname(SectName), Segname(SegName),
+        CanonicalName((Twine(SegName) + Twine(',') + SectName).str()) {}
+
+  void setContentsRef(ArrayRef<uint8_t> Data) { ContentsRef = Data; }
+
+  void setOwnedContents(std::vector<uint8_t> &&Data) {
+    OwnedContents = std::move(Data);
+  }
+
+  ArrayRef<uint8_t> getContents() const {
+    if (OwnedContents)
+      return *OwnedContents;
+    return ContentsRef;
+  }
+
   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);