[PATCH] D15423: [ELF] - Place RW sections that go after relro to another memory page.

[George Rimar via llvm-commits]

grimar added inline comments.

================
Comment at: ELF/Writer.cpp:847
@@ +846,3 @@
+                                OutputSectionBase<ELFT> *Sec) {
+  if (!Config->ZRelro || ((Cur->p_flags & PF_W) && isRelroSection(Sec)))
+    return false;
----------------
ruiu wrote:
> What does this condition mean? (Particularly, why do you have to check for PF_W?)
That this condition checks if we already ended proccessing the relro sections or not.
If !Config->ZRelro then we are not anyways. If we are in a writable segment and isRelroSection then we are still processing. 
updateRelro() also checks if we are still in a writable segment.
Thats just for additional safety I think. I guess using linkerscript we can place some of them to not writable segment for example (not sure for what), so its just checks that we are still doing what was supposed: preparing sections to be converted from W to R.

================
Comment at: ELF/Writer.cpp:849
@@ +848,3 @@
+    return false;
+  return GnuRelroPhdr->p_type;
+}
----------------
ruiu wrote:
> What's the meaning of this flag?
Flag has same use meaning as it used for in updateRelro(). If p_type is not set  then PT_GNU_RELRO header was not "updated" yet. What means we didn`t start to proccess the relro sections sequence. If it is set and we are here then it we are somewhere after relro.

================
Comment at: ELF/Writer.cpp:922
@@ -892,3 +921,3 @@
 
       if (Sec->getFlags() & SHF_TLS) {
         if (!TlsPhdr.p_vaddr)
----------------
ruiu wrote:
> Can you add new code here?
> 
>   if (/* Some expression to check if the last section is in RELRO but the current one is not */) {
>     VA = RoundUpToAlignment(VA, Target->getPageSize());
>     FileOff = RoundUpToAlignment(FileOff, Target->getPageSize());
>   }
> 
Done.


http://reviews.llvm.org/D15423