[PATCH] D64281: [Object] Create MutableObject class for doing Object Mutations [Part 1]

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/Object/MutableObject.h:68
+
+  MutableObject(const ObjectFile &ObjFile,
+                SectionList::VecType OriginalSections,
----------------
abrachet wrote:
> jhenderson wrote:
> > Why can't the MutableObject just query the ObjectFile for its sections etc? Note that ObjectFile provides section_begin and section_end iterators.
> > 
> > Also, I'd be very wary of adding Symbols as a global list. It's technically valid for an ELF to have multiple SHT_SYMTAB instances, although in practice only one of these will be considered by tools as "the symbol table" in the ELF. It probably makes more sense for these to be owned by a symbol table class, and then for the MutableObject to have a reference to that class, which it manipulates.
> > 
> > Finally, Segments are not a type in the ObjectFile class, because they aren't a universal concept (ELF and Mach-O have them, but not COFF for example, I believe). I'm okay with this being specific to ELF for now, but it needs to be designed such that a common base class can be added later to handle the other formats (similar to how there is ObjectFile, ELFObjectFile etc). Perhaps you should just rename this class to MutableELFObject now, to avoid confusion.
> > Note that ObjectFile provides section_begin and section_end iterators.
> Thanks so much @jhenderson, this is a much better way to do this! I do have some questions about the best way to go about this. I'll will just talk about sections here, a SectionRef is just a DataRefImpl, for ELF its fine to use a DataRefImpl that points to an Elf_Shdr outside of the file because its getSection method just uses it as a pointer and does no checking if that pointer exists in the files mapped address space. Which is great, it means that I can easily implement this for ELF sections because I can have a MutableRange over SectionRef's which is what ObjectFile::sections() iterates over, and I can extend SectionRef to an OwnableSectionRef or similar. But not all the ObjectFile sub classes are this easy. I only looked at COFF and MachO, COFF is the same as ELF, but MachO's getSection(DataRefImpl) uses it as an offset into its sections, so without changing how its internal getSection works, I'm not sure the best way to do this.
> SectionRef is just a DataRefImpl
This isn't quite true. A SectionRef is a DataRefImpl assoicated with an owning ObjectFile. If you were to construct new SectionRefs within MutableObject, you could maybe have them associated with some new ObjectFile instance, possibly MutableObject (versions of which might want to inherit from ELFObjectFile, MachOObjectFile etc) itself, and add them to the MutableRange like that. This should work regardless of the file format. Existing SectionRefs would continue to point at the original object file.


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

https://reviews.llvm.org/D64281