[PATCH] D48406: [ELF] Make non-writable non-executable PROGBITS sections closer to .text

[Fangrui Song via Phabricator via llvm-commits]

MaskRay marked 3 inline comments as done.
MaskRay added inline comments.


================
Comment at: lld/trunk/ELF/Writer.cpp:760-766
       Rank |= RF_WRITE;
+    // Make non-executable and non-writable PROGBITS sections (e.g .rodata
+    // .eh_frame) closer to .text . They likely contain PC or GOT relative
+    // relocations and there could be relocation overflow if other huge sections
+    // (.dynstr .dynsym) were placed in between.
+    else if (Sec->Type == SHT_PROGBITS)
+      Rank |= RF_PROGBITS_NOT_EXEC_OR_WRITE;
----------------
ruiu wrote:
> nit: I'd avoid writing a long comment before `else if` because it visually split a consecutive if ~ else if ~ else expressions. This is probably better:
> 
>   if (IsWriter) {
>     ...
>   } else if (...) {
>     // your comment here
>     your code here
>   }
> 
> That said, the best way to arrange this particular piece code is to replace `else { if` with `else if` like this
> 
>   if (IsExec) {
>     ...
>   } else if (IsWriter) {
>     ...
>   } else if (Sec->Type == SHT_PROGBITS) {
>     ...
>   }
Done in r335743


================
Comment at: lld/trunk/ELF/Writer.cpp:762
+    // Make non-executable and non-writable PROGBITS sections (e.g .rodata
+    // .eh_frame) closer to .text . They likely contain PC or GOT relative
+    // relocations and there could be relocation overflow if other huge sections
----------------
ruiu wrote:
> space before `.`
I tend to avoid immediate trailing dot when the last word is an identifier (OT: when I double click it, the trailing dot will be a part of the selection) or URL (slight OT: trailing punctuation is not friendly to some terminals). That said, I don't have a strong preference here so I'll follow yours.


Repository:
  rL LLVM

https://reviews.llvm.org/D48406