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

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added a comment.

LGTM with these changes.



================
Comment at: lld/trunk/ELF/Writer.cpp:704
   RF_EXEC = 1 << 13,
-  RF_NON_TLS_BSS = 1 << 12,
-  RF_NON_TLS_BSS_RO = 1 << 11,
-  RF_NOT_TLS = 1 << 10,
-  RF_ALLOC_FIRST = 1 << 9,
+  RF_PROGBITS_NOT_EXEC_OR_WRITE = 1 << 12,
+  RF_NON_TLS_BSS = 1 << 11,
----------------
I'd name this RF_RODATA for conciseness.


================
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;
----------------
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) {
    ...
  }


================
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
----------------
space before `.`


Repository:
  rL LLVM

https://reviews.llvm.org/D48406