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

[abrachet]

Summary

Add support for removing sections and symbols.

[abrachet]

Use new MutableTable. Add support for deletion.

[abrachet]

rebase

[jhenderson]

Self note: I still need to look at coverage of the unit tests.

llvm/include/llvm/Object/MutableELFObject.h

74

entries, similarly -> entries. Similary

128

non remove index -> next index that hasn't been removed

The comment should say what happens if there are no such indexes before the end of the table.

[jhenderson]

llvm/include/llvm/Object/MutableELFObject.h

131

You need to assert that CurrentIndex is valid.

std::find_if might be an easier to read function to use here.

138

Could this be based on getNextIndex()? You could check first if the first element has not been removed, and then call getNextIndex(0);

145

non removed entries -> entries that haven't been removed

146

Maybe getEffectiveIndex? Relative doesn't really sound right. What is it relative to?

154–155

"different to size() because size() returns the number of entries that haven't been removed."

156

Perhaps call this getEndIndex()

346

Superfluous overload.

354

Superfluous overload.

llvm/unittests/Object/MutableELFObjectTest.cpp

400

Missing trailing full stop.

425

This shouldn't be auto. Can it be done inline in the check?

430

This shouldn't be auto.

441

Can this be done inline in the check?

445

Missing trailing full stop.

468

Probably requires a comment saying why "6".

481

"index. It"

But really, I'm not sure you should have these tests here. We don't usually test assertions. If this a likely situation to occur, then perhaps the methods should return an Expected.

[jhenderson]

llvm/unittests/Object/MutableELFObjectTest.cpp

418

Not sure, but it might be interesting to add some dynamic symbols and show that iterating over those isn't affected by the regular table changes.

433

I think it would be interesting to see what happens when you remove the second symbol too, leaving only the null entry. I also think it would be interesting to test what happens if you try to remove the null entry (I think this should probably be an error).

494

Again, a test case for trying to remove the null section header might be interesting (it should probably be an error).

504–505

Rather than looking at just one, you should probably loop over all of the remaining sections, including the null one.

[abrachet]

Addressed review comments

[abrachet]

llvm/unittests/Object/MutableELFObjectTest.cpp

418

Wow! Great catch! There was actually a bug here with my moveSymbolNext not differentiating between the two symbol tables. Thanks :)

433

I haven't done this or the similar comment for sections yet. Do you think the removeSymbol method should return Error or should this just be an assert?

[jhenderson]

llvm/include/llvm/Object/MutableELFObject.h

128

You've lost the "the" between "Gets" and "next".

This part hasn't been addressed:

The comment should say what happens if there are no such indexes before the end of the table.

130

Why the cast? And what is the type of Range? I don't think you want auto here.

131

What happens if std::find_if returns Range.end()? Won't that result in you dereferencing something invalid?

151

You misunderstood my previous comment. I only wanted you to change the comment from the word "different". In other words, the full text should be:

/// Returns the index of the last element. This is different to size()
/// because size() returns the number of entries that haven't been removed.

llvm/unittests/Object/MutableELFObjectTest.cpp

433

Good question. I'd probably just assert for now.

484–485

added by yaml2obj

[abrachet]

Addressed review comments

[abrachet]

llvm/include/llvm/Object/MutableELFObject.h

130

Because templates are very finicky :) The alternative is I could do seq<uint64_t>(CurrentIndex + 1, Mappings.size()). Do you have a preference either way?

131

I had to check this to make sure, *llvm::seq(0, 10).end() yields 10, so it just returns the index in the end state, so just Mappings.size().

llvm/unittests/Object/MutableELFObjectTest.cpp

433

Ok, I've added assertions to removeSymbol and removeSection. I should not test these explicitly because it is an assert, right?

[rupprecht]

llvm/include/llvm/Object/MutableELFObject.h

133–137

*llvm::find_if(seq(CurrentIndex + 1, (uint64_t)Mappings.size()), [this](uint64_t Index) { ... });

191–196

Many of these methods are now linear, meaning this library isn't feasible to operate on files that have a large number of symbols (especially if you end up calling these methods in a loop and go quadratic). I think this is fine to experiment with but should be noted, so it can be fixed before adding uses of it.

llvm/unittests/Object/MutableELFObjectTest.cpp

484–488

I think writing a test matcher would make this more self-documenting than a comment.
e.g.
EXPECT_THAT(MutableObject, SectionsAre({".remove_me", ..., }));
which would check both std::distance and each section name by iterating over all sections.

Writing a test helper (instead of a full blown matcher) would also work; you might not need a thorough matcher outside of this test.

[abrachet]

Addressed review comments

[abrachet]

llvm/include/llvm/Object/MutableELFObject.h

191–196

FWIW, I think size is the only method which gets its complexity changed, this can be solved by just keeping a running count, and having add and remove methods change Size. Does that sound ok?

Methods like getMutableSection index into the table without any checks to deleted entries so those are all O(1), I'm trying to think of others which became O(N^2) but I cannot think of them.

llvm/unittests/Object/MutableELFObjectTest.cpp

484–488

I created this MatchesRange template function. It's not as cool as what you were suggesting but I couldn't figure out how to make a gtest matcher.

[jhenderson]

llvm/include/llvm/Object/MutableELFObject.h

128–130

Hmm... thinking about it, how about "Gets the index of the next entry that hasn't ..." and "If there is no entry after ..."

Also "this returns the index one past the last entry" to be more precise.

133

Maybe the cast indicates that these indexes should be size_t everywhere, since they're indexes into a container? What do you think?

191–196

Keeping a running count would make sense to me, if it improves the complexity of other functions.

llvm/unittests/Object/MutableELFObjectTest.cpp

23

Do you need the MatchLen parameter? I don't think you do...

28

std::distance already returns a size_t. No need to cast it.

31

const T& maybe?

433

Exactly. Don't test asserts.

490

Match -> SectionsMatch

[labath]

llvm/unittests/Object/MutableELFObjectTest.cpp

484–488

You could consider something like this:

std::vector<StringRef> Sections;
transform(MutableObject.sections(), [](??? S) { return S->getName(); }, std::back_inserter(Sections));
EXPECT_THAT(Sections, testing::ElementsAre(".foo", ".bar", ...));

The reason I would prefer that over your current solution is because this will give you a mostly reasonable error message if the check fails, whereas here you'll just get "false is not true"(tm). (You could even make a utility function to hide the transform stuff and make the check a one-liner.)

[rupprecht]

llvm/include/llvm/Object/MutableELFObject.h

191–196

getNextIndex() is also linear (with the linear factor being # of removed symbols), making getFirstIndex() linear, which makes all the *_begin() iterator methods linear.

See rL354597 for a real world motivation with llvm-objcopy. An easy way to need to "remove" a lot of symbols is to extract a single section with llvm-objcopy --only-section .foo which will implicitly delete symbols not in that section. If the number of symbols not in that section is large, it will be slow if you keep having to iterate over all removed symbols. (See the test case here if you're interested: https://reviews.llvm.org/D58296?vs=on&id=187690)

That said, there isn't necessarily a need to prematurely optimize this. I think it's fine to just document that the methods are linear, and if we run into regressions when using it on real scenarios, we can rework the data structures.

[abrachet]

Addressed review comments.

[abrachet]

llvm/include/llvm/Object/MutableELFObject.h

133

Sounds good. I have changed to size_t it does make more sense.

191–196

Keeping a running count would make sense to me, if it improves the complexity of other functions.

I think that MutableTable::size() isn't actually even used anywhere anymore I just updated it because it was used previously but its use was replaced by getEndIndex(). Should I just remove it then? Keeping a count of the number of elements would only improve the complexity of size() I think.

getNextIndex() is also linear (with the linear factor being # of removed symbols), making getFirstIndex() linear, which makes all the *_begin() iterator methods linear.

Oh, right.

See rL354597 for a real world motivation with llvm-objcopy. An easy way to need to "remove" a lot of symbols is to extract a single section with llvm-objcopy --only-section .foo which will implicitly delete symbols not in that section. If the number of symbols not in that section is large, it will be slow if you keep having to iterate over all removed symbols. (See the test case here if you're interested: https://reviews.llvm.org/D58296?vs=on&id=187690)

FWIW currently the deletion of a symbol I think is linear (the use of remove_if) and has the cost of copying the elements where this doesn't have that same cost. Certainly it will be slower if iteration happens many times after.

That said, there isn't necessarily a need to prematurely optimize this. I think it's fine to just document that the methods are linear, and if we run into regressions when using it on real scenarios, we can rework the data structures.

Absolutely :)

llvm/unittests/Object/MutableELFObjectTest.cpp

28

I thought so too but it returns the iterators difference_type which on most containers is ptrdiff_t. That's a really strange decision by the committee I think, I don't see the rational of not using size_t given distance will never be negative.

484–488

Oh that's much better, before when I didn't know the order yaml2obj was putting the sections in I had to use lldb because all I got was "false is not true" like you said. Thanks!

You could even make a utility function to hide the transform stuff and make the check a one-liner.

I'll probably do this later when I use it in more than one place.

[MaskRay]

llvm/include/llvm/Object/MutableELFObject.h

133

uint64_t -> size_t change should be made to Part 1 D64281.

[labath]

llvm/unittests/Object/MutableELFObjectTest.cpp

28

For random-access iterators, it is valid to plug the begin and end iterators into std::distance in reverse order and get a negative value in return (but only since c++11).

[jhenderson]

llvm/include/llvm/Object/MutableELFObject.h

128–130

You missed this bit:

Also "this returns the index one past the last entry" to be more precise.

133

uint64_t -> size_t change should be made to Part 1 D64281.

+1 to this

191–196

If size() is not used, get rid of it and update the getEndIndex() comment.

llvm/unittests/Object/MutableELFObjectTest.cpp

28

You're both right. My bad.

[rupprecht]

llvm/unittests/Object/MutableELFObjectTest.cpp

508–510

These checks should also use the same pattern above (checking the whole list of section names), with the common bits extracted to a utility method.

(A gmock matcher would be nice, but the API to write a new matcher is insanely hard to get right).

[abrachet]

Addressed comments

[abrachet]

I've been using ::testing::ElementsAre(...) against a vector of StringRef a lot, when it fails the output is really bad. Like this: element #1 is equal to { '.' (46, 0x2E), 's' (115, 0x73), 'e' (101, 0x65), 'c' (99, 0x63), '0' (48, 0x30) }, Is there a way to provide formatting for types to gtest? Or should I just be using const char *?

llvm/unittests/Object/MutableELFObjectTest.cpp

28

Wow I never knew that

130–151

Should I just remove all of this?

[labath]

In D65633#1638673, @abrachet wrote:

I've been using ::testing::ElementsAre(...) against a vector of StringRef a lot, when it fails the output is really bad. Like this: element #1 is equal to { '.' (46, 0x2E), 's' (115, 0x73), 'e' (101, 0x65), 'c' (99, 0x63), '0' (48, 0x30) }, Is there a way to provide formatting for types to gtest? Or should I just be using const char *?

Yeah, there is. It involves writing either an operator<< or a PrintTo function. The StringRef class already has an operator<<, but the problem here is that gtest wants to print it as a container instead of using that operator<<. I don't know if there's anything we can do here, but @sammccall might. If it bothers you too much you can use a vector<string> on the LHS of the match ( i wouldn't use const char * because I am not sure if the returned StringRefs are guaranteed to be null terminated, and its best to not rely on that).

[sammccall]

In D65633#1638796, @labath wrote:

In D65633#1638673, @abrachet wrote:

I've been using ::testing::ElementsAre(...) against a vector of StringRef a lot, when it fails the output is really bad. Like this: element #1 is equal to { '.' (46, 0x2E), 's' (115, 0x73), 'e' (101, 0x65), 'c' (99, 0x63), '0' (48, 0x30) }, Is there a way to provide formatting for types to gtest? Or should I just be using const char *?

Yeah, there is. It involves writing either an operator<< or a PrintTo function. The StringRef class already has an operator<<, but the problem here is that gtest wants to print it as a container instead of using that operator<<. I don't know if there's anything we can do here, but @sammccall might.

Right, the sequence is:

• PrintTo() is found by overload resolution, if there's no specialized version, it falls back to the generic template
• the generic template checks whether it's a container (by looking for nested iterator and const_iterator types)
• for non-containers, it attempts to use operator<< if it exists, falling back to "n-byte object XX-XX-XX-XX" otherwise
• for containers, it prints each element in the same manner

So overloading operator<< doesn't bypass the container detection, but overloading PrintTo() does. And this is what gtest does for std::string, in gtest-printers.h. I think we should do the same as a local modification to internal/custom/gtest-printers.h.

If it bothers you too much you can use a vector<string> on the LHS of the match ( i wouldn't use const char * because I am not sure if the returned StringRefs are guaranteed to be null terminated, and its best to not rely on that).

This has bothered at various times, but I never really investigated how to fix it. I'll try to put together a patch.

[jhenderson]

llvm/include/llvm/Object/MutableELFObject.h

154–155

This comment needs updating.

llvm/unittests/Object/MutableELFObjectTest.cpp

129–130

Is this needed now that you have the following lines?

130–151

It looks to me to be largely redundant now (though you should still keep the "change the section name in the middle" bit).

489–491

Do you need this, given you have the previous line?

503–505

Do you need this?

513–515

Do you need this?

[sammccall]

In D65633#1638835, @sammccall wrote:

In D65633#1638796, @labath wrote:

In D65633#1638673, @abrachet wrote:

I've been using ::testing::ElementsAre(...) against a vector of StringRef a lot, when it fails the output is really bad. Like this: element #1 is equal to { '.' (46, 0x2E), 's' (115, 0x73), 'e' (101, 0x65), 'c' (99, 0x63), '0' (48, 0x30) }, Is there a way to provide formatting for types to gtest? Or should I just be using const char *?

Yeah, there is. It involves writing either an operator<< or a PrintTo function. The StringRef class already has an operator<<, but the problem here is that gtest wants to print it as a container instead of using that operator<<. I don't know if there's anything we can do here, but @sammccall might.

This has bothered at various times, but I never really investigated how to fix it. I'll try to put together a patch.

D66520 addresses this.

[rupprecht]

llvm/unittests/Object/MutableELFObjectTest.cpp

22–23

Is T ever going to be something besides StringRef? If not, removing that template parameter may be better.

132–136

Can SectionNames be inlined to avoid variable reuse?

EXPECT_THAT(collect<StringRef>(MutableObject.sections(), getSectionName),
            ::testing::ElementsAre("", ".sec0", ".sec1", ".sec2", ".symtab",
                                   ".strtab", ".shstrtab"));

[abrachet]

Addressed comments

[jhenderson]

No more comments from me. LGTM.

llvm/include/llvm/Object/MutableELFObject.h

template <typename ELFT> class MutableELFObject : public ELFObjectFile<ELFT> {
   /// 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.
+  /// a New mapping, the index is into the NewValues vector. A Removed mapping
+  /// means that an entry has been removed and should be skipped over when
+  /// iterating over the MutableTable. Notably removed values stay in the table
+  /// to preserve the index of all other other entries, similarly insertion is
+  /// not supported.
   ///
   /// 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 };
+      enum MappedType { Original, New, Removed };
 
       uint64_t Index;
       MappedType Type;
 
       MappingType(uint64_t Index, MappedType Type) : Index(Index), Type(Type) {}
 
       operator uint64_t() const { return Index; }
     };
explicit MutableTable(ArrayRef<OrigType> OriginalValues)
                      });
     }
 
     /// Get the OrigType at index Index regardless of whether it is an OrigType
     /// or NewType. In the case that Mappings[Index].Type == New, call NewTypes
     /// operator OrigType to make the proper conversion.
     const OrigType &operator[](uint64_t Index) const {
       assert(Index < Mappings.size() && "Out of bounds");
+      assert(Mappings[Index].Type != MappingType::Removed);
       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(uint64_t Index) const {
       assert(Index < OriginalValues.size() && "Out of bounds");
+      assert(Mappings[Index].Type != MappingType::Removed);
       return OriginalValues[Index];
     }
 
+    /// Gets the next non remove index after \param CurrentIndex.
+    uint64_t getNextIndex(uint64_t CurrentIndex) const {
+      // <= so that Index can reach an end iterator state.
+      while (Mappings[++CurrentIndex].Type == MappingType::Removed &&
+             CurrentIndex <= Mappings.size())
+        ;
+      return CurrentIndex;
+    }
+
+    /// Returns the first index which has not been removed.
+    uint64_t getFirstIndex() const {
+      for (uint64_t I = 0; I != Mappings.size(); ++I)
+        if (Mappings[I].Type != MappingType::Removed)
+          return I;
+      return Mappings.size();
+    }
+
+    /// Counts the number of non removed entries between 0 and \param Index.
+    uint64_t getRelativeIndex(uint64_t Index) const {
+      uint64_t Ret = 0;
+      for (uint64_t I = 0; I < Index; ++I)
+        if (Mappings[I].Type != MappingType::Removed)
+          ++Ret;
+      return Ret;
+    }
+
+    /// Returns the index of the last element. This is different than size
+    /// because size returns the number of valid (non removed) entries.
+    size_t getLastIndex() const { return Mappings.size(); }
+
+    /// 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;
+    }
+
     /// 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)
         return NewValues[Mappings[Index]];
       NewValues.emplace_back(Arguments...);
       Mappings[Index] = MappingType(NewValues.size() - 1, MappingType::New);
       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(uint64_t Index) const {
       assert(Index < Mappings.size() && "Out of bounds");
+      assert(Mappings[Index].Type != MappingType::Removed);
       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 size() const {
+      return llvm::count_if(Mappings, [](MappingType &Mapping) {
+        return Mapping.Type != MappingType::Removed;
+      });
+    }
 
     size_t originalSize() const { return OriginalValues.size(); }
   };
 
   using Elf_Shdr = Elf_Shdr_Impl<ELFT>;
   using Elf_Ehdr = Elf_Ehdr_Impl<ELFT>;
   using Elf_Sym = Elf_Sym_Impl<ELFT>;
 
   using MutableSymbolTable = MutableTable<Elf_Sym, MutableELFSymbol<ELFT>>;
template <typename ELFT> class MutableELFObject : public ELFObjectFile<ELFT> {
   /// sections are iterated with their index, not address in the file, which
   /// 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;
   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;
   }
 
   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)),
         Symbols(findSymbolTable(ELF::SHT_SYMTAB)),
         DynSymbols(findSymbolTable(ELF::SHT_DYNSYM)) {}
 
   section_iterator section_begin() const override {
-    return section_iterator(SectionRef(toDataRef(0), this));
+    return section_iterator(
+        SectionRef(toDataRef(Sections.getFirstIndex()), this));
   }
 
   section_iterator section_end() const override {
-    return section_iterator(SectionRef(toDataRef(Sections.size()), this));
+    return section_iterator(
+        SectionRef(toDataRef(Sections.getLastIndex()), this));
   }
 
   basic_symbol_iterator symbol_begin() const override {
-    return basic_symbol_iterator(
-        SymbolRef(toDataRef(findSectionOfType(ELF::SHT_SYMTAB), 0), this));
+    return basic_symbol_iterator(SymbolRef(
+        toDataRef(findSectionOfType(ELF::SHT_SYMTAB), Symbols.getFirstIndex()),
+        this));
   }
 
   basic_symbol_iterator symbol_end() const override {
     return basic_symbol_iterator(SymbolRef(
-        toDataRef(findSectionOfType(ELF::SHT_SYMTAB), Symbols.size()), this));
+        toDataRef(findSectionOfType(ELF::SHT_SYMTAB), Symbols.getLastIndex()),
+        this));
   }
 
   elf_symbol_iterator dynamic_symbol_begin() const override {
     return basic_symbol_iterator(
-        SymbolRef(toDataRef(findSectionOfType(ELF::SHT_DYNSYM), 0), this));
+        SymbolRef(toDataRef(findSectionOfType(ELF::SHT_DYNSYM),
+                            DynSymbols.getFirstIndex()),
+                  this));
   }
 
   elf_symbol_iterator dynamic_symbol_end() const override {
-    return basic_symbol_iterator(SymbolRef(
-        toDataRef(findSectionOfType(ELF::SHT_DYNSYM), DynSymbols.size()),
+    return basic_symbol_iterator(
+        SymbolRef(toDataRef(findSectionOfType(ELF::SHT_DYNSYM),
+                            DynSymbols.getLastIndex()),
                   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,
public:
   Expected<MutableELFSection<ELFT> &> getMutableSection(SectionRef Sec);
 
   /// 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);
   }
+
+  /// 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);
+  }
+
+  /// Removes a symbol.
+  void removeSymbol(symbol_iterator Sym) { removeSymbol(*Sym); }
+
+  /// Removes a section.
+  void removeSection(SectionRef Sec) {
+    Sections.remove(Sec.getRawDataRefImpl().p);
+  }
+
+  /// Removes a section.
+  void removeSection(section_iterator Sec) { removeSection(*Sec); }
 };
 
 template <typename ELFT>
 void MutableELFObject<ELFT>::moveSectionNext(DataRefImpl &Sec) const {
-  ++Sec.p;
+  Sec.p = Sections.getNextIndex(Sec.p);
 }
 
 template <typename ELFT>
 uint64_t MutableELFObject<ELFT>::getSectionIndex(DataRefImpl Sec) const {
-  return Sec.p;
+  return Sections.getRelativeIndex(Sec.p);
 }
 
 template <typename ELFT>
 Expected<StringRef>
 MutableELFObject<ELFT>::getSectionName(DataRefImpl Sec) const {
   if (const MutableELFSection<ELFT> *SecOrNull = Sections.getConstIfNew(Sec.p))
     return SecOrNull->Name;
   return ELFObjectFile<ELFT>::getSectionName(Sec);
 }
 
 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>
+void MutableELFObject<ELFT>::moveSymbolNext(DataRefImpl &Sym) const {
+  Sym.d.b = Symbols.getNextIndex(Sym.d.b);
+}
+
+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

 #include "llvm/Support/YAMLTraits.h"
 #include "llvm/Testing/Support/Error.h"
 #include "gtest/gtest.h"
 
 using namespace llvm;
 using namespace object;
 using namespace yaml;
 
 // Test that when no modifications have been made SectionRef's methods work
 // the same in both ELFObjectFile and MutableELFObject.
 TEST(MutableELFObject, NoChange) {
   StringRef Yaml = R"(
 --- !ELF
 FileHeader:
   Class:     ELFCLASS64
   Data:      ELFDATA2LSB
   Type:      ET_REL
   Machine:   EM_X86_64
 Sections:
   - Name:    .text
     Type:    SHT_PROGBITS
     Flags:   [ SHF_ALLOC, SHF_EXECINSTR ]
     Content: "DEADBEEF")";
 
   SmallString<0> Storage;
   raw_svector_ostream OS(Storage);
Sections:
   auto compareSectionName = [](section_iterator Iter, const char *Name) {
     StringRef SecName;
     EXPECT_FALSE(Iter->getName(SecName));
     EXPECT_EQ(SecName, Name);
   };
 
   ptrdiff_t NumSections =
       std::distance(MutableObject.section_begin(), MutableObject.section_end());
   ASSERT_EQ(NumSections, 7);
 
   auto Iter = MutableObject.section_begin();
   compareSectionName(Iter, nullptr);
 
   compareSectionName(++Iter, ".sec0");
   compareSectionName(++Iter, ".sec1");
   compareSectionName(++Iter, ".sec2");
 
   Iter = MutableObject.section_begin();
   std::advance(Iter, 2);
   auto MutSectionOrErr = MutableObject.getMutableSection(Iter);
   ASSERT_EQ(std::distance(MutableObject.section_begin(), Iter), 2);
   ASSERT_EQ(
       std::distance(MutableObject.section_begin(), MutableObject.section_end()),
       7);
   ASSERT_THAT_EXPECTED(MutSectionOrErr, Succeeded());
   MutSectionOrErr->Name = ".new_name";
 
   Iter = MutableObject.section_begin();
   compareSectionName(Iter, nullptr);
   compareSectionName(++Iter, ".sec0");
   compareSectionName(++Iter, ".new_name");
   compareSectionName(++Iter, ".sec2");
 }
 
 // Test MutableELFSection::setData().
 TEST(MutableELFObject, ChangeSectionContents) {
   SmallString<0> Storage;
   Expected<std::unique_ptr<ObjectFile>> ErrOrObj = yaml2ObjectFile(Storage, R"(
 --- !ELF
DynamicSymbols:
 
   auto DynSym = symbol_iterator(++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");
 }
+
+// Test removeSymbol method
+TEST(MutableELFObject, RemoveSymbols) {
+  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:     first
+    Index:    SHN_ABS
+    Value:    0x1234
+    Binding:  STB_LOCAL
+  - Name:     second
+    Index:    SHN_ABS
+    Value:    0
+    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));
+
+  auto Distance =
+      std::distance(MutableObject.symbol_begin(), MutableObject.symbol_end());
+  EXPECT_EQ(Distance, 3);
+
+  auto FirstSym = ++MutableObject.symbol_begin();
+  auto NameOrErr = symbol_iterator(FirstSym)->getName();
+  ASSERT_THAT_EXPECTED(NameOrErr, Succeeded());
+  EXPECT_EQ(*NameOrErr, "first");
+  MutableObject.removeSymbol(FirstSym);
+
+  auto NewFirstSym = ++MutableObject.symbol_begin();
+  auto NewNameOrErr = symbol_iterator(NewFirstSym)->getName();
+  ASSERT_THAT_EXPECTED(NewNameOrErr, Succeeded());
+  EXPECT_EQ(*NewNameOrErr, "second");
+
+  Distance =
+      std::distance(MutableObject.symbol_begin(), MutableObject.symbol_end());
+  EXPECT_EQ(Distance, 2);
+}
+
+// Test removeSection method
+TEST(MutableELFObject, RemoveSections) {
+  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
+Sections:
+  - Name:    .remove_me
+    Type:    SHT_PROGBITS
+  - Name:    .remove_me_too
+    Type:    SHT_PROGBITS)");
+
+  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()),
+      6);
+
+  auto Iter = ++MutableObject.section_begin();
+  StringRef Name;
+  ASSERT_FALSE(Iter->getName(Name));
+  EXPECT_EQ(Name, ".remove_me");
+
+  MutableObject.removeSection(Iter);
+
+  EXPECT_EQ(
+      std::distance(MutableObject.section_begin(), MutableObject.section_end()),
+      5);
+
+  // At this point Iter is on a removed index, it is invalid and any method
+  // should abort (from an assert). If for some reason tests are being run with
+  // assertions disabled, these ASSERTs should not be run.
+#ifndef NDEBUG
+  ASSERT_DEATH(Iter->getName(Name), ".*");
+  ASSERT_DEATH(Iter->getSize(), ".*");
+  ASSERT_DEATH(Iter->isData(), ".*");
+#endif
+
+  Iter = ++MutableObject.section_begin();
+  ASSERT_FALSE(Iter->getName(Name));
+  EXPECT_EQ(Name, ".remove_me_too");
+
+  MutableObject.removeSection(Iter);
+
+  EXPECT_EQ(
+      std::distance(MutableObject.section_begin(), MutableObject.section_end()),
+      4);
+
+  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");
+}