[PATCH] D64281: [Object] Create MutableELFObject Class for Doing Mutations on ELFObjectFiles [Part 1]

[Alex Brachet via Phabricator via llvm-commits]

abrachet marked 5 inline comments as done.
abrachet added inline comments.


================
Comment at: llvm/include/llvm/Object/ELFObjectFile.h:801
   const Elf_Shdr *EShdr = getSection(Sec);
-  return !isBerkeleyText(Sec) && EShdr->sh_type != ELF::SHT_NOBITS &&
-         EShdr->sh_flags & ELF::SHF_ALLOC;
+  return !ELFObjectFile<ELFT>::isBerkeleyText(Sec) &&
+          EShdr->sh_type != ELF::SHT_NOBITS &&
----------------
jhenderson wrote:
> Are you sure you want to be calling the base-class method though? Isn't that going to go wrong if Sec is a modified or added section?
When these functions are called, they get called with a DataRef pointing to the Elf_Shdr whether they are new or not. I was segfaulting here before because in `ELFObjectFile` the DataRef's point to the Elf_Shdr but in `MutableELFObject` they are indexes into the `MutableRange`. I couldn't find a way for the DataRefs to be interpreted the same way between the two classes. This isn't ideal though.


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:20
+// taking an Iterable::value_type.
+template <typename T> class MutableRange {
+public:
----------------
jhenderson wrote:
> I've been thinking about how this class is used by its clients. To me, it doesn't seem great that clients have to know whether a given member is new or not (i.e. whether it is stored in NewValues etc). I think it's okay to provide makeMutable to get a mutable reference to one of the original members, but I also feel like getNew should not be in the public interface. Instead, you should provide a function that can take a key or something to look things up with, possibly a MappingType, and return the corresponding item as a const variable.
I had something like this originally I think, and I agree that right now it isn't very ergonomic. The problem is that the original and new entries are of completely different types. I haven't found a great way to do this. What I don't do right now but might be better is require that the new type, T have a conversion operator. But no matter what, there needs to be a public method to get a pointer to the new type. This would require that MutableRange also have a template parameter to the original type it is pointing to (unless the conversion was to uintptr_t i suppose).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64281/new/

https://reviews.llvm.org/D64281