Create MutableObjectWriter class

[abrachet]

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

1167

I'm not sure if I'm using StringTableBuilder incorrectly. An assertion failed saying that the string was not in the table. I looked into it with lldb and manually added the string it said was not there, it still didn't work.

[jhenderson]

I've made a number of first-pass comments for you.

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

1098–1100

Don't need to add this now, but some TODOs to add are:

1. What to do if the string table index is a removed section.
2. What to do if the e_shstrndx value is either SHN_XINDEX.
3. What to do if the e_shstrndx value is 0.

1104

This shouldn't be an assert. What if the e_shstrndx value points at a non-string table?

1140

This seems like an unnecessary copy here. Why can't it wait until you actually need to write the .shstrtab to the buffer?

1142–1151

This block of code is very similar to the above block. Consider sharing it.

1167

I can't see from the code what is going wrong. Check to make sure that the strings are actually added to the StringTableBuilder when you call add, and take a look at the return values from that. I doubt they'll be much different from what getOffset eventually will return.

Also, setting the section name isn't part of laying out a section, so this feels like it's being done in the wrong palce.

1175

What about segments that are nested in other segments? This code doesn't handle that.

1181

What if a program header covers the ELF header and program header table? This code won't work for that.

1190–1191

I think you've got this alignment in the wrong place. I'm pretty sure it should be before the offset is set...

1201–1203

It would be easier to see what's going on with a traditional for loop here.

1250

What about data within segments not covered by sections? This code doesn't handle that.

[abrachet]

Put option to use writer behind flag. Other large changes

[abrachet]

Use right diff

[abrachet]

Fixed UB bug. Still doesn't write correctly, llvm-readelf says the shstrtab is not null terminated, presumably the other sections are written poorly too.

[jhenderson]

Overall, the design looks pretty good now. I think it is clear to me what is happening. I do think you need to write significantly more tests though. Possibly the way to do it is to add a case to the existing tests for basic copies using the switch, and show that the output is identical. If it isn't, you need to understand why there are differences and whether they are important.

llvm/test/tools/llvm-objcopy/ELF/mutable-object-no-change.test

1 ↗

(On Diff #217235)

coppied -> copied

Copied under what circumstance? Perhaps "is properly copied when using the mutable object interface, and no switches are specified".

3–12 ↗

(On Diff #217235)

It's more normal to put the YAML at the end of the test.

I think your test case needs to test program headers and symbols too, so this needs some of each. Perhaps a separate test case would be appropriate to test segment layout for nested segments.

16 ↗

(On Diff #217235)

Delete the extra blank line.

llvm/tools/llvm-objcopy/CommonOpts.td

87 ↗

(On Diff #217235)

ObjectFile -> Object

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp

818 ↗

(On Diff #217235)

/// -> //

Maybe make this comment more explanatory too.

827 ↗

(On Diff #217235)

I'm hesitant about this not taking an r-value reference or copy, because you std::move it further down the stack. This feels like it might be at risk of surprising a user of the interface later on.

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

2313 ↗

(On Diff #217235)

Probably worth a using for the typename ELFT::Phdr type, maybe even in the class definition.

2332 ↗

(On Diff #217235)

Shouldn't this be Obj.getHeader().e_phnum * sizeof(Phdr)?

2337 ↗

(On Diff #217235)

Maybe a stable_sort would be a good plan here?

2354 ↗

(On Diff #217235)

What if the parent segment hasn't been laid out yet? I believe that you could theoretically have the parent segment after the child segment, in the ordered vector, if the alignment of the parent is greater than an otherwise identical child.

2369 ↗

(On Diff #217235)

Why?

(I know it's the null section, but it's a good idea to be clear about it)

2385 ↗

(On Diff #217235)

Huh? Do you mean to be using sh_addralign here?

2476 ↗

(On Diff #217235)

Nit: delete the blank line

[abrachet]

Addressed review comments

[abrachet]

I didn't get to all comments yet, and I also haven't added more tests because currently the object file isn't being create properly. Once I am making a valid ELF file I will do more rigorous testing.

Right now llvm-readelf complains that that the .shstrtab is not null terminated, so probably a layout issue of sections. I've been looking into it and couldn't find the problem yet. My section offsets are slightly lower than how llvm-objcopy currently does layout. Does anyone have any ideas why that is?

Currently this patch creates sections at offsets 0, 38, 38, 50, 51 and llvm-objcopy does 0, 40, 40, 58, 59 (both with no arguments)

llvm/test/tools/llvm-objcopy/ELF/mutable-object-no-change.test

3–12 ↗

(On Diff #217235)

Oops forgot to do the first part, I'll do that in a later diff. Haven't done second part see my out of line comment.

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp

827 ↗

(On Diff #217235)

There is no copy constructor for ObjectFiles so I could make one for MutableELFObject, I can't move In or *O the compiler says. I don't really know the intricacies of when you can and cannot move an object, probably I could move the OwningObjectFile though, this would require larger changes to this method. Do you have any thoughts?

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

2332 ↗

(On Diff #217235)

Good catch :)

2337 ↗

(On Diff #217235)

I actually just learned about this today in class and was thinking it was just what I needed :)

2354 ↗

(On Diff #217235)

I haven't gotten around to this yet because I have bigger problems that I want to address before potentially adding more bugs.

I can do this the quick way which would be having an std::set<size_t> and whenever *ParentIndex is larger than Index I layout that segment and then add it to the set and check every iteration of the loop if Index is in there. Seems ugly though. I could also a bool laidOut to MutableELFSegment<ELFT> because its ok to make every segment mutable because there are so few of them I believe.

2385 ↗

(On Diff #217235)

Oh yes thanks!

2476 ↗

(On Diff #217235)

Are you sure? There was previously a blank line between template class ELFWriter<ELF32BE>; and the end of namesapce elf. I think it looks better too.

[jhenderson]

In D66403#1650286, @abrachet wrote:

I didn't get to all comments yet, and I also haven't added more tests because currently the object file isn't being create properly. Once I am making a valid ELF file I will do more rigorous testing.

Right now llvm-readelf complains that that the .shstrtab is not null terminated, so probably a layout issue of sections. I've been looking into it and couldn't find the problem yet. My section offsets are slightly lower than how llvm-objcopy currently does layout. Does anyone have any ideas why that is?

No idea myself off the top of my head, but the best thing to do is probably just to manually look at the produced object in a hex viewer, and see what looks wrong (consider section sizes, the section offset, and whether the right section is marked as the section header string table).

Currently this patch creates sections at offsets 0, 38, 38, 50, 51 and llvm-objcopy does 0, 40, 40, 58, 59 (both with no arguments)

I'd try manually laying out the ELF by hand, and see what your calculations come to, then compare them to what happens when you run the tool with a debugger. Where do the offsets start differing, and what causes that? Obvious suggestion might be problems with alignment, but I don't know.

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp

827 ↗

(On Diff #217235)

I don't really think we want a copy, thinking about it more. We probably do want an r-value reference though. I'd change the signatures and call sites as follows, assuming it works (not had the time to verify them myself):

return elf::executeObjcopyWithMutableObject(Config, std::move(*ELFBinary), Out)

Error executeObjcopyWithMutableObject(const CopyConfig &Config,
                                      object::ELFObjectFileBase &&In,
                                      Buffer &Out) {
  ...
  return handleArgsAndWrite(std::move(*O), Out)
}

Error handleArgsAndWrite(ELFObjectFile<ELFT> &&Obj, Buffer &Out)

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

2327–2332 ↗

(On Diff #217775)

Adding the segments like this is a good idea taken from llvm-objcopy's existing handling (even if I do say so myself, since I remember suggesting this originally!), but where do you prevent them actually being written out?

2385 ↗

(On Diff #217775)

clang-format?

2458 ↗

(On Diff #217775)

I might be being blind, but where do you write the program header table?

2354 ↗

(On Diff #217235)

I don't think you need to add a member to MutableELFSegment. You could have a local vector/map of bool though, so you can do IsLaidOut[Index] and just check and update that value at appropriate times.

2369 ↗

(On Diff #217235)

Thinking about it from a strict correctness point of view, I think you just want to skip all sections with type SHT_NULL. From the gABI:

SHT_NULL - This value marks the section header as inactive; it does not have an associated section. Other members of the section header have undefined values.

2476 ↗

(On Diff #217235)

My mistake. I misread the old diff. Please re-add!

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

356–359

Canonical names are Elf_Ehdr, Elf_Phdr etc.

llvm/include/llvm/Object/MutableELFObject.h

public:
   void removeSection(SectionRef Sec) {
     uintptr_t Index = Sec.getRawDataRefImpl().p;
     assert(Sections.find(Index) != Sections.end() &&
            "Removing section not previously in segment");
     Sections.erase(Index);
   }
 
   operator const Elf_Phdr &() const { return Header; }
+
+  std::set<uint64_t>::iterator begin() const { return Sections.begin(); }
+  std::set<uint64_t>::iterator end() const { return Sections.end(); }
 };
 
 template <typename ELFT> class MutableELFObject : public ELFObjectFile<ELFT> {
   /// This class is used for a 'copy on write' effect with tables in an ELF
   /// object file.
   ///
   /// The table holds entries of two different but related types. OrigType
   /// represents the object type in the file originaly while NewType is the
public:
   }
 
   /// Removes a section.
   void removeSection(SectionRef Sec) {
     Sections.remove(Sec.getRawDataRefImpl().p);
   }
 
   Elf_Ehdr &getHeader() { return FileHeader; }
+
+  const Elf_Shdr &getSectionHeader(SectionRef Sec) const {
+    return *getSection(Sec.getRawDataRefImpl());
+  }
+
 };
 
 template <typename ELFT>
 void MutableELFObject<ELFT>::moveSectionNext(DataRefImpl &Sec) const {
   Sec.p = Sections.getNextIndex(Sec.p);
 }
 
 template <typename ELFT>

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

 #include "Buffer.h"
 #include "CopyConfig.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/Twine.h"
 #include "llvm/BinaryFormat/ELF.h"
 #include "llvm/MC/StringTableBuilder.h"
 #include "llvm/Object/ELFObjectFile.h"
+#include "llvm/Object/MutableELFObject.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/FileOutputBuffer.h"
 #include <cstddef>
 #include <cstdint>
 #include <functional>
 #include <memory>
 #include <set>
 #include <vector>
public:
   ELFWriter(Object &Obj, Buffer &Buf, bool WSH);
 };
 
 class BinaryWriter : public Writer {
 private:
   std::unique_ptr<BinarySectionWriter> SecWriter;
 
   uint64_t TotalSize;
 
 public:
   ~BinaryWriter() {}
   Error finalize() override;
   Error write() override;
   BinaryWriter(Object &Obj, Buffer &Buf) : Writer(Obj, Buf) {}
 };
 
 class IHexWriter : public Writer {
   struct SectionCompare {
     bool operator()(const SectionBase *Lhs, const SectionBase *Rhs) const;
   };
Segment &addSegment(ArrayRef<uint8_t> Data) {
     Segments.emplace_back(llvm::make_unique<Segment>(Data));
     return *Segments.back();
   }
   bool isRelocatable() const {
     return (Type != ELF::ET_DYN && Type != ELF::ET_EXEC) || MustBeRelocatable;
   }
 };
 
+using namespace object;
+
+DataRefImpl toDataRef(uint64_t P) {
+  DataRefImpl DRI;
+  DRI.p = P;
+  return DRI;
+}
+
+template <typename ELFT> class MutableObjectWriter {
+  object::MutableELFObject<ELFT> &MutableObject;
+
+  StringTableBuilder ShstrBuilder;
+  object::MutableELFSection<ELFT> *Shstrtab = nullptr;
+  StringTableBuilder SymStrBuilder;
+  object::MutableELFSection<ELFT> *SymStrtab = nullptr;
+
+  size_t CurrentOffset = sizeof(typename ELFT::Ehdr);
+
+  std::set<uint64_t> SectionsInSegments;
+
+  Error init();
+  Error layoutSegmentsAndSections();
+  Error layoutSection(uint64_t Section);
+  Error layoutOtherSections();
+
+public:
+  MutableObjectWriter(object::MutableELFObject<ELFT> &Object)
+      : MutableObject(Object), ShstrBuilder(StringTableBuilder::ELF),
+        SymStrBuilder(StringTableBuilder::ELF) {}
+  ~MutableObjectWriter() {}
+
+  Error finalize();
+  Error write(StringRef Filename);
+};
+
+template <typename ELFT> Error MutableObjectWriter<ELFT>::init() {
+  Expected<MutableELFSection<ELFT> &> ShstrtabOrErr =
+      MutableObject.getMutableSection(SectionRef(
+          toDataRef(MutableObject.getHeader().e_shstrndx), &MutableObject));
+  if (!ShstrtabOrErr)
+    return ShstrtabOrErr.takeError();
+  Shstrtab = &ShstrtabOrErr.get();
+  assert(Shstrtab->Header.sh_type == ELF::SHT_STRTAB);
+
+  section_iterator Sec =
+      llvm::find_if(MutableObject.sections(), [](SectionRef Sec) {
+        return ELFSectionRef(Sec).getType() == ELF::SHT_SYMTAB;
+      });
+
+  if (Sec != MutableObject.section_end()) {
+    uint64_t Link = MutableObject.getSectionHeader(*Sec).sh_link;
+    Expected<MutableELFSection<ELFT> &> SymStrtabOrErr =
+        MutableObject.getMutableSection(
+            SectionRef(toDataRef(Link), &MutableObject));
+    if (!SymStrtabOrErr)
+      return SymStrtabOrErr.takeError();
+    SymStrtab = &SymStrtabOrErr.get();
+  }
+
+  MutableObject.getHeader().e_shnum =
+      std::distance(MutableObject.section_begin(), MutableObject.section_end());
+  MutableObject.getHeader().e_phnum =
+      std::distance(MutableObject.segment_begin(), MutableObject.segment_end());
+  MutableObject.getHeader().e_phoff = CurrentOffset;
+  CurrentOffset +=
+      sizeof(typename ELFT::Phdr) * MutableObject.getHeader().e_phnum;
+  MutableObject.getHeader().e_shoff = CurrentOffset;
+  CurrentOffset +=
+      sizeof(typename ELFT::Shdr) * MutableObject.getHeader().e_shnum;
+
+  for (SectionRef Sec : MutableObject.sections()) {
+    StringRef Name;
+    if (std::error_code EC = Sec.getName(Name))
+      return createStringError(EC, "couldn't get section name");
+    ShstrBuilder.add(Name);
+  }
+  ShstrBuilder.finalize();
+  Shstrtab->Data = OwningArrayRef<uint8_t>(ShstrBuilder.getSize());
+  ShstrBuilder.write(Shstrtab->Data.data());
+
+  for (BasicSymbolRef BSR : MutableObject.symbols()) {
+    ELFSymbolRef Sym(BSR);
+    Expected<StringRef> NameOrErr = Sym.getName();
+    if (!NameOrErr)
+      return NameOrErr.takeError();
+    SymStrBuilder.add(*NameOrErr);
+  }
+  SymStrBuilder.finalize();
+  SymStrtab->Data = OwningArrayRef<uint8_t>(SymStrBuilder.getSize());
+  SymStrBuilder.write(SymStrtab->Data.data());
+
+  return Error::success();
+}
+
+template <typename ELFT>
+Error MutableObjectWriter<ELFT>::layoutSection(uint64_t Section) {
+  Expected<MutableELFSection<ELFT> &> SecOrErr =
+      MutableObject.getMutableSection(
+          SectionRef(toDataRef(Section), &MutableObject));
+  if (!SecOrErr)
+    return SecOrErr.takeError();
+  MutableELFSection<ELFT> &Sec = *SecOrErr;
+  CurrentOffset = alignTo(
+      CurrentOffset, Sec.Header.sh_addralign ? Sec.Header.sh_addralign : 1);
+  Sec.Header.sh_offset = CurrentOffset;
+  Sec.Header.sh_name = 0; // ShstrBuilder.getOffset(StringRef(Sec.Name));
+  if (Sec.Header.sh_type != ELF::SHT_NOBITS)
+    CurrentOffset += Sec.Header.sh_size;
+  return Error::success();
+}
+
+template <typename ELFT>
+Error MutableObjectWriter<ELFT>::layoutSegmentsAndSections() {
+  for (auto Seg : MutableObject.segments()) {
+    Expected<MutableELFSegment<ELFT> &> SegmentOrErr =
+        MutableObject.getMutableSegment(Seg);
+    if (!SegmentOrErr)
+      return SegmentOrErr.takeError();
+    MutableELFSegment<ELFT> &Segment = *SegmentOrErr;
+    Segment.Header.p_offset = CurrentOffset;
+
+    for (uint64_t Section : Segment) {
+      assert(SectionsInSegments.find(Section) == SectionsInSegments.end() &&
+             "Section in multiple segments");
+      SectionsInSegments.insert(Section);
+      if (Error E = layoutSection(Section))
+        return E;
+    }
+    CurrentOffset = alignTo(
+        CurrentOffset, Segment.Header.p_align ? Segment.Header.p_align : 1);
+    Segment.Header.p_filesz = Segment.Header.p_offset - CurrentOffset;
+  }
+
+  return Error::success();
+}
+
+template <typename ELFT>
+Error MutableObjectWriter<ELFT>::layoutOtherSections() {
+  // Don't layout the first section.
+  for (uint64_t Sec :
+       seq<uint64_t>(1, std::distance(MutableObject.section_begin(),
+                                      MutableObject.section_end())))
+    if (SectionsInSegments.find(Sec) == SectionsInSegments.end())
+      if (Error E = layoutSection(Sec))
+        return E;
+
+  return Error::success();
+}
+
+template <typename ELFT> Error MutableObjectWriter<ELFT>::finalize() {
+  if (Error E = init())
+    return E;
+
+  CurrentOffset +=
+      MutableObject.getHeader().e_shnum * sizeof(typename ELFT::Phdr);
+
+  if (Error E = layoutSegmentsAndSections())
+    return E;
+
+  if (Error E = layoutOtherSections())
+    return E;
+
+  return Error::success();
+}
+
+template <typename ELFT>
+Error MutableObjectWriter<ELFT>::write(StringRef Filename) {
+  Expected<std::unique_ptr<FileOutputBuffer>> BufferOrErr =
+      FileOutputBuffer::create(Filename, CurrentOffset,
+                               FileOutputBuffer::F_executable);
+  if (!BufferOrErr)
+    return BufferOrErr.takeError();
+
+  std::unique_ptr<FileOutputBuffer> Buf = std::move(*BufferOrErr);
+  uint8_t *const Start = reinterpret_cast<uint8_t *>(Buf->getBufferStart());
+  uint8_t *Current = Start;
+
+  auto WriteToBuf = [&Current](void *Src, size_t Size) {
+    memcpy(Current, Src, Size);
+    Current += Size;
+  };
+
+  WriteToBuf(&MutableObject.getHeader(), sizeof(typename ELFT::Ehdr));
+
+  for (auto Phdr : MutableObject.segments()) {
+    auto PhdrOrErr = MutableObject.getMutableSegment(Phdr);
+    if (!PhdrOrErr)
+      return PhdrOrErr.takeError();
+    WriteToBuf(&PhdrOrErr->Header, sizeof(typename ELFT::Phdr));
+  }
+
+  for (auto Sec : MutableObject.sections()) {
+    auto SecOrErr = MutableObject.getMutableSection(Sec);
+    if (!SecOrErr)
+      return SecOrErr.takeError();
+    WriteToBuf(&SecOrErr->Header, sizeof(typename ELFT::Shdr));
+    memcpy(Start + SecOrErr->Header.sh_offset, SecOrErr->Data.data(),
+           SecOrErr->Header.sh_size);
+  }
+
+  if (Error E = Buf->commit())
+    return E;
+
+  return Error::success();
+}
+
 } // end namespace elf
 } // end namespace objcopy
 } // end namespace llvm
 
 #endif // LLVM_TOOLS_OBJCOPY_OBJECT_H

llvm/unittests/tools/CMakeLists.txt

 if(LLVM_TARGETS_TO_BUILD MATCHES "X86")
   add_subdirectory(
     llvm-cfi-verify
   )
 endif()
 
 add_subdirectory(
+  llvm-objcopy
   llvm-exegesis
 )
 

llvm/unittests/tools/llvm-objcopy/CMakeLists.txt

+include_directories(
+  ${LLVM_MAIN_SRC_DIR}/tools/llvm-objcopy
+  )
+
+set(LLVM_LINK_COMPONENTS
+  MC
+  Object
+  ObjectYaml
+  Support
+  )
+
+add_llvm_unittest(LLVMObjcopyTests
+  MutableObjectWriterTest.cpp
+  )
+
+target_link_libraries(LLVMObjcopyTests PRIVATE LLVMTestingSupport)

llvm/unittests/tools/llvm-objcopy/MutableObjectWriterTest.cpp

+
+#include "ELF/Object.h"
+#include "llvm/ADT/SmallString.h"
+#include "llvm/Object/MutableELFObject.h"
+#include "llvm/Object/ObjectFile.h"
+#include "llvm/ObjectYAML/yaml2obj.h"
+#include "llvm/Support/Error.h"
+#include "llvm/Support/YAMLTraits.h"
+#include "llvm/Testing/Support/Error.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+using namespace object;
+using namespace objcopy;
+
+TEST(MutableObjectWriter, Test) {
+  SmallString<0> Storage;
+  Expected<std::unique_ptr<ObjectFile>> ErrOrObj =
+      yaml::yaml2ObjectFile(Storage, R"(
+--- !ELF
+FileHeader:
+  Class:     ELFCLASS64
+  Data:      ELFDATA2LSB
+  Type:      ET_REL
+  Machine:   EM_X86_64
+Sections:
+  - Name:    .sec0
+    Type:    SHT_PROGBITS
+  - Name:    .sec1
+    Type:    SHT_PROGBITS
+  - Name:    .sec2
+    Type:    SHT_PROGBITS
+ProgramHeaders:
+  - Type:          PT_LOAD
+    Flags:         [ PF_X, PF_R ]
+    VAddr:         0x1000
+    Sections:
+      - Section:   .sec0
+      - Section:   .sec1
+      - Section:   .sec2)");
+
+  ASSERT_THAT_EXPECTED(ErrOrObj, Succeeded());
+  auto *ELFObjFile = dyn_cast<ELFObjectFile<ELF64LE>>(ErrOrObj->get());
+  ASSERT_TRUE(ELFObjFile);
+  MutableELFObject<ELF64LE> MutableObject(std::move(*ELFObjFile));
+
+  elf::MutableObjectWriter<ELF64LE> Writer(MutableObject);
+  Error E = Writer.finalize();
+  ASSERT_THAT_ERROR(std::move(E), Succeeded());
+  E = Writer.write("test.tmp");
+  ASSERT_THAT_ERROR(std::move(E), Succeeded());
+}