[TargetLoweringObjectFileImpl] Produce .text.hot. instead of .text.hot for -fno-unique-section-names

[MaskRay]

Summary

GNU ld's internal linker script uses (https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=add44f8d5c5c05e08b11e033127a744d61c26aee)

.text           :
{
  *(.text.unlikely .text.*_unlikely .text.unlikely.*)
  *(.text.exit .text.exit.*)
  *(.text.startup .text.startup.*)
  *(.text.hot .text.hot.*)
  *(SORT(.text.sorted.*))
  *(.text .stub .text.* .gnu.linkonce.t.*)
  /* .gnu.warning sections are handled specially by elf.em.  */
  *(.gnu.warning)
}

Because *(.text.exit .text.exit.*) is ordered before *(.text .text.*), in a -ffunction-sections build, the C library function exit will be placed before other functions.
gold's -z keep-text-section-prefix has the same problem.

In lld, -z keep-text-section-prefix recognizes .text.{exit,hot,startup,unlikely,unknown}.*, but not .text.{exit,hot,startup,unlikely,unknown}, to avoid the strange placement problem.

In -fno-function-sections or -fno-unique-section-names mode, a function whose function_section_prefix is set to .exit"
will go to the output section .text instead of .text.exit when linked by lld.
To address the problem, append a dot to become .text.exit.

[wmi]

Although less likely, is there possiblity that someone has already been using attribute((section(".text.hot."))? Adding another trailing period will break the existing usage. We may either tolerate that case, or trigger an assertion.

[MaskRay]

In D79600#2026282, @wmi wrote:

Although less likely, is there possiblity that someone has already been using attribute((section(".text.hot."))? Adding another trailing period will break the existing usage. We may either tolerate that case, or trigger an assertion.

__attribute__((section(".text.hot.")) overrides the default section name .text.$function or .text.$prefix.$function

__attribute__((section(".text.hot."))) int foo() { return 42; } produces .text.hot. with or without -ffunction-sections

[tmsriram]

@MaskRay Could you please elaborate more on the problem being solved as it is not still fully clear to me? Is it that only the "exit" function has this problem? If you could also mention why the linker is not the right place to handle this problem, it would be useful. Thanks.

[MaskRay]

In D79600#2027342, @tmsriram wrote:

@MaskRay Could you please elaborate more on the problem being solved as it is not still fully clear to me? Is it that only the "exit" function has this problem? If you could also mention why the linker is not the right place to handle this problem, it would be useful. Thanks.

GNU ld's internal linker script orders *(.text.exit .text.exit.*) before *(.text .text.*). This property makes C functions unlikely exit hot startup to be ordered before other normal C functions in a -ffunction-sections build.

So in lld, I don't want to let -z keep-text-section-prefix recognize .text.exit (and the like). lld should just recognize .text.exit.*

Because of this, in -fno-unique-section-names mode, we may get .text.exit which will not go to the output section .text.exit. To fix that, name the input section .text.exit.

[tmsriram]

As far as I can tell, this LGTM. I am not an expert on section names and how this affects assumptions in GNU land, so please do seek an expert's review if necessary.

[MaskRay]

In D79600#2028496, @tmsriram wrote:

As far as I can tell, this LGTM. I am not an expert on section names and how this affects assumptions in GNU land, so please do seek an expert's review if necessary.

.text.exit. works with GNU ld's internal linker script and gold's -z keep-text-section-prefix. I CCed some linker experts.

[grimar]

After reading the discussion in D79590, I think this makes sense, so LGTM. Please hold this a bit, perhaps Peter or somebody else might also have comments.

[psmith]

I don't have a lot to add. I think this change will work with LLD and the GNU tools, no objections to this change. Having LLD not recognise the section names without a suffix may become a problem if GNU tools still output them. My guess is that LLD is still most used with clang at the moment, but this may change over time.

[MaskRay]

In D79600#2031300, @psmith wrote:

I don't have a lot to add. I think this change will work with LLD and the GNU tools, no objections to this change. Having LLD not recognise the section names without a suffix may become a problem if GNU tools still output them. My guess is that LLD is still most used with clang at the moment, but this may change over time.

Yes, this will work with lld, GNU ld and gold. Your concern may be that GCC will still emit .text.exit? It emits .text.exit.* (function section manner). It does not support -fno-unique-section-names. Part of the reasons is that the related GNU as feature is a fairly new addition 2020-02 http://sourceware.org/PR25380 (I am going to raise a GCC feature request.)

edit: filed https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95095

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

if (Kind.isMergeableCString()) {
     Name = SizeSpec + utostr(Align);
   } else if (Kind.isMergeableConst()) {
     Name = ".rodata.cst";
     Name += utostr(EntrySize);
   } else {
     Name = getSectionPrefixForGlobal(Kind);
   }
 
+  bool HasPrefix = false;
   if (const auto *F = dyn_cast<Function>(GO)) {
-    if (Optional<StringRef> Prefix = F->getSectionPrefix())
+    if (Optional<StringRef> Prefix = F->getSectionPrefix()) {
       Name += *Prefix;
+      HasPrefix = true;
+    }
   }
 
   if (UniqueSectionName) {
     Name.push_back('.');
     TM.getNameWithPrefix(Name, GO, Mang, /*MayAlwaysUsePrivate*/true);
-  }
+  } else if (HasPrefix)
+    Name.push_back('.');
   return Name;
 }
 
 namespace {
 class LoweringDiagnosticInfo : public DiagnosticInfo {
   const Twine &Msg;
 
 public:

llvm/test/Transforms/CodeGenPrepare/X86/section.ll

 ; RUN: opt < %s -codegenprepare -S | FileCheck %s
+; RUN: llc < %s | FileCheck --check-prefix=ASM1 %s
+; RUN: llc < %s -function-sections | FileCheck --check-prefix=ASM2 %s
 
 target triple = "x86_64-pc-linux-gnu"
 
 ; This tests that hot/cold functions get correct section prefix assigned
 
 ; CHECK: hot_func1{{.*}}!section_prefix ![[HOT_ID:[0-9]+]]
+; ASM1: .section .text.hot.,"ax",@progbits
+; ASM2: .section .text.hot.hot_func1,"ax",@progbits
 ; The entry is hot
 define void @hot_func1() !prof !15 {
   ret void
 }
 
 ; CHECK: hot_func2{{.*}}!section_prefix ![[HOT_ID:[0-9]+]]
 ; Entry is cold but inner block is hot
 define void @hot_func2(i32 %n) !prof !16 {

 for.end:
   ret void
 }
 
 ; For instrumentation based PGO, we should only look at block counts,
 ; not call site VP metadata (which can exist on value profiled memcpy,
 ; or possibly left behind after static analysis based devirtualization).
 ; CHECK: cold_func1{{.*}}!section_prefix ![[COLD_ID:[0-9]+]]
+; ASM1: .section .text.unlikely.,"ax",@progbits
+; ASM2: .section .text.unlikely.cold_func1,"ax",@progbits
 define void @cold_func1() !prof !16 {
   call void @hot_func1(), !prof !17
   call void @hot_func1(), !prof !17
   ret void
 }
 
 ; CHECK: cold_func2{{.*}}!section_prefix ![[COLD_ID]]
 define void @cold_func2() !prof !16 {