Place .text.hot section before .text.

[tcwang]

Summary

Objects with profiling information can mark symbols with .hot prefix/suffix.
With the option -z keep-text-section-prefix, all the symbols marked as hot
will be put into a separate section named as '.text.hot' (as well as cold
symbols, (.text.unlikely), .text.startup, .text.exit, .text.unliekly, etc.)

The previous version of lld does not specify ordering for these text sections
and all the sections with prefix comes after .text section. This doesn't benefit
performance in some cases and is different than the ordering of gold. (In gold,
these secctions are ordered as .text.hot, .text, .text.startup, .text.exit,
.text.unlikely)

This commit reorders the text sections by putting .text.hot before .text, in
the same order as gold (only for text sections with prefix, other section orders
are still different than gold as previous version).

The change only affects linking results when -z keep-text-section-prefix is passed.

Unit test text-section-prefix is changed accordingly.

[pcc]

I don't understand why this ordering should always result in better performance. To the extent that it affects performance at all (e.g. on ARM a particular order might allow the linker to create fewer range extension thunks) it would seem to be arbitrary which order would be better for any given program.

[george.burgess.iv]

Chrome OS developed a dependency (perf, not functional) on things in .text.hot coming before things in .text. Namely, they have code that maps the first N MB of Chrome's .text section to hugepages, under the assumption that the code near the beginning of the binary will be hotter than the code near the end of the binary.

Naturally, this says nothing about how general programs will perform with this change. I similarly expect this to be more or less a wash for many programs.

I don't frequent lld often, but my 2c are that emulating this in lld is trivial and doing so AFAICT loses us nothing. Hence, I don't see a compelling reason to avoid this change. Since a user's tripped over this to the tune of a few percent perf regression (?), I'm more in favor than not.

Like said, I don't poke lld very much, so I have no plans to be the one who stamps this :)

[hfinkel]

Care to update the title of this review? ;)

[tcwang]

In D55679#1330653, @hfinkel wrote:

Care to update the title of this review? ;)

Just did. Thanks!

[pcc]

Thanks for the explanation.

It may seem like a trivial change now, but changes like this add up and may result in increased complexity later (e.g. suppose that we wanted to add an --output-section-ordering-file feature; this would need to interact somehow with this change), and once you start emulating another linker like this it can be difficult to undo the change since you don't know who might be unintentionally depending on the emulation. Unless there's evidence that more programs are relying on this assumption I think it's premature to code it into the linker.

I also consider it a bug for ChromeOS to make this assumption. This isn't the first lld transition issue we've seen whose root cause turned out to be an incorrect assumption. Can't you change ChromeOS so that instead of assuming that the first few pages are hot it just moves the pages covered by .text.hot into hugepages? It's possible to use a linker script to get the address of the start and end of .text.hot. See for example:
https://cs.chromium.org/chromium/src/base/android/library_loader/anchor_functions.lds
and how the symbols defined by that file are used.

Assuming that this is the code that implements hugepages:
https://cs.chromium.org/chromium/src/chromeos/hugepage_text/hugepage_text.cc
it would seem to result in simplifying the code a little as well.

[tcwang]

In D55679#1330672, @pcc wrote:

Thanks for the explanation.

It may seem like a trivial change now, but changes like this add up and may result in increased complexity later (e.g. suppose that we wanted to add an --output-section-ordering-file feature; this would need to interact somehow with this change), and once you start emulating another linker like this it can be difficult to undo the change since you don't know who might be unintentionally depending on the emulation. Unless there's evidence that more programs are relying on this assumption I think it's premature to code it into the linker.

I also consider it a bug for ChromeOS to make this assumption. This isn't the first lld transition issue we've seen whose root cause turned out to be an incorrect assumption. Can't you change ChromeOS so that instead of assuming that the first few pages are hot it just moves the pages covered by .text.hot into hugepages? It's possible to use a linker script to get the address of the start and end of .text.hot. See for example:
https://cs.chromium.org/chromium/src/base/android/library_loader/anchor_functions.lds
and how the symbols defined by that file are used.

Assuming that this is the code that implements hugepages:
https://cs.chromium.org/chromium/src/chromeos/hugepage_text/hugepage_text.cc
it would seem to result in simplifying the code a little as well.

We agree that this is a bug for ChromeOS to make assumption based on gold. We will seek solution to fix it in the long term. I guess we might just keep the patch as a temporary workaround for now. We'll discard the patch and close it.

Thanks everyone for putting time to review it!

ELF/Writer.cpp

 // We compute a rank for each section. The rank indicates where the
 // section should be placed in the file.  Instead of using simple
 // numbers (0,1,2...), we use a series of flags. One for each decision
 // point when placing the section.
 // Using flags has two key properties:
 // * It is easy to check if a give branch was taken.
 // * It is easy two see how similar two ranks are (see getRankProximity).
 enum RankFlags {
-  RF_NOT_ADDR_SET = 1 << 18,
-  RF_NOT_INTERP = 1 << 17,
-  RF_NOT_ALLOC = 1 << 16,
-  RF_WRITE = 1 << 15,
-  RF_EXEC_WRITE = 1 << 14,
-  RF_EXEC = 1 << 13,
+  RF_NOT_ADDR_SET = 1 << 19,
+  RF_NOT_INTERP = 1 << 18,
+  RF_NOT_ALLOC = 1 << 17,
+  RF_WRITE = 1 << 16,
+  RF_EXEC_WRITE = 1 << 15,
+  RF_EXEC = 1 << 14,
+  RF_EXEC_HOT = 1 << 13,
   RF_RODATA = 1 << 12,
   RF_NON_TLS_BSS = 1 << 11,
   RF_NON_TLS_BSS_RO = 1 << 10,
   RF_NOT_TLS = 1 << 9,
   RF_BSS = 1 << 8,
   RF_NOTE = 1 << 7,
   RF_PPC_NOT_TOCBSS = 1 << 6,
   RF_PPC_TOCL = 1 << 5,
static unsigned getSectionRank(const OutputSection *Sec) {
   // * Writable sections come last, such that .bss lands at the very
   //   end of the last PT_LOAD.
   bool IsExec = Sec->Flags & SHF_EXECINSTR;
   bool IsWrite = Sec->Flags & SHF_WRITE;
 
   if (IsExec) {
     if (IsWrite)
       Rank |= RF_EXEC_WRITE;
+    else {
+      if (isSectionPrefix(".text.hot.", Sec->Name))
+        Rank |= RF_EXEC_HOT;
       else
         Rank |= RF_EXEC;
+    }
   } else if (IsWrite) {
     Rank |= RF_WRITE;
   } else if (Sec->Type == SHT_PROGBITS) {
     // 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.
     Rank |= RF_RODATA;

test/ELF/text-section-prefix.s

 # REQUIRES: x86
 # RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t
 # RUN: ld.lld -z keep-text-section-prefix %t -o %t2
 # RUN: llvm-readelf -l %t2 | FileCheck %s
 # RUN: ld.lld %t -o %t3
 # RUN: llvm-readelf -l %t3 | FileCheck --check-prefix=CHECKNO %s
 # RUN: ld.lld -z nokeep-text-section-prefix %t -o %t4
 # RUN: llvm-readelf -l %t4 | FileCheck --check-prefix=CHECKNO %s
 
-# CHECK: .text
 # CHECK: .text.hot
+# CHECK: .text
 # CHECK: .text.startup
 # CHECK: .text.exit
 # CHECK: .text.unlikely
 # CHECKNO: .text
 # CHECKNO-NOT: .text.hot
 
 _start:
   ret