Keep the output text sections with prefixes ".text.hot" , ".text.unlikely", ".text.startup", ".text.exit" separate

[tmsriram]

Summary

Separate output sections for selected text section prefixes to enable TLB optimizations and for readablilty.

I committed a patch to gold linker recently to do this although that was via a flag. Talking to Rui offline, he suggested I make this the default. This is useful for the following reasons:

• We have tools to map specific code to huge pages and we do not want to do this for unlikely executed code, improves huge page TLB.
• We also munlock code that is unlikely executed and it is easier to find it with this.
• This is also useful or code layout verification.

Is this alright?

[ruiu]

And this should also slightly improve performance because of less number of page faults and improved locality for hot paths, I guess?

[pcc]

Note that this ordering isn't necessarily best for every architecture. On ARM for example it is best to put the hot sections in the middle (see also D44969).

I think that for hot sections my preference would be to treat them as if they were listed in --symbol-ordering-file. The behaviour for the other sections seems fine to me.

[ruiu]

What do you mean by "as if they were listed in --symbol-ordering-file"?

[pcc]

I mean that on ARM any sections named .text.hot.* would be placed in the middle of the .text section right after the sections named in the --symbol-ordering-file (if present).

[tmsriram]

In D45841#1072712, @pcc wrote:

Note that this ordering isn't necessarily best for every architecture. On ARM for example it is best to put the hot sections in the middle (see also D44969).

When you say "this ordering" I am confused because currently there is no ordering of the output sections with prefix ".text". Looking at it, seems like the order in which they were seen is the order in the final binary.

I think that for hot sections my preference would be to treat them as if they were listed in --symbol-ordering-file. The behaviour for the other sections seems fine to me.

So, this patch would break any symbol ordering across prefixes. Maybe it is better to disable this feature if symbol-ordering-file is used, what do you think?

[pcc]

In D45841#1072731, @tmsriram wrote:

In D45841#1072712, @pcc wrote:

Note that this ordering isn't necessarily best for every architecture. On ARM for example it is best to put the hot sections in the middle (see also D44969).

When you say "this ordering" I am confused because currently there is no ordering of the output sections with prefix ".text". Looking at it, seems like the order in which they were seen is the order in the final binary.

Yes, I meant the ordering imposed by this patch which would be hot followed by unlikely, startup, etc.

I think that for hot sections my preference would be to treat them as if they were listed in --symbol-ordering-file. The behaviour for the other sections seems fine to me.

So, this patch would break any symbol ordering across prefixes. Maybe it is better to disable this feature if symbol-ordering-file is used, what do you think?

That would also work for now.

[tmsriram]

In D45841#1072734, @pcc wrote:

In D45841#1072731, @tmsriram wrote:

In D45841#1072712, @pcc wrote:

Note that this ordering isn't necessarily best for every architecture. On ARM for example it is best to put the hot sections in the middle (see also D44969).

When you say "this ordering" I am confused because currently there is no ordering of the output sections with prefix ".text". Looking at it, seems like the order in which they were seen is the order in the final binary.

Yes, I meant the ordering imposed by this patch which would be hot followed by unlikely, startup, etc.

Ok, this patch itself does not impose an ordering among output sections. The ordering is arbitrary and follows what is seen first by the linker. Look at the test case shown. If I swap .text.exit and .text.startup for instance, the order would be different.

I think that for hot sections my preference would be to treat them as if they were listed in --symbol-ordering-file. The behaviour for the other sections seems fine to me.

So, this patch would break any symbol ordering across prefixes. Maybe it is better to disable this feature if symbol-ordering-file is used, what do you think?

That would also work for now.

I was thinking a lot more about this and playing with it. Two things:

• This patch will break symbol-ordering-file for symbols that are in two different output sections.
• This patch could also affect linker scripts which assume all .text is in the same output section.

Given this, I believe it might be better to not change the default behavior and have this feature under a flag, just like what gold does. What do you think?

[pcc]

In D45841#1073829, @tmsriram wrote:

In D45841#1072734, @pcc wrote:

In D45841#1072731, @tmsriram wrote:

In D45841#1072712, @pcc wrote:

Note that this ordering isn't necessarily best for every architecture. On ARM for example it is best to put the hot sections in the middle (see also D44969).

When you say "this ordering" I am confused because currently there is no ordering of the output sections with prefix ".text". Looking at it, seems like the order in which they were seen is the order in the final binary.

Yes, I meant the ordering imposed by this patch which would be hot followed by unlikely, startup, etc.

Ok, this patch itself does not impose an ordering among output sections. The ordering is arbitrary and follows what is seen first by the linker. Look at the test case shown. If I swap .text.exit and .text.startup for instance, the order would be different.

Got it. Sorry I missed that part.

I think that for hot sections my preference would be to treat them as if they were listed in --symbol-ordering-file. The behaviour for the other sections seems fine to me.

So, this patch would break any symbol ordering across prefixes. Maybe it is better to disable this feature if symbol-ordering-file is used, what do you think?

That would also work for now.

I was thinking a lot more about this and playing with it. Two things:

• This patch will break symbol-ordering-file for symbols that are in two different output sections.
• This patch could also affect linker scripts which assume all .text is in the same output section.

Given this, I believe it might be better to not change the default behavior and have this feature under a flag, just like what gold does. What do you think?

Seems reasonable. Even if we made it conditional on presence of --symbol-ordering-file or a linker script I think it would be a little confusing for either of those things to have that sort of effect on prefixed output sections.

[grimar]

I think this is OK, but since it changes the traditional legacy "GNU" linkers behavior, don't we want a flag to disable that?
Since gold has a flag, we will need to support it anyways I think, so we probably should have -no-<something>/-<something> flags,
matching the gold ones and just a different default value.

ELF/Writer.cpp

107 ↗

(On Diff #143170)

This needs a comment I think.

test/ELF/text-section-prefix.s

12 ↗

(On Diff #143170)

You do not need symbols, no? Just having different .text.* sections should be enough for this test case.

[grimar]

In D45841#1073829, @tmsriram wrote:

I was thinking a lot more about this and playing with it. Two things:

• This patch will break symbol-ordering-file for symbols that are in two different output sections.
• This patch could also affect linker scripts which assume all .text is in the same output section.

Given this, I believe it might be better to not change the default behavior and have this feature under a flag, just like what gold does. What do you think?

I do not think linker scripts is a problem probably. When you write a script, you usually explicitly say where to put the sections,
so if script wants all .text* to be in a single section it just do .text : { *(.text) } and I think this is what scripts usually do.
If they don't, then there will be multiple .text sections, but I am not sure it is correct for the script to assume that case without listing the output section .text.

For symbol-ordering-file, disabling the feature looks like a solution.

But actually following the gold behaviour (so to have the feature under the flag and do not change the default) looks good to me for start, as it is safe and can
easily be switched.

[tmsriram]

New option -z keep-text-section-prefix.

• Changed to have this feature enabled by an option
• The default is not changed.

[grimar]

Can you add a test case, please?

ELF/Driver.cpp

765 ↗

(On Diff #145097)

Since each word is separated in a command line option name, I believe naming
should use uppercases:

ZKeepTextSectionPrefix

ELF/Writer.cpp

94 ↗

(On Diff #145097)

This can be just:

return SectionName.startswith(PrefixName) || SectionName == PrefixName.drop_back();

I would probably also use shorter namings for arguments:
PrefixName -> Prefix
SectionName -> Name

114 ↗

(On Diff #145097)

I think using commas would be more correct?

".text.hot", ".text.unlikely", ".text.startup" or ".text.exit"

[tmsriram]

Add back the test that was omitted in the last update.

Make all the other suggested changes.

[grimar]

test/ELF/text-section-prefix.s

2 ↗

(On Diff #145216)

Please add a check for nokeep-text-section-prefix too.

[ruiu]

ELF/Driver.cpp

762–766 ↗

(On Diff #145216)

Sort alphabetically.

ELF/Writer.cpp

113 ↗

(On Diff #145216)

Could you also mention why you want to do that in the first place? In this case the intention of doing this is more important than what we are doing for future readers.

119–120 ↗

(On Diff #145216)

nit: remove {}

[tmsriram]

Add check for nokeep-text-section-prefix

[grimar]

test/ELF/text-section-prefix.s

5 ↗

(On Diff #145235)

I think it is a bit overcomplicated.

Can you just add 2 cases here please:

1. No any flags.
2. Case with -z nokeep-text-section-prefix.

Them should be the same. And I think the following should work:

1. CHECKNO: .text
2. CHECKNO-NOT: .text

[tmsriram]

Updated with more comments and nits fixed.

[tmsriram]

Updated test.

[ruiu]

LGTM

ELF/Writer.cpp

117 ↗

(On Diff #145275)

nit: we remove {} in a case like this.

lld/trunk/ELF/Config.h

struct Configuration {
   bool WarnCommon;
   bool WarnMissingEntry;
   bool WarnSymbolOrdering;
   bool WriteAddends;
   bool ZCombreloc;
   bool ZCopyreloc;
   bool ZExecstack;
   bool ZHazardplt;
+  bool ZKeepTextSectionPrefix;
   bool ZNodelete;
   bool ZNodlopen;
   bool ZNow;
   bool ZOrigin;
   bool ZRelro;
   bool ZRodynamic;
   bool ZText;
   bool ZRetpolineplt;

lld/trunk/ELF/Driver.cpp

Config->WarnBackrefs =
       Args.hasFlag(OPT_warn_backrefs, OPT_no_warn_backrefs, false);
   Config->WarnCommon = Args.hasFlag(OPT_warn_common, OPT_no_warn_common, false);
   Config->WarnSymbolOrdering =
       Args.hasFlag(OPT_warn_symbol_ordering, OPT_no_warn_symbol_ordering, true);
   Config->ZCombreloc = getZFlag(Args, "combreloc", "nocombreloc", true);
   Config->ZCopyreloc = getZFlag(Args, "copyreloc", "nocopyreloc", true);
   Config->ZExecstack = getZFlag(Args, "execstack", "noexecstack", false);
   Config->ZHazardplt = hasZOption(Args, "hazardplt");
+  Config->ZKeepTextSectionPrefix = getZFlag(
+      Args, "keep-text-section-prefix", "nokeep-text-section-prefix", false);
   Config->ZNodelete = hasZOption(Args, "nodelete");
   Config->ZNodlopen = hasZOption(Args, "nodlopen");
   Config->ZNow = getZFlag(Args, "now", "lazy", false);
   Config->ZOrigin = hasZOption(Args, "origin");
   Config->ZRelro = getZFlag(Args, "relro", "norelro", true);
   Config->ZRetpolineplt = hasZOption(Args, "retpolineplt");
   Config->ZRodynamic = hasZOption(Args, "rodynamic");
   Config->ZStackSize = args::getZOptionValue(Args, OPT_z, "stack-size", 0);

lld/trunk/ELF/Writer.cpp

private:
 
   uint64_t FileSize;
   uint64_t SectionHeaderOff;
 
   bool HasGotBaseSym = false;
 };
 } // anonymous namespace
 
+static bool isSectionPrefix(StringRef Prefix, StringRef Name) {
+  return Name.startswith(Prefix) || Name == Prefix.drop_back();
+}
+
 StringRef elf::getOutputSectionName(InputSectionBase *S) {
   if (Config->Relocatable)
     return S->Name;
 
   // This is for --emit-relocs. If .text.foo is emitted as .text.bar, we want
   // to emit .rela.text.foo as .rela.text.bar for consistency (this is not
   // technically required, but not doing it is odd). This code guarantees that.
   if (auto *IS = dyn_cast<InputSection>(S)) {
     if (InputSectionBase *Rel = IS->getRelocatedSection()) {
       OutputSection *Out = Rel->getOutputSection();
       if (S->Type == SHT_RELA)
         return Saver.save(".rela" + Out->Name);
       return Saver.save(".rel" + Out->Name);
     }
   }
 
+  // This check is for -z keep-text-section-prefix.  This option separates text
+  // sections with prefix ".text.hot", ".text.unlikely", ".text.startup" or
+  // ".text.exit".
+  // When enabled, this allows identifying the hot code region (.text.hot) in
+  // the final binary which can be selectively mapped to huge pages or mlocked,
+  // for instance.
+  if (Config->ZKeepTextSectionPrefix)
+    for (StringRef V :
+         {".text.hot.", ".text.unlikely.", ".text.startup.", ".text.exit."}) {
+      if (isSectionPrefix(V, S->Name))
+        return V.drop_back();
+    }
+
   for (StringRef V :
        {".text.", ".rodata.", ".data.rel.ro.", ".data.", ".bss.rel.ro.",
         ".bss.", ".init_array.", ".fini_array.", ".ctors.", ".dtors.", ".tbss.",
         ".gcc_except_table.", ".tdata.", ".ARM.exidx.", ".ARM.extab."}) {
-    StringRef Prefix = V.drop_back();
-    if (S->Name.startswith(V) || S->Name == Prefix)
-      return Prefix;
+    if (isSectionPrefix(V, S->Name))
+      return V.drop_back();
   }
 
   // CommonSection is identified as "COMMON" in linker scripts.
   // By default, it should go to .bss section.
   if (S->Name == "COMMON")
     return ".bss";
 
   return S->Name;

lld/trunk/test/ELF/text-section-prefix.s

+# RUN: llvm-mc -filetype=obj %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.startup
+# CHECK: .text.exit
+# CHECK: .text.unlikely
+# CHECKNO: .text
+# CHECKNO-NOT: .text.hot
+
+_start:
+  ret
+
+.section .text.f,"ax"
+f:
+  nop
+
+.section .text.hot.f_hot,"ax"
+f_hot:
+  nop
+
+.section .text.startup.f_startup,"ax"
+f_startup:
+  nop
+
+.section .text.exit.f_exit,"ax"
+f_exit:
+  nop
+
+.section .text.unlikely.f_unlikely,"ax"
+f_unlikely:
+  nop