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

[abrachet]

Summary

Added support for symbols to MutableELFObject.

[abrachet]

Added unit tests and increased functionality.

[abrachet]

Made segment support similar to sections from ObjectFile for more continuity.

[abrachet]

rebase

[abrachet]

rebase

[grimar]

Few comments/suggestions are below.

llvm/include/llvm/Object/MutableELFObject.h

125

Mappings.erase(Mappings.begin() + Index)?

Perhaps also worth adding an assert like was done above.

130

Mappings.push_back({NewValues.size() - 1, MappingType::New});

191

You should probably at least add an assert checking ShType is one of the symbol table types.

198

return {};
(Its a bit confusing to see None which is normally used for returning the empty Optional<>)

201

It could be findSectionIndex, since you nowere check that ShType is a symbol table.

218

What about implementing const version using non-const? I.e. call getWhichTable from getWhichTable const.

279

I think when you have a symbol iterator, you usually know if it is a regular or dynamic
symbol. I am not sure it worth to allow mixing them. I.e. I am thinking about making 2 different functions,
like removeSymbol and removeDynamicSymbol.

I am not feeling strong here, though it seems to be less error prone.

[abrachet]

Addressed review comments

[abrachet]

Thanks for the comments @grimar!

llvm/include/llvm/Object/MutableELFObject.h

130

I'll do you one better, emplace_back :)

218

How do I do this without const_casting this? Is that how you do it? This is very useful because this duplicating methods like this has been bugging me recently. Thanks!

279

I have added these, I don't feel strongly either. FWIW, all of the getSymbol* methods in ELFObjectFile work on either symbol type so I duplicated that here. I'll keep this as not done so others who have an opinion stronger than ours can chime in.

[grimar]

llvm/include/llvm/Object/MutableELFObject.h

218

I think there is no way to do this without const_cast, but this is a normal/common way.

[jhenderson]

I don't think you want to be modifying the number of dynamic symbols at all. That would require you to make other changes to loadable code, which would impact addresses, and effectively require you to completely relink the program.

You may however still need to modify the contents of the existing dynamic symbols (specifically you may need to be able to update their section indexes).

llvm/include/llvm/Object/MutableELFObject.h

203

How about findSectionOfType?

218

It doesn't look like you use the non-const version anywhere, so it can be deleted, right?

[abrachet]

Added remove to MutableTable
Added getMutableSymbol to MutableELFObject
Added unit tests for older changes

[abrachet]

Remove removeDynSymbol from MutableELFObject

[abrachet]

In D65367#1621113, @jhenderson wrote:

I don't think you want to be modifying the number of dynamic symbols at all. That would require you to make other changes to loadable code, which would impact addresses, and effectively require you to completely relink the program.

You may however still need to modify the contents of the existing dynamic symbols (specifically you may need to be able to update their section indexes).

Ok I can just not add an addDynSymbol and removeDynSymbol methods then.

[jhenderson]

Not had time to look at everything in this patch, but it feels like you don't want removal functionality to be added in this patch at the same time as basic symbol functionality. That should be a different patch, and can then include both symbol and section removal. What do you think?

By the way, the overall approach with removal seems sound.

llvm/include/llvm/Object/MutableELFObject.h

75

"and only the equivalent to push_back, add." sounds like an unfinished sentence to me.

114–115

This comment doesn't document what the function does. It talks about class implementation details, which doesn't really make sense.

117–118

I'm inclined to think that getNextIndex shouldn't provide you the first index under any circumstance. You have getFirstNonRemoved instead.

119

No need for doxygen-style comments in the middle of a function.

Your opening comment should however explain what happens when the last index is passed in/reached by the loop.

124

Assert that the Index hasn't already been removed.

126

Comment?

128

This comment should refer to the parameter e.g. "Adds \param New to the back of the list."

129

Don't use "New" as a variable name, since it won't work with the upcoming change in variable naming rules.

134

How about getFirstIndex, to match getNextIndex?

Also this function needs a comment.

135

I'd put the I == end() clause in here, rather than in the body of the loop and just return I instead of the llvm_unreachable statement.

146

I'd call this lastIndex, as this doesn't return an iterator.

element, this -> element. This

[abrachet]

Moved remove and add out of this patch

[jhenderson]

llvm/include/llvm/Object/MutableELFObject.h

74

be, entries -> be: entries

147

Does this belong to a later patch?

192

I'd be inclined to just call this getSymbolTable.

248–250

This overload seems a bit superfluous to me.

253

Call this getMutableDynamicSymbol

262–264

This overload seems a bit superfluous to me.

276

This change seems to be unrelated?

Avoid refactors of code which don't have anything to do with the current change, and put them in their own change.

331

Why is this looping over only the original sections?

llvm/unittests/Object/MutableELFObjectTest.cpp

237

Looks to me like you need a checking first of all counting the number of symbols. Also, why do you need to construct a new symbol_iterator, rather than using the output of ++MutableObject.symbol_begin() directly?

244

FirstSym and SecondSym?

Also, same comment as above.

252

"no change public methods" what are those? :)

I think you need to rewrite this comment.

282

I think the same comments made to patch 1 apply here too, regarding the std::move-ing of ELFObjFile making this reference invalid.

291

I think less confusing names might be FromBase and FromMutable or something like that. My first reaction was "why is a file a SymbolRef?"

319

This loop needs a comment explaining its purpose and the consumeError call.

329

Is this comment correct?

350

Same comments as earlier.

356

This seems a bit redundant...

Same goes for the other changes below. You're basically testing that C++ works...

I think you want to be fetching the symbol again from one or both of the object interfaces (base or sub-class) and showing that the value has been updated.

368

Don't abbreviate names unnecessarily: use "BasicDynamicSymbol"

388

Same comments as above.

394

My gut reaction was "where are the equivalent dynamic symbol tests for the earlier regular symbol cases?" Perhaps worth a comment to that effect somewhere, perhaps making it clear that the earlier cases test common code.

[rupprecht]

llvm/include/llvm/Object/MutableELFObject.h

47

using Elf_Sym = typename ELFT::Sym;

llvm/unittests/Object/MutableELFObjectTest.cpp

360–364

What happens to the string table in this case?

The names for symbols in the symbol table is backed by the string table, so if you change the name from "test" to "new_name", the string table needs to be rewritten for that, but it doesn't looks like there's a corresponding MutableTable StringTable to back that change.

[abrachet]

Addressed review comments

[abrachet]

llvm/include/llvm/Object/MutableELFObject.h

147

No it is needed. I explain this in an answer to your question here

331

Because ELFObjectFile::getSymbol() (and also directly many other symbol related methods) needs the DataRef to have an index to the original section, and this is how to differentiate between dynamic and regular symbols. I will think more about this, but this was the least disruptive way to inherit from ELFObjectFile I believe. It's worth noting that this is a private method.

llvm/unittests/Object/MutableELFObjectTest.cpp

237

SymbolicFile::symbol_begin() returns a basic_symbol_iterator pointing to BasicSymbolRefs whose only meaningful method for testing is getFlags(), but symbol_iterator points to SymbolRefs which has getValue and getName.

356

Oops! They are more meaningful now :)

360–364

Nothing happens for now. The idea, and this is the same as objcopy::elf::Object, is that it gets updated only when doing final layout.

[jhenderson]

llvm/unittests/Object/MutableELFObjectTest.cpp

237

Okay. Please write this as:

symbol_iterator FirstSym(++MutableObject.symbol_begin());

255–257

I'm not sure you need this here?

260

Perhaps "in SymbolRef's public methods beteween..." to avoid using "between" twice.

323

ElfObjFile might be invalid. You will need to make another copy of the input, like in patch 1.

[abrachet]

Addressed review comments

[jhenderson]

llvm/unittests/Object/MutableELFObjectTest.cpp

352

You've made a change to address my comment for this style above. Please make the same change throughout. It wasn't a local issue for my earlier point :)

[abrachet]

Addressed comments

[abrachet]

llvm/unittests/Object/MutableELFObjectTest.cpp

352

Sorry about that, silly mistake.

[rupprecht]

I don't see anything major left, but please wait for James to take another look

llvm/include/llvm/Object/MutableELFObject.h

338–351

I don't think these methods have anything to do with MutableELFObject, so they should moved Object/SymbolicFile.h so others can use it. (A FIXME comment is fine for now).

llvm/unittests/Object/MutableELFObjectTest.cpp

360–364

I suppose it depends on how the writer/finalizer for MutableElfObject looks like -- a naive implementation that just iterates over sections and writes out the contents will lose the renamed values, because those will be privately owned by MutableELFSymbol, and not reflected in the (non-mutated) string table.

[jhenderson]

LGTM.

[MaskRay]

llvm/include/llvm/Object/MutableELFObject.h

332

uint32_t I = 0, E = Sections.originalSize(); I != E; ++I)

https://llvm.org/docs/CodingStandards.html#don-t-evaluate-end-every-time-through-a-loop

Not a strong opinion though.

[abrachet]

Addressed review comments

[abrachet]

llvm/include/llvm/Object/MutableELFObject.h

332

Good catch! Thanks

llvm/include/llvm/Object/ELFObjectFile.h

 
 public:
   ELFObjectFile(ELFObjectFile<ELFT> &&Other);
   static Expected<ELFObjectFile<ELFT>> create(MemoryBufferRef Object);
 
   const Elf_Rel *getRel(DataRefImpl Rel) const;
   const Elf_Rela *getRela(DataRefImpl Rela) const;
 
-  const Elf_Sym *getSymbol(DataRefImpl Sym) const {
+  virtual const Elf_Sym *getSymbol(DataRefImpl Sym) const {
     auto Ret = EF.template getEntry<Elf_Sym>(Sym.d.a, Sym.d.b);
     if (!Ret)
       report_fatal_error(errorToErrorCode(Ret.takeError()).message());
     return *Ret;
   }
 
   virtual const Elf_Shdr *getSection(DataRefImpl Sec) const {
     return reinterpret_cast<const Elf_Shdr *>(Sec.p);
   }
 
   basic_symbol_iterator symbol_begin() const override;
   basic_symbol_iterator symbol_end() const override;
 
-  elf_symbol_iterator dynamic_symbol_begin() const;
-  elf_symbol_iterator dynamic_symbol_end() const;
+  virtual elf_symbol_iterator dynamic_symbol_begin() const;
+  virtual elf_symbol_iterator dynamic_symbol_end() const;
 
   section_iterator section_begin() const override;
   section_iterator section_end() const override;
 
   Expected<int64_t> getRelocationAddend(DataRefImpl Rel) const override;
 
   uint8_t getBytesInAddress() const override;
   StringRef getFileFormatName() const override;

llvm/include/llvm/Object/MutableELFObject.h

void setData(ArrayRef<uint8_t> Ref) {
     Header.sh_size = Data.size();
   }
 
   operator const Elf_Shdr &() const { return Header; }
 };
 
 /// This class is used for doing mutations on an ELFObjectFile. The
 /// ELFObjectFile class only provides an interface for reading object files.
+template <typename ELFT> struct MutableELFSymbol {
+  using Elf_Sym = typename ELFT::Sym;
+
+  Elf_Sym Header;
+  std::string Name;
+
+  MutableELFSymbol(const Elf_Sym &Header, StringRef Name)
+      : Header(Header), Name(Name) {}
+
+  operator const Elf_Sym &() const { return Header; }
+};
+
 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
   /// type to create on modification of the entry. It is required that
   /// NewType have an `operator const OrigType &()`.
   ///
   /// The table keeps a list of mappings, these mappings can have one of two
   /// states either Original or New. In the case of Original the index
   /// associated with the mapping is into the original table in the file. For
-  /// a New mapping, the index is into the NewValues vector. This design allows
-  /// fewer copies to be made than there would otherwise need to be, entries
-  /// with no modifications never get copied and the only overhead for those is
-  /// an index. Entries which get modified can have richer types during program
-  /// executation than are allowed by the object file standard.
+  /// a New mapping, the index is into the NewValues vector.
+  ///
+  /// The design of MutableTable allows fewer copies to be made than there
+  /// would otherwise need to be: entries with no modifications never get
+  /// copied and the only overhead for those is an index. Entries which get
+  /// modified can have richer types during program executation than are
+  /// allowed by the object file standard.
   template <typename OrigType, typename NewType> class MutableTable {
     struct MappingType {
       enum MappedType { Original, New };
 
       size_t Index;
       MappedType Type;
 
       MappingType(size_t Index, MappedType Type) : Index(Index), Type(Type) {}
public:
     /// operator OrigType to make the proper conversion.
     const OrigType &operator[](size_t Index) const {
       assert(Index < Mappings.size() && "Out of bounds");
       if (Mappings[Index].Type == MappingType::New)
         return static_cast<const OrigType &>(NewValues[Mappings[Index]]);
       return OriginalValues[Mappings[Index]];
     }
 
     /// Get the OrigType at index Index. This method ignores any changes made
     /// and always returns the OrigType from its original state at its original
     /// index.
     const OrigType &getOriginal(size_t Index) const {
       assert(Index < OriginalValues.size() && "Out of bounds");
       return OriginalValues[Index];
     }
 
     /// 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(size_t Index, Args &&... Arguments) {
       assert(Index < Mappings.size() && "Out of bounds");
       if (Mappings[Index].Type == MappingType::New)
         return NewValues[Mappings[Index]];
       Mappings[Index] = MappingType(NewValues.size(), MappingType::New);
       NewValues.emplace_back(std::forward<Args>(Arguments)...);
       return NewValues.back();
     }
 
     /// Returns a pointer to the NewType if the entry at the specified index
     /// has had makeMutable called on it. Otherwise this method returns nullptr.
     const NewType *getConstIfNew(size_t Index) const {
       assert(Index < Mappings.size() && "Out of bounds");
       return Mappings[Index].Type == MappingType::New
                  ? &NewValues[Mappings[Index]]
                  : nullptr;
     }
 
     /// Return the number of elements in the table.
     size_t size() const { return Mappings.size(); }
+
+    size_t originalSize() const { return OriginalValues.size(); }
   };
 
   using Elf_Shdr = typename ELFT::Shdr;
   using Elf_Ehdr = typename ELFT::Ehdr;
+  using Elf_Sym = typename ELFT::Sym;
+
+  using MutableSymbolTable = MutableTable<Elf_Sym, MutableELFSymbol<ELFT>>;
 
   MutableTable<Elf_Shdr, MutableELFSection<ELFT>> Sections;
+  MutableSymbolTable Symbols;
+  MutableSymbolTable DynSymbols;
 
   const Elf_Ehdr &getHeader() const {
     return *reinterpret_cast<const Elf_Ehdr *>(this->base());
   }
 
   /// Many getSection* methods in ELFObjectFile use getSection to get the
   /// the header associated with that section. This override returns a valid
   /// section header whether the section has been modified or not.
   const Elf_Shdr *getSection(DataRefImpl Sec) const override {
     return &Sections[Sec.p];
   }
 
   /// moveSectionNext must be overridden because MutableELFObject::section_begin
   /// works differently than in ELFObjectFile. In this class, sections are
   /// iterated with their index, not address in the file, which allows use with
   /// MutableTable.
+  const Elf_Sym *getSymbol(DataRefImpl Sym) const override {
+    const MutableSymbolTable &Table = getSymbolTable(Sym);
+    if (const MutableELFSymbol<ELFT> *SymOrNull = Table.getConstIfNew(Sym.d.b))
+      return &SymOrNull->Header;
+    return ELFObjectFile<ELFT>::getSymbol(Sym);
+  }
+
   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;
 
-  static DataRefImpl toDataRef(uintptr_t Ptr) {
-    DataRefImpl Ref;
-    Ref.p = Ptr;
-    return Ref;
+  Expected<StringRef> getSymbolName(DataRefImpl Sym) const override;
+  ArrayRef<Elf_Sym> findSymbolTable(uint64_t ShType) const;
+  uint32_t findSectionOfType(uint64_t ShType) const;
+
+  const MutableSymbolTable &getSymbolTable(DataRefImpl Sym) const {
+    return Sections.getOriginal(Sym.d.a).sh_type == ELF::SHT_SYMTAB
+               ? Symbols
+               : DynSymbols;
   }
 
+  static DataRefImpl toDataRef(uintptr_t Ptr);
+  static DataRefImpl toDataRef(uint32_t A, uint32_t B);
+
 public:
   explicit MutableELFObject(ELFObjectFile<ELFT> &&B)
       : ELFObjectFile<ELFT>(std::move(B)),
         Sections(ArrayRef<Elf_Shdr>(reinterpret_cast<const Elf_Shdr *>(
                                         this->base() + getHeader().e_shoff),
-                                    getHeader().e_shnum)) {}
+                                    getHeader().e_shnum)),
+        Symbols(findSymbolTable(ELF::SHT_SYMTAB)),
+        DynSymbols(findSymbolTable(ELF::SHT_DYNSYM)) {}
 
   section_iterator section_begin() const override {
     return section_iterator(SectionRef(toDataRef(0), this));
   }
 
   section_iterator section_end() const override {
     return section_iterator(SectionRef(toDataRef(Sections.size()), this));
   }
 
+  basic_symbol_iterator symbol_begin() const override {
+    return basic_symbol_iterator(
+        SymbolRef(toDataRef(findSectionOfType(ELF::SHT_SYMTAB), 0), this));
+  }
+
+  basic_symbol_iterator symbol_end() const override {
+    return basic_symbol_iterator(SymbolRef(
+        toDataRef(findSectionOfType(ELF::SHT_SYMTAB), Symbols.size()), this));
+  }
+
+  elf_symbol_iterator dynamic_symbol_begin() const override {
+    return basic_symbol_iterator(
+        SymbolRef(toDataRef(findSectionOfType(ELF::SHT_DYNSYM), 0), this));
+  }
+
+  elf_symbol_iterator dynamic_symbol_end() const override {
+    return basic_symbol_iterator(SymbolRef(
+        toDataRef(findSectionOfType(ELF::SHT_DYNSYM), DynSymbols.size()),
+        this));
+  }
+
+  /// Returns a mutable reference to the symbol at the specified index.
+  Expected<MutableELFSymbol<ELFT> &> getMutableSymbol(SymbolRef Sym) {
+    Expected<StringRef> Name = getSymbolName(Sym.getRawDataRefImpl());
+    if (!Name)
+      return Name.takeError();
+    return Symbols.makeMutable(Sym.getRawDataRefImpl().d.b,
+                               Symbols[Sym.getRawDataRefImpl().d.b], *Name);
+  }
+
+  /// Returns a mutable reference to the dynamic symbol at the specified index.
+  Expected<MutableELFSymbol<ELFT> &> getMutableDynamicSymbol(SymbolRef Sym) {
+    Expected<StringRef> Name = getSymbolName(Sym.getRawDataRefImpl());
+    if (!Name)
+      return Name.takeError();
+    return DynSymbols.makeMutable(Sym.getRawDataRefImpl().d.b,
+                                  DynSymbols[Sym.getRawDataRefImpl().d.b],
+                                  *Name);
+  }
+
   /// Returns a mutable reference to the section pointed to by Sec. A possible
   /// usage to change all sections with alignment of 0 to 1 could be:
   /// @code{.cpp}
   /// for (const SectionRef Sec : MutObj.sections())
   ///   if (!Sec.getAlignment()) {
   ///     if (auto MutSecOrErr = MutObj.getMutableSection(Sec))
   ///       then;
   ///     else
   ///       handleError(...);
   ///     MutSecOrErr->Header.sh_addralign = 1;
   ///   }
   /// @endcode
   Expected<MutableELFSection<ELFT> &> getMutableSection(SectionRef Sec) {
     const Elf_Shdr_Impl<ELFT> &Header = Sections[Sec.getRawDataRefImpl().p];
     Expected<StringRef> Name = getSectionName(Sec.getRawDataRefImpl());
     if (!Name)
       return Name.takeError();
     return Sections.makeMutable(Sec.getRawDataRefImpl().p, Header, *Name, this);
   }
 };
 
 template <typename ELFT>
 void MutableELFObject<ELFT>::moveSectionNext(DataRefImpl &Sec) const {
   ++Sec.p;
 }
 
 template <typename ELFT>
 uint64_t MutableELFObject<ELFT>::getSectionIndex(DataRefImpl Sec) const {
   return Sec.p;
 }

 template <typename ELFT>
 Expected<ArrayRef<uint8_t>>
 MutableELFObject<ELFT>::getSectionContents(DataRefImpl Sec) const {
   if (const MutableELFSection<ELFT> *SecOrNull = Sections.getConstIfNew(Sec.p))
     return ArrayRef<uint8_t>(SecOrNull->Data.data(), SecOrNull->Header.sh_size);
   return ELFObjectFile<ELFT>::getSectionContents(Sec);
 }
 
+template <typename ELFT>
+Expected<StringRef>
+MutableELFObject<ELFT>::getSymbolName(DataRefImpl Sym) const {
+  const MutableSymbolTable &SymbolTable = getSymbolTable(Sym);
+  if (const MutableELFSymbol<ELFT> *SymOrNull =
+          SymbolTable.getConstIfNew(Sym.d.b))
+    return SymOrNull->Name;
+  return ELFObjectFile<ELFT>::getSymbolName(Sym);
+}
+
+template <typename ELFT>
+ArrayRef<typename ELFT::Sym>
+MutableELFObject<ELFT>::findSymbolTable(uint64_t ShType) const {
+  assert(ShType == ELF::SHT_SYMTAB ||
+         ShType == ELF::SHT_DYNSYM && "Not a symbol table type");
+  for (const auto &Sec : this->sections())
+    if (ELFSectionRef(Sec).getType() == ShType)
+      return ArrayRef<Elf_Sym>(
+          reinterpret_cast<const Elf_Sym *>(this->base() +
+                                            ELFSectionRef(Sec).getOffset()),
+          Sec.getSize() / sizeof(Elf_Sym));
+
+  return {};
+}
+
+template <typename ELFT>
+uint32_t MutableELFObject<ELFT>::findSectionOfType(uint64_t ShType) const {
+  for (uint32_t I = 0, E = Sections.originalSize(); I < E; ++I)
+    if (Sections.getOriginal(I).sh_type == ShType)
+      return I;
+  return 0;
+}
+
+template <typename ELFT>
+DataRefImpl MutableELFObject<ELFT>::toDataRef(uintptr_t Ptr) {
+  DataRefImpl Ref;
+  Ref.p = Ptr;
+  return Ref;
+}
+
+template <typename ELFT>
+DataRefImpl MutableELFObject<ELFT>::toDataRef(uint32_t A, uint32_t B) {
+  DataRefImpl Ref;
+  Ref.d.a = A;
+  Ref.d.b = B;
+  return Ref;
+}
+
 } // namespace object
 } // namespace llvm
 
 #endif // LLVM_OBJECT_MUTABLEELFOBJECT_H

llvm/unittests/Object/MutableELFObjectTest.cpp

Sections:
   // Check that Contents has size 2 because header's sh_size was changed.
   EXPECT_EQ(Contents->size(), 2u);
 
   // Make sure a section wasn't added.
   ptrdiff_t NewNumSections =
       std::distance(MutableObject.section_begin(), MutableObject.section_end());
   EXPECT_EQ(NewNumSections, NumSections);
 }
+
+// Test basic public methods on symbols.
+TEST(MutableELFObject, BasicSymbol) {
+  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
+Symbols:
+  - Name:     test
+    Index:    SHN_ABS
+    Value:    0x1234
+    Binding:  STB_LOCAL
+  - Name:     second
+    Index:    SHN_ABS
+    Value:    1
+    Binding:  STB_GLOBAL)");
+
+  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.symbol_begin(), MutableObject.symbol_end()),
+      3);
+
+  symbol_iterator FirstSym(++MutableObject.symbol_begin());
+  EXPECT_EQ(FirstSym->getValue(), 0x1234U);
+  EXPECT_FALSE(FirstSym->getFlags() & SymbolRef::SF_Global);
+  auto NameOrErr = FirstSym->getName();
+  ASSERT_THAT_EXPECTED(NameOrErr, Succeeded());
+  EXPECT_EQ(*NameOrErr, "test");
+
+  symbol_iterator SecondSym(++FirstSym);
+  EXPECT_EQ(SecondSym->getValue(), 1U);
+  EXPECT_TRUE(SecondSym->getFlags() & SymbolRef::SF_Global);
+  NameOrErr = SecondSym->getName();
+  ASSERT_THAT_EXPECTED(NameOrErr, Succeeded());
+  EXPECT_EQ(*NameOrErr, "second");
+}
+
+// Test that there is no change in SymbolRef's public methods between
+// MutableELFObjectFile and ELFObjectFile.
+TEST(MutableELFObject, SameAsELFObjectFile) {
+  StringRef Yaml = R"(
+--- !ELF
+FileHeader:
+  Class:     ELFCLASS64
+  Data:      ELFDATA2LSB
+  Type:      ET_REL
+  Machine:   EM_X86_64
+DynamicSymbols:
+  - Name:    test
+    Index:   SHN_ABS
+    Value:   0x1234
+    Binding: STB_GLOBAL
+  - Name:     second
+    Index:    SHN_ABS
+    Value:    0
+    Binding:  STB_GLOBAL)";
+
+  SmallString<0> Storage;
+  raw_svector_ostream OS(Storage);
+  yaml::Input Input(Yaml);
+  ASSERT_THAT_ERROR(convertYAML(Input, OS), Succeeded());
+
+  Expected<std::unique_ptr<ObjectFile>> MutObjOrErr =
+      ObjectFile::createObjectFile(MemoryBufferRef(OS.str(), "YamlObject"));
+  ASSERT_THAT_EXPECTED(MutObjOrErr, Succeeded());
+  auto *ELFObjFile = dyn_cast<ELFObjectFile<ELF64LE>>(MutObjOrErr->get());
+  ASSERT_TRUE(ELFObjFile);
+  MutableELFObject<ELF64LE> MutableObject(std::move(*ELFObjFile));
+
+  Expected<std::unique_ptr<ObjectFile>> ObjOrErr =
+      ObjectFile::createObjectFile(MemoryBufferRef(OS.str(), "YamlObject"));
+  ASSERT_THAT_EXPECTED(ObjOrErr, Succeeded());
+  ELFObjFile = dyn_cast<ELFObjectFile<ELF64LE>>(ObjOrErr->get());
+  ASSERT_TRUE(ELFObjFile);
+
+  auto TestSymbols = [](SymbolRef FromBase, SymbolRef FromMutable) {
+    EXPECT_EQ(FromBase.getValue(), FromMutable.getValue());
+    EXPECT_EQ(FromBase.getAlignment(), FromMutable.getAlignment());
+    EXPECT_EQ(FromBase.getCommonSize(), FromMutable.getCommonSize());
+    EXPECT_EQ(FromBase.getValue(), FromMutable.getValue());
+
+    auto ObjNameOrErr = FromBase.getName();
+    ASSERT_THAT_EXPECTED(ObjNameOrErr, Succeeded());
+    auto MutNameOrErr = FromMutable.getName();
+    ASSERT_THAT_EXPECTED(MutNameOrErr, Succeeded());
+    EXPECT_EQ(*ObjNameOrErr, *MutNameOrErr);
+
+    auto ObjAddrOrErr = FromBase.getAddress();
+    ASSERT_THAT_EXPECTED(ObjAddrOrErr, Succeeded());
+    auto MutAddrOrErr = FromMutable.getAddress();
+    ASSERT_THAT_EXPECTED(MutAddrOrErr, Succeeded());
+    EXPECT_EQ(*ObjAddrOrErr, *MutAddrOrErr);
+
+    auto ObjTypeOrErr = FromBase.getType();
+    ASSERT_THAT_EXPECTED(ObjTypeOrErr, Succeeded());
+    auto MutTypeOrErr = FromMutable.getType();
+    ASSERT_THAT_EXPECTED(MutTypeOrErr, Succeeded());
+    EXPECT_EQ(*ObjTypeOrErr, *MutTypeOrErr);
+  };
+
+  for (const auto &Tuple : zip(ELFObjFile->symbols(), MutableObject.symbols()))
+    TestSymbols(std::get<0>(Tuple), std::get<1>(Tuple));
+
+  // Make every symbol mutable to test that there is no change between original
+  // symbols and mutable symbols.
+  for (const SymbolRef &Sym : MutableObject.symbols())
+    ASSERT_THAT_EXPECTED(MutableObject.getMutableSymbol(Sym), Succeeded());
+
+  for (const auto &Tuple : zip(ELFObjFile->symbols(), MutableObject.symbols()))
+    TestSymbols(std::get<0>(Tuple), std::get<1>(Tuple));
+}
+
+// Test mutating symbols.
+TEST(MutableELFObject, MutateSymbol) {
+  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
+Symbols:
+  - Name:    test
+    Index:   SHN_ABS
+    Value:   2
+    Binding: STB_GLOBAL)");
+
+  ASSERT_THAT_EXPECTED(ErrOrObj, Succeeded());
+  auto *ELFObjFile = dyn_cast<ELFObjectFile<ELF64LE>>(ErrOrObj->get());
+  ASSERT_TRUE(ELFObjFile);
+  MutableELFObject<ELF64LE> MutableObject(std::move(*ELFObjFile));
+
+  symbol_iterator TestSym(++MutableObject.symbol_begin());
+  Expected<MutableELFSymbol<ELF64LE> &> MutSymOrErr =
+      MutableObject.getMutableSymbol(*TestSym);
+  ASSERT_THAT_EXPECTED(MutSymOrErr, Succeeded());
+  EXPECT_EQ(MutSymOrErr->Header.st_value, 2u);
+  MutSymOrErr->Header.st_value = 5;
+  EXPECT_EQ(TestSym->getValue(), 5u);
+
+  EXPECT_EQ(MutSymOrErr->Name, "test");
+  MutSymOrErr->Name = "new_name";
+  Expected<StringRef> NameOrErr = TestSym->getName();
+  ASSERT_THAT_EXPECTED(NameOrErr, Succeeded());
+  EXPECT_EQ(*NameOrErr, "new_name");
+}
+
+// Test basic public methods on dynamic symbols. Symbols and dynamic symbols
+// share the same inner working so it isn't crucial to test both. This test of
+// dynamic symbols is enough to ensure that dynamic symbols work properly.
+TEST(MutableELFObject, BasicDynamicSymbol) {
+  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
+DynamicSymbols:
+  - Name:    test
+    Index:   SHN_ABS
+    Value:   0x1234
+    Binding: STB_GLOBAL)");
+
+  ASSERT_THAT_EXPECTED(ErrOrObj, Succeeded());
+  auto *ELFObjFile = dyn_cast<ELFObjectFile<ELF64LE>>(ErrOrObj->get());
+  ASSERT_TRUE(ELFObjFile);
+  MutableELFObject<ELF64LE> MutableObject(std::move(*ELFObjFile));
+
+  symbol_iterator DynSym(++MutableObject.dynamic_symbol_begin());
+  EXPECT_EQ(DynSym->getValue(), 0x1234U);
+  EXPECT_TRUE(DynSym->getFlags() & SymbolRef::SF_Global);
+  auto NameOrErr = DynSym->getName();
+  ASSERT_THAT_EXPECTED(NameOrErr, Succeeded());
+  EXPECT_EQ(*NameOrErr, "test");
+}