[PATCH] D16991: Split the creation of program headers in a few steps

[Rui Ueyama via llvm-commits]

ruiu added inline comments.

================
Comment at: ELF/Writer.cpp:53
@@ +52,3 @@
+    }
+    Elf_Phdr H;
+    OutputSectionBase<ELFT> *First = nullptr;
----------------
You can "Elf_Phdr H = {}" here, can't you?

================
Comment at: ELF/Writer.cpp:54-55
@@ +53,4 @@
+    Elf_Phdr H;
+    OutputSectionBase<ELFT> *First = nullptr;
+    OutputSectionBase<ELFT> *Last = nullptr;
+  };
----------------
I think I like this idea. I was thinking to make Phdr a list of output sections, but that was too flexible. This two-pointer approach is on the other hand directly reflects what we actually have in an ELF file.

================
Comment at: ELF/Writer.cpp:95
@@ +94,3 @@
+
+  // We create a a section for the elf header and one for the program headers.
+  const unsigned int NumDummySections = 2;
----------------
s/a a/a/
s/elf/ELF/

================
Comment at: ELF/Writer.cpp:96
@@ +95,3 @@
+  // We create a a section for the elf header and one for the program headers.
+  const unsigned int NumDummySections = 2;
+  ArrayRef<OutputSectionBase<ELFT> *> getSections() const {
----------------
I think we usually omit `int`.

================
Comment at: ELF/Writer.cpp:137
@@ -119,1 +136,3 @@
 
+  typedef typename ELFFile<ELFT>::uintX_t uintX_t;
+  OutputSectionBase<ELFT> ElfHeader("", 0, SHF_ALLOC);
----------------
Move this to beginning of the function.

================
Comment at: ELF/Writer.cpp:1198-1200
@@ -1183,1 +1197,5 @@
+template <class ELFT> void Writer<ELFT>::scanHeaders() {
+  auto AddHdr = [this](unsigned Type, unsigned Flags) -> Phdr & {
+    return *Phdrs.emplace(Phdrs.end(), Type, Flags);
+  };
 
----------------
You may want to return a pointer instead of a reference because in a few places in this function you wrote "&AddHdr".

================
Comment at: ELF/Writer.cpp:1253
@@ -1272,1 +1252,3 @@
 
+  // TLS header.
+  if (TlsHdr.First)
----------------
// Add the TLS segment unless it's empty.

================
Comment at: ELF/Writer.cpp:1284-1285
@@ +1283,4 @@
+template <class ELFT> void Writer<ELFT>::assignAddresses() {
+  // We know the final amount of program headers here.
+  scanHeaders();
+
----------------
I think this function call worth to be moved to Writer::run because now PHDRs construction and address assignments are nicely separated. You may want to rename scanHeaders to createPhdrs or something like that.

================
Comment at: ELF/Writer.cpp:1294-1295
@@ +1293,4 @@
+  SmallPtrSet<OutputSectionBase<ELFT> *, 4> PageAlign;
+  for (unsigned I = 0, N = Phdrs.size(); I != N; ++I) {
+    const Phdr &P = Phdrs[I];
+    if (P.H.p_type == PT_GNU_RELRO) {
----------------
Why don't you use a range-based for loop?

================
Comment at: ELF/Writer.cpp:1296-1302
@@ +1295,9 @@
+    const Phdr &P = Phdrs[I];
+    if (P.H.p_type == PT_GNU_RELRO) {
+      auto I = std::find(OutputSections.begin(), OutputSections.end(), P.Last);
+      ++I;
+      if (I != OutputSections.end() && needsPhdr(*I) &&
+          !((*I)->getFlags() & SHF_TLS))
+        PageAlign.insert(*I);
+    }
+    if (P.H.p_type != PT_LOAD)
----------------
Can you add a comment for this piece of code? It is not obvious.

================
Comment at: ELF/Writer.cpp:1324
@@ +1323,3 @@
+
+    if (needsPhdr<ELFT>(Sec)) {
+      // Don't allocate VA space for TLS NOBITS sections. The PT_TLS PHDR is
----------------
Why process only needsPhdr sections?

================
Comment at: ELF/Writer.cpp:1353
@@ +1352,3 @@
+    if (PHdr.First) {
+      auto *Last = PHdr.Last;
+      H.p_filesz = Last->getFileOff() - PHdr.First->getFileOff();
----------------
I wouldn't use auto here.

================
Comment at: ELF/Writer.cpp:1446
@@ -1414,2 +1445,3 @@
   // Write the program header table.
-  memcpy(Buf + EHdr->e_phoff, &Phdrs[0], Phdrs.size() * sizeof(Phdrs[0]));
+  uint8_t *HBuf = Buf + EHdr->e_phoff;
+  for (Phdr &P : Phdrs) {
----------------
Instead of uint8_t *, you can cast to Elf_Phdr *.


http://reviews.llvm.org/D16991