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

[Alex Brachet via Phabricator via llvm-commits]

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


================
Comment at: llvm/include/llvm/Object/MutableObject.h:68
+
+  MutableObject(const ObjectFile &ObjFile,
+                SectionList::VecType OriginalSections,
----------------
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.


================
Comment at: llvm/include/llvm/Object/MutableObject.h:74-75
+public:
+  static Expected<std::unique_ptr<MutableObject>>
+  createMutableObject(const ObjectFile &ObjFile);
+
----------------
jhenderson wrote:
> I don't understand why you need this. Why can't you have a public constructor?
Right, it could actually never fail because the ObjectFile is already valid so there's no point in this.


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

https://reviews.llvm.org/D64281