[PATCH] D33964: [LLVM][llvm-objcopy] Added basic plumbing to get things started

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Looking good so far to me, apart from lots of small nits for the most part.

One general nit on comments - I believe that they should all end in full stops.



================
Comment at: tools/llvm-objcopy/Object.cpp:18
+  uint8_t *Buf = Out.getBufferStart();
+  Buf += sizeof(typename ELFT::Ehdr) + Index * sizeof(typename ELFT::Phdr);
+  typename ELFT::Phdr &Phdr = *reinterpret_cast<typename ELFT::Phdr *>(Buf);
----------------
Could we typedef these as well? It will read more easily, I think.


================
Comment at: tools/llvm-objcopy/Object.cpp:24
+  Phdr.p_vaddr = VAddr;
+  Phdr.p_paddr = VAddr; // TODO: add PAddr to Segment
+  Phdr.p_filesz = FileSize;
----------------
Any particular reason this TODO has been left undone?


================
Comment at: tools/llvm-objcopy/Object.cpp:165
+      return A->Addr < B->Addr;
+    return A->Index < B->Index;
+  };
----------------
I personally would think that using offset to determine order may be simpler, as it removes the need to test for SHF_ALLOC. Also, sections don't have to appear in the file in the order that they appear in the section header table, so I'd only use Index as a fall-back if you get two sections with the same offset.


================
Comment at: tools/llvm-objcopy/Object.cpp:171
+template <class ELFT> void Object<ELFT>::assignOffsets() {
+  // Decide file offsets and indexs
+  size_t PhdrSize = Segments.size() * sizeof(Elf_Phdr);
----------------
Typo -> "Decide file offsets and indexes"


================
Comment at: tools/llvm-objcopy/Object.cpp:173
+  size_t PhdrSize = Segments.size() * sizeof(Elf_Phdr);
+  // After the header and the program headers we can put section data.
+  uint64_t Offset = sizeof(Elf_Ehdr) + PhdrSize;
----------------
For clarity, I'd rephrase this comment as follows:
"We can put section data after the ELF header and the program headers."


================
Comment at: tools/llvm-objcopy/Object.cpp:177
+  for (auto &Section : Sections) {
+    // The segment can have a different alignment than the section. We need to
+    // make sure
----------------
We need to make sure of what? Perhaps "We need to check both are respected" or something to that effect.


================
Comment at: tools/llvm-objcopy/Object.cpp:189
+    Section->Offset = Offset;
+    Section->Index = Index;
+    if (Section->Type != SHT_NOBITS)
----------------
Section->Index = Index++; and then drop the later increment.


================
Comment at: tools/llvm-objcopy/Object.cpp:225
+  uint8_t *Buf = Out.getBufferStart();
+  typename ELFT::Ehdr &Ehdr = *reinterpret_cast<typename ELFT::Ehdr *>(Buf);
+  std::copy(Ident, Ident + 16, Ehdr.e_ident);
----------------
Use typedef.


================
Comment at: tools/llvm-objcopy/Object.h:31
+  llvm::StringRef Name;
+  Segment *ParrentSegment;
+  uint64_t HeaderOffset;
----------------
ParentSegment


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:73
+    report_fatal_error(EC.message());
+  // now if the program fails for any reason the output file will be deleted
+  Obj.write(*Buffer);
----------------
I think this comment is in the wrong place, or even unnecessary entirely.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.h:14
+#include "llvm/Support/Error.h"
+#include <system_error>
+
----------------
Unused header?


Repository:
  rL LLVM

https://reviews.llvm.org/D33964