[Object] Create MutableELFObject Class for Doing Mutations on ELFObjectFiles [Part 4]

[abrachet]

Summary

Added addSection and addSymbol methods

[jhenderson]

I think some other interesting test cases are a) adding more than one symbol/section, b) adding and then removing a symbol/section, c) removing stuff and then adding stuff, showing that the iteration still works correctly.

llvm/include/llvm/Object/MutableELFObject.h

173

My instinct is that this should be a void return type.

352

I think this should just be a void return type.

357

I think this should just be a void return type.

414–419

This seems like an unrelated change?

llvm/unittests/Object/MutableELFObjectTest.cpp

523–525

Comment why there's 4 sections already.

544–546

Comment why there's already a symbol here.

548

It's not a header. Symbols are just that - symbols (unlike sections which have a header and actual contents). So this should just be called "Sym" or something like that.

[abrachet]

Addressed review comments

[abrachet]

llvm/include/llvm/Object/MutableELFObject.h

173

Hmm, I'm not sure about this and the other comments. I think it is more convenient this way, but also I wonder when you would add something and then immediately change something. The cost of this return is very small though I would say.

I don't have a strong opinion either way.

[jhenderson]

llvm/include/llvm/Object/MutableELFObject.h

173

My thinking is that add is akin to vector's push_back method, which doesn't return anything.

llvm/unittests/Object/MutableELFObjectTest.cpp

523

"yaml2obj creates the .symtab ..."

545

"... creates the index 0 symbol even when ..."

591

for (size_t I = 0; I < 10000; ++I) would seem more natural.

I'd also reduce it to 1000 or 100 for speed.

602

AddRemoveSymbol

637

AddRemoveSection

666–683

Could you add some new lines in this block between the groups of setup followed by checks, to make it easier to see where the interesting tests actually are, please.

[abrachet]

Addressed review comments

[abrachet]

Fixed failing test for std::distance call.

[jhenderson]

LGTM, though @rupprecht should confirm too.

[abrachet]

rebase

[MaskRay]

llvm/include/llvm/Object/MutableELFObject.h

352

void addSymbol(MutableELFSymbol<ELFT> Sym)

If you don't have a lvalue overload, taking an rvalue is usually inferior to pass-by-value because you'll not able to call addSymbol with a lvalue.

355

void addSection(MutableELFSection<ELFT> Sym)

I think the method is self-explanatory. No need for a comment.

[MaskRay]

llvm/include/llvm/Object/MutableELFObject.h

41

Omit Other, but why do you need the defaulted move ctor?

175

Swap the two statements to omit - 1.

[abrachet]

Addressed comments. Made unit tests better.

[jhenderson]

Latest changes LGTM.

llvm/include/llvm/Object/MutableELFObject.h

template <typename ELFT> struct MutableELFSection {
   MutableELFSection(const Elf_Shdr &Header, StringRef Name, void *Base)
       : Header(Header), Name(Name),
         Data(OwningArrayRef<uint8_t>(Header.sh_size)) {
     std::memcpy(Data.data(),
                 reinterpret_cast<const uint8_t *>(Base) + Header.sh_offset,
                 Header.sh_size);
   }
 
+  MutableELFSection(const Elf_Shdr &Header, StringRef Name,
+                    OwningArrayRef<uint8_t> &&Data)
+      : Header(Header), Name(Name), Data(std::move(Data)) {}
+
+  MutableELFSection(MutableELFSection &&Other) = default;
+
   void setData(ArrayRef<uint8_t> Ref) {
     Data = OwningArrayRef<uint8_t>(Ref);
     Header.sh_size = Data.size();
   }
 
   operator const Elf_Shdr &() const { return Header; }
 };
 
public:
     /// Removes the entry at \param Index.
     void remove(uint64_t Index) {
       assert(Index < OriginalValues.size() && "Out of bounds");
       assert(Mappings[Index].Type != MappingType::Removed &&
              "Entry already removed");
       Mappings[Index].Type = MappingType::Removed;
     }
 
+    /// Add's \param Value to the end of the table.
+    NewType &add(NewType &&Value) {
+      NewValues.emplace_back(std::move(Value));
+      Mappings.emplace_back(NewValues.size() - 1, MappingType::New);
+      return NewValues.back();
+    }
+
     /// If the entry at index Index has already been made mutable, this returns
     /// a reference to that. Otherwise, this replaces the current entry at the
     /// specified index with a NewType constructued with Arguments.
     template <typename... Args>
     NewType &makeMutable(uint64_t Index, Args &&... Arguments) {
       assert(Index < Mappings.size() && "Out of bounds");
       assert(Mappings[Index].Type != MappingType::Removed);
       if (Mappings[Index].Type == MappingType::New)
template <typename ELFT> class MutableELFObject : public ELFObjectFile<ELFT> {
   /// allows use with MutableTable.
   void moveSectionNext(DataRefImpl &Sec) const override;
   Expected<StringRef> getSectionName(DataRefImpl Sec) const override;
   uint64_t getSectionIndex(DataRefImpl Sec) const override;
   Expected<ArrayRef<uint8_t>>
   getSectionContents(DataRefImpl Sec) const override;
 
   void moveSymbolNext(DataRefImpl &Sym) const override;
+  const Elf_Sym *getSymbol(DataRefImpl Sym) const override;
   Expected<StringRef> getSymbolName(DataRefImpl Sym) const override;
   ArrayRef<Elf_Sym> findSymbolTable(uint64_t ShType) const;
   uint32_t findSectionOfType(uint64_t ShType) const;
 
   const MutableSymbolTable &getWhichTable(DataRefImpl Sym) const {
     return Sections.getOriginal(Sym.d.a).sh_type == ELF::SHT_SYMTAB
                ? Symbols
                : DynSymbols;
public:
 
   /// Returns a mutable reference to the section pointed to by the
   /// section_iterator. It is usually more ergonomic to use the overload
   /// which takes a SectionRef.
   Expected<MutableELFSection<ELFT> &> getMutableSection(section_iterator Sec) {
     return getMutableSection(*Sec);
   }
 
+  /// Add symbol \param Sym.
+  MutableELFSymbol<ELFT> &addSymbol(MutableELFSymbol<ELFT> &&Sym) {
+    return Symbols.add(std::move(Sym));
+  }
+
+  /// Add section \param Sec.
+  MutableELFSection<ELFT> &addSection(MutableELFSection<ELFT> &&Sec) {
+    return Sections.add(std::move(Sec));
+  }
+
   /// Removes a symbol.
   void removeSymbol(SymbolRef Sym) {
     assert(Sections.getOriginal(Sym.getRawDataRefImpl().d.a).sh_type ==
                ELF::SHT_SYMTAB &&
            "Not pointing to symbol table");
     Symbols.remove(Sym.getRawDataRefImpl().d.b);
   }
 

 }
 
 template <typename ELFT>
 void MutableELFObject<ELFT>::moveSymbolNext(DataRefImpl &Sym) const {
   Sym.d.b = Symbols.getNextIndex(Sym.d.b);
 }
 
 template <typename ELFT>
+const Elf_Sym_Impl<ELFT> *
+MutableELFObject<ELFT>::getSymbol(DataRefImpl Sym) const {
+  const MutableSymbolTable &Table = getWhichTable(Sym);
+  if (const MutableELFSymbol<ELFT> *SymOrNull = Table.getConstIfNew(Sym.d.b))
+    return &SymOrNull->Header;
+  return ELFObjectFile<ELFT>::getSymbol(Sym);
+}
+
+template <typename ELFT>
 Expected<StringRef>
 MutableELFObject<ELFT>::getSymbolName(DataRefImpl Sym) const {
   const MutableSymbolTable &SymbolTable = getWhichTable(Sym);
   if (const MutableELFSymbol<ELFT> *SymOrNull =
           SymbolTable.getConstIfNew(Sym.d.b))
     return SymOrNull->Name;
   return ELFObjectFile<ELFT>::getSymbolName(Sym);
 }

llvm/unittests/Object/MutableELFObjectTest.cpp

#endif
 
   Iter = ++MutableObject.section_begin();
   EXPECT_EQ(Iter->getIndex(), 1u);
 
   ASSERT_FALSE(Iter->getName(Name));
   EXPECT_NE(Name, ".remove_me");
   EXPECT_NE(Name, ".remove_me_too");
 }
+
+TEST(MutableELFObject, AddSectionAndSymbols) {
+  SmallString<0> Storage;
+  Expected<std::unique_ptr<ObjectFile>> ErrOrObj = yaml2ObjectFile(Storage, R"(
+--- !ELF
+FileHeader:
+  Class:     ELFCLASS64
+  Data:      ELFDATA2LSB
+  Type:      ET_REL
+  Machine:   EM_X86_64)");
+
+  ASSERT_THAT_EXPECTED(ErrOrObj, Succeeded());
+  auto *ELFObjFile = dyn_cast<ELFObjectFile<ELF64LE>>(ErrOrObj->get());
+  ASSERT_TRUE(ELFObjFile);
+  MutableELFObject<ELF64LE> MutableObject(std::move(*ELFObjFile));
+
+  EXPECT_EQ(
+      std::distance(MutableObject.section_begin(), MutableObject.section_end()),
+      4);
+
+  Elf_Shdr_Impl<ELF64LE> Header;
+  Header.sh_size = 64;
+
+  MutableObject.addSection({Header, ".newsec", OwningArrayRef<uint8_t>(64)});
+
+  EXPECT_EQ(
+      std::distance(MutableObject.section_begin(), MutableObject.section_end()),
+      5);
+
+  auto Iter = MutableObject.section_begin();
+  std::advance(Iter, 4);
+
+  EXPECT_EQ(Iter->getSize(), 64u);
+  StringRef Name;
+  EXPECT_FALSE(Iter->getName(Name));
+  EXPECT_EQ(Name, ".newsec");
+
+  EXPECT_EQ(
+      std::distance(MutableObject.symbol_begin(), MutableObject.symbol_end()),
+      1);
+
+  Elf_Sym_Impl<ELF64LE> SymHeader;
+  SymHeader.st_shndx = ELF::SHN_ABS;
+  SymHeader.st_value = 1234;
+  SymHeader.st_size = 2;
+  auto ReturnedSym = MutableObject.addSymbol({SymHeader, "newsym"});
+  EXPECT_EQ(ReturnedSym.Header.st_value, 1234u);
+
+  EXPECT_EQ(
+      std::distance(MutableObject.symbol_begin(), MutableObject.symbol_end()),
+      2);
+
+  ELFSymbolRef NewSym = *symbol_iterator(++MutableObject.symbol_begin());
+  EXPECT_EQ(NewSym.getValue(), 1234u);
+  EXPECT_EQ(NewSym.getSize(), 2u);
+  Expected<StringRef> NameOrErr = NewSym.getName();
+  ASSERT_THAT_EXPECTED(NameOrErr, Succeeded());
+  EXPECT_EQ(*NameOrErr, "newsym");
+}