Addressed review comments

Fixed failing test for std::distance call.

Made a virtual getHeader method in ELFObjectFile

Addressed review comments

Addressed review comments

Addressed comments

Addressed review comments

Addressed review comments

Addressed review comments

Rebase

rebase

Addressed review comments

Addressed review comments

Moved remove and add out of this patch

Addressed review comments

Addressed review comments

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

Remove removeDynSymbol from MutableELFObject

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

Added doxygen comments to private interface. NFC

Thanks for the comments @grimar!

Addressed review comments

Create a new ObjectFile in unit test

In D64281#1619189, @jhenderson wrote:

Aargh, you pushed your latest update whilst I was doing my last review, so I can't see what are the latest changes!

added a better overload to public methods which take a SectionRef so they can be used in for-each loops.

What is this bit referring to?

Addressed review comments

Realized from the code example in doxygen comment that it was very unergonomic, added a better overload to public methods which take a SectionRef so they can be used in for-each loops.

In D64281#1617212, @jhenderson wrote:

Since you're introducing a new class in a public header, you should write doxygen comments for at least the public/protected interface, and quite possibly any private bits too. I think if you are careful with those comments too, it will help explain the design of the classes.

I've not looked at the unit tests today. I'll do that tomorrow. The interface looks a lot cleaner than it did when I last looked at this. Hopefully adding comments will make it much clearer what's going on. If you make MutableTable a sub-class, then my concerns about the leaky implementation (i.e. needing to decide between fetching the section properties from the table or the base version) are significantly lessened.

Addressed review comments

Fixed failing test cases

My preference would be to use std::distance(begin, end) for O(N) size computations to make it clear that they're "interesting", as I mentioned in the original review/discussion.

This makes a lot of sense to me.

In D65255#1616820, @gribozavr wrote:

Sorry, this patch broke tests: http://lab.llvm.org:8011/builders/clang-x86_64-debian-fast/builds/15993, I reverted it in r368035.

Have you rebased recently? I feel like the content in yaml2macho.cpp may have moved to the ObjectYAML library recently.

I hadn't actually done that yet but just did so in rL368021

In D65788#1616378, @jhenderson wrote:

How many places use this and in what context? I'm conscious of std::size which becomes available in C++17, and the natural implementation of that is to simply use the standard .size() method on a container (see https://en.cppreference.com/w/cpp/iterator/size). It also does something else for fixed-sized arrays, but this particular version doesn't work for that. Finally, why do you need llvm::size() instead of a container.size() method?

Use new MutableTable. Add support for deletion.

Fixed the reason for needing to make a member mutable

Complete overhaul of how MutableRange works. It is now called MutableTable which seemed more fitting. It has a much cleaner interface for MutableELFObject while maintaining low overhead.

I haven't updated all ignored instances of fs::remove, I therefore can't mark it LLVM_NODISCARD for now.

I think I remember reading that casting a [[nodiscard]] functions return to void was broken on some compilers, ie still warns. You cast fs::remove to void a lot here which is a great way to show intent (I wish we mentioned this in the style guide) but if I'm not just making this up, it might not be worth the trouble to give fs::remove the nodiscard attribute. Maybe something to look into if you ever do this.

rebase

Removed unnecessary LLVM_LINK_COMPONENTS in yaml2obj

Again responding to your comment @jhenderson but not quoting due to length.

Fixed silly mistake

Addressed review comments.

Fixed link error in LLVMBuild.txt not CMakeLists.txt

(won't quote because of length but I'll tag you so you know I am responding) @jhenderson
That does work, and was what I was thinking of before. Also I quite like keeping pointers so it just works with the methods in the base class. But I'm still not sure what type T should be. If we use sections as an example, when changing the name it isn't possible (or feasible, I suppose that sh_name could be a pointer (not into the string table but a pointer into the processes memory) for modified sections). Right now, the original type is just an Elf_Shdr, or rather the mapping type points to one, but for a modified section, it indexes into the vector where to find the T, which is a wrapper around an Elf_Shdr but for example for its name it has a StringRef so it can be easily changed. This is how llvm-objcopy does it, but it copies every single section whether or not they will ever be modified. I agree that it is ugly for the users of the class to ever have to deal with the MappingType, it isn't ideal, but I don't know how to do this unless we copy every single one like is done in llvm-objcopy, and at that point I would suggest the Section type just have an Elf_Shdr * to the original and just keep a vector of Sections.

fixed link issue

rebase

Addressed review comments

Addressed review comments

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

Added unit tests and increased functionality.