[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

75

entries, similarly -> entries. Similary

129

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

132

You need to assert that CurrentIndex is valid.

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

139

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

146

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

147

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

155–156

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

157

Perhaps call this getEndIndex()

337

Superfluous overload.

345

Superfluous overload.

llvm/unittests/Object/MutableELFObjectTest.cpp

397

Missing trailing full stop.

422

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

427

This shouldn't be auto.

438

Can this be done inline in the check?

442

Missing trailing full stop.

465

Probably requires a comment saying why "6".

478

"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

415

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.

430

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).

491

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

501–502

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

415

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

430

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

129

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.

131

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

132

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

152

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

430

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

481–482

added by yaml2obj

[abrachet]

Addressed review comments

[abrachet]

llvm/include/llvm/Object/MutableELFObject.h

131

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?

132

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

430

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

134–138

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

189–190

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

481–485

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

189–190

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

481–485

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

129–131

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.

134

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

189–190

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?

430

Exactly. Don't test asserts.

487

Match -> SectionsMatch

[labath]

llvm/unittests/Object/MutableELFObjectTest.cpp

481–485

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

189–190

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

134

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

189–190

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.

481–485

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

134

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

129–131

You missed this bit:

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

134

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

+1 to this

189–190

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

505–507

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

135–145

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

155–156

This comment needs updating.

llvm/unittests/Object/MutableELFObjectTest.cpp

135–136

Is this needed now that you have the following lines?

135–145

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

486–488

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

500–502

Do you need this?

510–512

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.

138–142

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

 //===-- MutableELFObject.h --------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_OBJECT_MUTABLEELFOBJECT_H
 #define LLVM_OBJECT_MUTABLEELFOBJECT_H
 
 #include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/Sequence.h"
 #include "llvm/Object/ELFObjectFile.h"
 
 namespace llvm {
 namespace object {
 
 template <typename ELFT> class MutableELFObject;
 
 /// This class is used by MutableELFObject to provide a mutable version of an
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 };
 
       size_t Index;
       MappedType Type;
 
       MappingType(size_t Index, MappedType Type) : Index(Index), Type(Type) {}
 
       operator size_t() const { return Index; }
     };
explicit MutableTable(ArrayRef<OrigType> TheOriginalValues)
                       });
     }
 
     /// 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[](size_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(size_t Index) const {
       assert(Index < OriginalValues.size() && "Out of bounds");
+      assert(Mappings[Index].Type != MappingType::Removed);
       return OriginalValues[Index];
     }
 
+    /// Gets the index of the next entry that hasn't been removed after \param
+    /// CurrentIndex. If there is no entry after \param CurrentIndex which has
+    /// not been removed, this returns the index one past the last entry.
+    size_t getNextIndex(size_t CurrentIndex) const {
+      assert(CurrentIndex < Mappings.size());
+      return *llvm::find_if(
+          seq(CurrentIndex + 1, Mappings.size()), [this](size_t Index) {
+            return Mappings[Index].Type != MappingType::Removed;
+          });
+    }
+
+    /// Returns the first index which has not been removed.
+    size_t getFirstIndex() const {
+      return Mappings[0].Type != MappingType::Removed ? 0 : getNextIndex(0);
+    }
+
+    /// Counts the number of entries that haven't been removed between 0 and
+    /// \param Index.
+    size_t getEffectiveIndex(size_t Index) const {
+      size_t Ret = 0;
+      for (size_t I = 0; I < Index; ++I)
+        if (Mappings[I].Type != MappingType::Removed)
+          ++Ret;
+      return Ret;
+    }
+
+    /// Returns the index of the last element.
+    size_t getEndIndex() const { return Mappings.size(); }
+
+    /// Removes the entry at \param Index.
+    void remove(size_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(size_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]];
       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");
+      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 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);
   }
 
+  /// moveSectionNext must be overriden because the section_iterators in
+  /// MutableELFObject work differently than in ELFObjectFile. In this class,
+  /// 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 &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)),
         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.getEndIndex()), 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.getEndIndex()),
+        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()),
+        toDataRef(findSectionOfType(ELF::SHT_DYNSYM), DynSymbols.getEndIndex()),
         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();
public:
   /// @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);
   }
+
+  /// Removes a symbol.
+  void removeSymbol(SymbolRef Sym) {
+    assert(Sections.getOriginal(Sym.getRawDataRefImpl().d.a).sh_type ==
+               ELF::SHT_SYMTAB &&
+           "Not pointing to symbol table");
+    assert(Sym.getRawDataRefImpl().d.b && "Cannot remove index 0 symbol");
+    Symbols.remove(Sym.getRawDataRefImpl().d.b);
+  }
+
+  /// Removes a section.
+  void removeSection(SectionRef Sec) {
+    assert(Sec.getRawDataRefImpl().p && "Cannot remove index 0 section");
+    Sections.remove(Sec.getRawDataRefImpl().p);
+  }
 };
 
 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.getEffectiveIndex(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 {
+  const MutableSymbolTable &SymbolTable = getSymbolTable(Sym);
+  Sym.d.b = SymbolTable.getNextIndex(Sym.d.b);
+}
+
+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);
 }

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;
 
+template <typename Range>
+std::vector<StringRef> collect(
+    Range TheRange,
+    function_ref<StringRef(const decltype(*TheRange.begin()) &)> Extractor =
+        [](auto It) { return *It; }) {
+  std::vector<StringRef> Ret;
+  llvm::transform(TheRange, std::back_inserter(Ret), Extractor);
+  return Ret;
+}
+
+StringRef getSectionName(const SectionRef &Sec) {
+  Expected<StringRef> NameOrErr = Sec.getName();
+  if (!NameOrErr) {
+    consumeError(NameOrErr.takeError());
+    return "Couldn't get section name";
+  }
+  return *NameOrErr;
+}
+
 // 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
- Name:    .sec1
     Type:    SHT_PROGBITS
   - Name:    .sec2
     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));
 
-  auto compareSectionName = [](section_iterator Iter, const char *Name) {
-    auto NameOrErr = Iter->getName();
-    ASSERT_THAT_EXPECTED(NameOrErr, Succeeded());
-    EXPECT_EQ(*NameOrErr, Name);
-  };
-
-  ptrdiff_t NumSections =
-      std::distance(MutableObject.section_begin(), MutableObject.section_end());
-  ASSERT_EQ(NumSections, 7);
-
-  auto Iter = MutableObject.section_begin();
-  compareSectionName(Iter, nullptr);
+  EXPECT_THAT(collect(MutableObject.sections(), getSectionName),
+              ::testing::ElementsAre("", ".sec0", ".sec1", ".sec2", ".symtab",
+                                     ".strtab", ".shstrtab"));
 
-  compareSectionName(++Iter, ".sec0");
-  compareSectionName(++Iter, ".sec1");
-  compareSectionName(++Iter, ".sec2");
-
-  Iter = MutableObject.section_begin();
+  section_iterator 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");
+  EXPECT_THAT(collect(MutableObject.sections(), getSectionName),
+              ::testing::ElementsAre("", ".sec0", ".new_name", ".sec2",
+                                     ".symtab", ".strtab", ".shstrtab"));
 }
 
 // Test MutableELFSection::setData().
 TEST(MutableELFObject, ChangeSectionContents) {
   SmallString<0> Storage;
   Expected<std::unique_ptr<ObjectFile>> ErrOrObj = yaml2ObjectFile(Storage, R"(
 --- !ELF
 FileHeader:
DynamicSymbols:
 
   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");
 }
+
+// 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
+DynamicSymbols:
+  - Name:     a
+  - Name:     b)");
+
+  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);
+
+  EXPECT_EQ(std::distance(MutableObject.dynamic_symbol_begin(),
+                          MutableObject.dynamic_symbol_end()),
+            3);
+
+  auto FirstSym = ++MutableObject.symbol_begin();
+  Expected<StringRef> 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");
+
+  EXPECT_EQ(
+      std::distance(MutableObject.symbol_begin(), MutableObject.symbol_end()),
+      2);
+  EXPECT_EQ(std::distance(MutableObject.dynamic_symbol_begin(),
+                          MutableObject.dynamic_symbol_end()),
+            3);
+
+  MutableObject.removeSymbol(*NewFirstSym);
+  EXPECT_EQ(
+      std::distance(MutableObject.symbol_begin(), MutableObject.symbol_end()),
+      1);
+  EXPECT_EQ(std::distance(MutableObject.dynamic_symbol_begin(),
+                          MutableObject.dynamic_symbol_end()),
+            3);
+}
+
+// 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_THAT(collect(MutableObject.sections(), getSectionName),
+              ::testing::ElementsAre("", ".remove_me", ".remove_me_too",
+                                     ".symtab", ".strtab", ".shstrtab"));
+
+  auto Iter = ++MutableObject.section_begin();
+  Expected<StringRef> NameOrErr = Iter->getName();
+  ASSERT_THAT_EXPECTED(NameOrErr, Succeeded());
+  EXPECT_EQ(*NameOrErr, ".remove_me");
+  MutableObject.removeSection(*Iter);
+
+  EXPECT_THAT(collect(MutableObject.sections(), getSectionName),
+              ::testing::ElementsAre("", ".remove_me_too", ".symtab", ".strtab",
+                                     ".shstrtab"));
+
+  Iter = ++MutableObject.section_begin();
+  MutableObject.removeSection(*Iter);
+
+  EXPECT_THAT(collect(MutableObject.sections(), getSectionName),
+              ::testing::ElementsAre("", ".symtab", ".strtab", ".shstrtab"));
+
+  Iter = MutableObject.section_begin();
+  EXPECT_EQ(Iter->getIndex(), 0u);
+  ++Iter;
+  EXPECT_EQ(Iter->getIndex(), 1u);
+  ++Iter;
+  EXPECT_EQ(Iter->getIndex(), 2u);
+  ++Iter;
+  EXPECT_EQ(Iter->getIndex(), 3u);
+  ++Iter;
+  EXPECT_EQ(MutableObject.section_end(), Iter);
+}