[PATCH] D63185: [llvm-objcopy] [WIP] Librarify llvm-objcopy

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

You're going to need to account for program headers in your layout code. If you take a look at llvm-objcopy, a lot of the layout work is done based on those, rather than the sections, because sections within segments should move with their segment.



================
Comment at: llvm/tools/llvm-objcopy/MutableObject/MutableObject.h:38
+  virtual SectionBase *clone(Optional<StringRef> NewName = None) const {
+    auto Ptr = new SectionBase(*this);
+    if (NewName)
----------------
You should consider using smart pointers rather than new. Probably a unique_ptr is sufficient.


================
Comment at: llvm/tools/llvm-objcopy/MutableObject/MutableObject.h:44
+
+  virtual SectionBase *clone(ArrayRef<uint8_t> NewData) const {
+    auto Ptr = new SectionBase(*this);
----------------
Maybe "clone" isn't the right name for this function? It's doing more than cloning.


================
Comment at: llvm/tools/llvm-objcopy/MutableObject/MutableObject.h:324
+template <typename ELFT>
+Error MutableELFObject<ELFT>::createNewShstrtab() {
+  std::vector<uint8_t> NewData {0};
----------------
The MC library already has a StringTableBuilder class that does a lot of this work, so you should probably use that, rather than writing things from scratch.


================
Comment at: llvm/tools/llvm-objcopy/MutableObject/MutableObject.h:383-384
+
+  Header.e_shnum = NumSections;
+  Header.e_shoff = CurrentOffset;
+
----------------
It probably doesn't make sense for the elf header to be updated in this method. That should be derivable right before writing the header to disk. Possible exception is that we need the section header offset somewhere.


================
Comment at: llvm/tools/llvm-objcopy/MutableObject/MutableObject.h:390
+template <typename ELFT>
+Error MutableELFObject<ELFT>::writeToDisk(objcopy::Buffer &Buff) {
+  auto ConstHeader = Object.getELFFile()->getHeader();
----------------
writeToDisk is probably the wrong name for a function that writes to a buffer... ;)


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

https://reviews.llvm.org/D63185