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

[George Rimar via llvm-commits]

grimar added inline comments.

================
Comment at: ELF/Writer.cpp:379
@@ -378,1 +378,3 @@
 
+  bool AIsData = A->getName() == ".data";
+  bool BIsData = B->getName() == ".data";
----------------
rafael wrote:
> This needs a comment and should probably be more explicit. Something like
> 
> bool AIsRelRo = helperfunc(...)
> bool BIsRelRo = helperfunc(...)
> if (AIsRelRo != BIsRelRo)
>   return AIsRelRo;
> 
> 
Done.

================
Comment at: ELF/Writer.cpp:739
@@ -732,1 +738,3 @@
       } else {
+        if (Config->ZRelro && !GnuRelroPhdr.p_filesz) {
+          // If section is .data or .bss, we should calculate the current
----------------
ruiu wrote:
> I don't really want to make this for-loop longer than that is now. This is getting hard to understand over time. Can you create another for loop and move this piece of code outside of the loop?
Adding another loop would add too much of code and too much duplication. Bcos I still need to calculate VA. I moved that logic to external method instead.

================
Comment at: ELF/Writer.cpp:744
@@ +743,3 @@
+          // read-only but .got.plt will be writeable.
+          bool CloseGnuRo = (Sec == Out<ELFT>::GotPlt && !Config->ZNow) ||
+                            (Sec == Out<ELFT>::Bss) ||
----------------
rafael wrote:
> You should be able to write this as
> 
>  if (cur is rw and sec is relro)
>   Add sec to relro segment.
> 
> The "is relro" check should use the same helper function that is used in the sorting.
Done.


http://reviews.llvm.org/D14218