[PATCH] D14218: [ELF2] Implements -z relro: create an ELF "PT_GNU_RELRO" segment header in the object.

[Rui Ueyama via llvm-commits]

ruiu added inline comments.

================
Comment at: ELF/Driver.cpp:192
@@ -189,1 +191,3 @@
+    else if (S == "norelro")
+      Config->ZRelro = false;
   }
----------------
We probably should enable this by default, too.

================
Comment at: ELF/Writer.cpp:48
@@ -47,1 +47,3 @@
   void scanRelocs(const InputSection<ELFT> &C);
+  void updateRelRo(Elf_Phdr *Cur, Elf_Phdr *GnuRelroPhdr,
+                   OutputSectionBase<ELFT> *Sec, uintX_t VA);
----------------
You named isRelroSection and GnuRelroPhdr, so you want to name this updateRelro (instead of updateRelRo) for consistency.

================
Comment at: ELF/Writer.cpp:693
@@ +692,3 @@
+
+  if (!Config->ZRelro || !(Cur->p_flags & PF_W))
+    return;
----------------
  if (!Config->ZRelro || !(Cur->p_flags & PF_W) || isRelroSection(Sec))
    return;

================
Comment at: ELF/Writer.cpp:700
@@ +699,3 @@
+              VA - Cur->p_vaddr, 1 /*p_align*/);
+
+    GnuRelroPhdr->p_filesz = VA - Cur->p_vaddr;
----------------
Remove this blank line.

================
Comment at: ELF/Writer.cpp:803
@@ -769,1 +802,3 @@
 
+  if (GnuRelroPhdr.p_filesz) {
+    Elf_Phdr *PH = &Phdrs[++PhdrIdx];
----------------
So you are checking if p_filesz is not zero here to determine if we need the phdr, but

================
Comment at: ELF/Writer.cpp:839-840
@@ -800,2 +838,4 @@
     ++I;
+  if (Config->ZRelro)
+    ++I;
   return I;
----------------
Here you are using something different. Are they guaranteed to be the same?


http://reviews.llvm.org/D14218