Basic block sections for functions with implicit-section-name attribute

[tmsriram]

Summary

Functions can have section names set via #pragma or section attributes, basic block sections should be correctly named for such functions.

With #pragma, the expectation is that all functions in that file are placed in the same section in the final binary. Basic block sections should be correctly named with the unique flag set so that the final binary has all the
basic blocks of the function in that named section. This patch fixes the bug by calling getExplictSectionGlobal when implicit-section-name attribute is set to make sure the function's basic blocks get the correct section name.

[rahmanl]

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

752–759

Except may for chromium with its complex implementation detail here: https://chromium.googlesource.com/chromium/src/+/66.0.3359.158/base/memory/protected_memory.h
Any thoughts on whether this would work with it (related bug: https://bugs.chromium.org/p/chromium/issues/detail?id=990942).

880

It seems like only the last parameter is changed between this call and the one above. Maybe it is OK to change the original function's signature?

[tmsriram]

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

752–759

Section attributes work just fine with the patch. I can add another test. This "," separated specific usage however does not seem to do anything with linux ELF. The "," is just treated as another character.

[rahmanl]

LGTM with minor comments.

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

752–758

This sentence sounds more relevant to the callsite of the function. Here maybe we can simply say "Get a unique ID if forced to emit unique sections. [the rest of the comment]"

752–759

It's probably fine because we only do unique sections for code (and only with the -basic-block-sections option) and the section flags are always the same 'ax'. Anyhow, we can look at this when we build chromium with basic block sections.

llvm/test/CodeGen/X86/basic-block-sections-pragma-sections.ll

6

nit: This is not really ordering basic block sections. Maybe "list" is a better name?

[tmsriram]

Address reviewer comments:

1. Add another test for section name
2. Fix nits.

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

880

I looked at all the call sites for this function and this signature is used across MachO, COFF, etc. Futher, all other functions that get sections use the same method. One thing I could do is to use default parameters. I am keeping it unchanged but I can modify it if you like.

[snehasish]

lgtm

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

657

nit: If you use a reference here then you can remove the deref and simplify the diff.

789

nit: /* ForceUnique = */ false

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

if (isVerbose())
     OutStreamer->GetCommentOS()
         << "-- Begin function "
         << GlobalValue::dropLLVMManglingEscape(F.getName()) << '\n';
 
   // Print out constants referenced by the function
   emitConstantPool();
 
   // Print the 'header' of function.
-  // If basic block sections is desired and function sections is off,
-  // explicitly request a unique section for this function.
-  if (MF->front().isBeginSection() && !TM.getFunctionSections())
+  // If basic block sections are desired, explicitly request a unique section
+  // for this function's entry block.
+  if (MF->front().isBeginSection())
     MF->setSection(getObjFileLowering().getUniqueSectionForFunction(F, TM));
   else
     MF->setSection(getObjFileLowering().SectionForGlobal(&F, TM));
   OutStreamer->SwitchSection(MF->getSection());
 
   if (!MAI->hasVisibilityOnlyWithLinkage())
     emitVisibility(CurrentFnSym, F.getVisibility());
 

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

 public:
   LoweringDiagnosticInfo(const Twine &DiagMsg,
                          DiagnosticSeverity Severity = DS_Error)
       : DiagnosticInfo(DK_Lowering, Severity), Msg(DiagMsg) {}
   void print(DiagnosticPrinter &DP) const override { DP << Msg; }
 };
 }
 
-MCSection *TargetLoweringObjectFileELF::getExplicitSectionGlobal(
-    const GlobalObject *GO, SectionKind Kind, const TargetMachine &TM) const {
+static MCSection *selectExplicitSectionGlobal(
+    const GlobalObject *GO, SectionKind Kind, const TargetMachine &TM,
+    MCContext &Ctx, Mangler &Mang, unsigned &NextUniqueID,
+    bool Retain, bool ForceUnique) {
   StringRef SectionName = GO->getSection();
 
   // Check if '#pragma clang section' name is applicable.
   // Note that pragma directive overrides -ffunction-section, -fdata-section
   // and so section name is exactly as user specified and not uniqued.
   const GlobalVariable *GV = dyn_cast<GlobalVariable>(GO);
   if (GV && GV->hasImplicitSection()) {
     auto Attrs = GV->getAttributes();
static MCSection *selectExplicitSectionGlobal(
 
   unsigned EntrySize = getEntrySizeForKind(Kind);
 
   // A section can have at most one associated section. Put each global with
   // MD_associated in a unique section.
   unsigned UniqueID = MCContext::GenericSectionID;
   const MCSymbolELF *LinkedToSym = getLinkedToSymbol(GO, TM);
   const bool Associated = GO->getMetadata(LLVMContext::MD_associated);
-  const bool Retain = Used.count(GO);
   if (Associated || Retain) {
     UniqueID = NextUniqueID++;
     if (Associated)
       Flags |= ELF::SHF_LINK_ORDER;
-    if (Retain && (getContext().getAsmInfo()->useIntegratedAssembler() ||
-                   getContext().getAsmInfo()->binutilsIsAtLeast(2, 36)))
+    if (Retain && (Ctx.getAsmInfo()->useIntegratedAssembler() ||
+                   Ctx.getAsmInfo()->binutilsIsAtLeast(2, 36)))
       Flags |= ELF::SHF_GNU_RETAIN;
   } else {
-    if (getContext().getAsmInfo()->useIntegratedAssembler() ||
-        getContext().getAsmInfo()->binutilsIsAtLeast(2, 35)) {
+    if (Ctx.getAsmInfo()->useIntegratedAssembler() ||
+        Ctx.getAsmInfo()->binutilsIsAtLeast(2, 35)) {
       // Symbols must be placed into sections with compatible entry
       // sizes. Generate unique sections for symbols that have not
       // been assigned to compatible sections.
       if (Flags & ELF::SHF_MERGE) {
-        auto maybeID = getContext().getELFUniqueIDForEntsize(SectionName, Flags,
-                                                             EntrySize);
+        auto maybeID = Ctx.getELFUniqueIDForEntsize(SectionName, Flags,
         if (maybeID)
           UniqueID = *maybeID;
         else {
           // If the user has specified the same section name as would be created
           // implicitly for this symbol e.g. .rodata.str1.1, then we don't need
           // to unique the section as the entry size for this symbol will be
           // compatible with implicitly created sections.
           SmallString<128> ImplicitSectionNameStem = getELFSectionNameForGlobal(
-              GO, Kind, getMangler(), TM, EntrySize, false);
-          if (!(getContext().isELFImplicitMergeableSectionNamePrefix(
-                    SectionName) &&
+              GO, Kind, Mang, TM, EntrySize, false);
+          if (!(Ctx.isELFImplicitMergeableSectionNamePrefix(SectionName) &&
                 SectionName.startswith(ImplicitSectionNameStem)))
             UniqueID = NextUniqueID++;
         }
       } else {
         // We need to unique the section if the user has explicity
         // assigned a non-mergeable symbol to a section name for
         // a generic mergeable section.
-        if (getContext().isELFGenericMergeableSection(SectionName)) {
-          auto maybeID = getContext().getELFUniqueIDForEntsize(
+        if (Ctx.isELFGenericMergeableSection(SectionName)) {
+          auto maybeID = Ctx.getELFUniqueIDForEntsize(
               SectionName, Flags, EntrySize);
           UniqueID = maybeID ? *maybeID : NextUniqueID++;
         }
       }
     } else {
       // If two symbols with differing sizes end up in the same mergeable
       // section that section can be assigned an incorrect entry size. To avoid
       // this we usually put symbols of the same size into distinct mergeable
       // sections with the same name. Doing so relies on the ",unique ,"
       // assembly feature. This feature is not avalible until bintuils
       // version 2.35 (https://sourceware.org/bugzilla/show_bug.cgi?id=25380).
       Flags &= ~ELF::SHF_MERGE;
       EntrySize = 0;
     }
   }
 
-  MCSectionELF *Section = getContext().getELFSection(
+  // Increment uniqueID if we are forced to emit a unique section.
+  // This works perfectly fine with section attribute or pragma section as the
+  // sections with the same name are grouped together by the assembler.
+  if (ForceUnique && UniqueID == MCContext::GenericSectionID)
+    UniqueID = NextUniqueID++;
+
+  MCSectionELF *Section = Ctx.getELFSection(
       SectionName, getELFSectionType(SectionName, Kind), Flags, EntrySize,
       Group, IsComdat, UniqueID, LinkedToSym);
   // Make sure that we did not get some other section with incompatible sh_link.
   // This should not be possible due to UniqueID code above.
   assert(Section->getLinkedToSymbol() == LinkedToSym &&
          "Associated symbol mismatch between sections");
 
-  if (!(getContext().getAsmInfo()->useIntegratedAssembler() ||
-        getContext().getAsmInfo()->binutilsIsAtLeast(2, 35))) {
+  if (!(Ctx.getAsmInfo()->useIntegratedAssembler() ||
+        Ctx.getAsmInfo()->binutilsIsAtLeast(2, 35))) {
     // If we are using GNU as before 2.35, then this symbol might have
     // been placed in an incompatible mergeable section. Emit an error if this
     // is the case to avoid creating broken output.
     if ((Section->getFlags() & ELF::SHF_MERGE) &&
         (Section->getEntrySize() != getEntrySizeForKind(Kind)))
       GO->getContext().diagnose(LoweringDiagnosticInfo(
           "Symbol '" + GO->getName() + "' from module '" +
           (GO->getParent() ? GO->getParent()->getSourceFileName() : "unknown") +
           "' required a section with entry-size=" +
           Twine(getEntrySizeForKind(Kind)) + " but was placed in section '" +
           SectionName + "' with entry-size=" + Twine(Section->getEntrySize()) +
           ": Explicit assignment by pragma or attribute of an incompatible "
           "symbol to this section?"));
   }
 
   return Section;
 }
 
+MCSection *TargetLoweringObjectFileELF::getExplicitSectionGlobal(
+    const GlobalObject *GO, SectionKind Kind, const TargetMachine &TM) const {
+  return selectExplicitSectionGlobal(GO, Kind, TM, getContext(), getMangler(),
+                                     NextUniqueID, Used.count(GO),
+                                     /* ForceUnique = */false);
+}
+
 static MCSectionELF *selectELFSectionForGlobal(
     MCContext &Ctx, const GlobalObject *GO, SectionKind Kind, Mangler &Mang,
     const TargetMachine &TM, bool EmitUniqueSection, unsigned Flags,
     unsigned *NextUniqueID, const MCSymbolELF *AssociatedSymbol) {
 
   StringRef Group = "";
   bool IsComdat = false;
   if (const Comdat *C = getELFComdat(GO)) {
return selectELFSectionForGlobal(getContext(), GO, Kind, getMangler(), TM,
                                    Used.count(GO), EmitUniqueSection, Flags,
                                    &NextUniqueID);
 }
 
 MCSection *TargetLoweringObjectFileELF::getUniqueSectionForFunction(
     const Function &F, const TargetMachine &TM) const {
   SectionKind Kind = SectionKind::getText();
   unsigned Flags = getELFSectionFlags(Kind);
+  // If the function's section names is pre-determined via pragma or a
+  // section attribute, call selectExplicitSectionGlobal.
+  if (F.hasSection() || F.hasFnAttribute("implicit-section-name"))
+    return selectExplicitSectionGlobal(
+        &F, Kind, TM, getContext(), getMangler(), NextUniqueID,
+        Used.count(&F), /* ForceUnique = */true);
+  else
     return selectELFSectionForGlobal(
         getContext(), &F, Kind, getMangler(), TM, Used.count(&F),
         /*EmitUniqueSection=*/true, Flags, &NextUniqueID);
 }
 
 MCSection *TargetLoweringObjectFileELF::getSectionForJumpTable(
     const Function &F, const TargetMachine &TM) const {
   // If the function can be removed, produce a unique section so that
   // the table doesn't prevent the removal.
   const Comdat *C = F.getComdat();
   bool EmitUniqueSection = TM.getFunctionSections() || C;

llvm/test/CodeGen/X86/basic-block-sections-named-section.ll

+; RUN: llc < %s -mtriple=x86_64-pc-linux -basic-block-sections=all | FileCheck %s
+; RUN: llc < %s -mtriple=x86_64-pc-linux -function-sections -basic-block-sections=all | FileCheck %s
+; RUN: echo "!_Z3fooi" > %t.order.txt
+; RUN: echo "!!2" >> %t.order.txt
+; RUN: llc < %s -mtriple=x86_64-pc-linux -basic-block-sections=%t.order.txt | FileCheck %s --check-prefix=LIST
+; RUN: llc < %s -mtriple=x86_64-pc-linux -function-sections -basic-block-sections=%t.order.txt | FileCheck %s --check-prefix=LIST
+
+; CHECK: .section foo_section,"ax",@progbits,unique,1
+; CHECK-LABEL: _Z3fooi:
+; CHECK: .section foo_section,"ax",@progbits,unique,2
+; CHECK-NEXT: _Z3fooi.__part.1:
+; CHECK: .section foo_section,"ax",@progbits,unique,3
+; CHECK-NEXT: _Z3fooi.__part.2:
+
+; LIST: .section  foo_section,"ax",@progbits,unique,1
+; LIST-LABEL: _Z3fooi:
+; LIST: .section  foo_section,"ax",@progbits,unique,2
+; LIST-NEXT: _Z3fooi.__part.0:
+; LIST-NOT: .section  foo_section,"ax",@progbits,unique,3
+
+;; Source to generate the IR:
+;; __attribute__((section("foo_section")))
+;; int foo(int n) {
+;;   if (n < 0)
+;;     exit(-1);
+;;   return 0;
+;; }
+
+define dso_local i32 @_Z3fooi(i32 %n) local_unnamed_addr section "foo_section" {
+entry:
+  %cmp = icmp slt i32 %n, 0
+  br i1 %cmp, label %if.then, label %if.end
+
+if.then:                                          ; preds = %entry
+  tail call void @exit(i32 -1) #2
+  unreachable
+
+if.end:                                           ; preds = %entry
+  ret i32 0
+}
+
+declare dso_local void @exit(i32) local_unnamed_addr

llvm/test/CodeGen/X86/basic-block-sections-pragma-sections.ll

+; RUN: llc < %s -mtriple=x86_64-pc-linux -basic-block-sections=all | FileCheck %s
+; RUN: llc < %s -mtriple=x86_64-pc-linux -function-sections -basic-block-sections=all | FileCheck %s
+; RUN: echo "!_Z3fooi" > %t.list.txt
+; RUN: echo "!!2" >> %t.list.txt
+; RUN: llc < %s -mtriple=x86_64-pc-linux -basic-block-sections=%t.list.txt | FileCheck %s --check-prefix=LIST
+; RUN: llc < %s -mtriple=x86_64-pc-linux -function-sections -basic-block-sections=%t.list.txt | FileCheck %s --check-prefix=LIST
+
+; CHECK: .section foo_section,"ax",@progbits,unique,1
+; CHECK-LABEL: _Z3fooi:
+; CHECK: .section foo_section,"ax",@progbits,unique,2
+; CHECK-NEXT: _Z3fooi.__part.1:
+; CHECK: .section foo_section,"ax",@progbits,unique,3
+; CHECK-NEXT: _Z3fooi.__part.2:
+
+; LIST: .section  foo_section,"ax",@progbits,unique,1
+; LIST-LABEL: _Z3fooi:
+; LIST: .section  foo_section,"ax",@progbits,unique,2
+; LIST-NEXT: _Z3fooi.__part.0:
+; LIST-NOT: .section  foo_section,"ax",@progbits,unique,3
+
+;; Source to generate the IR:
+;; #pragma clang section text = "foo_section"
+;; int foo(int n) {
+;;   if (n < 0)
+;;     exit(-1);
+;;   return 0;
+;; }
+
+define dso_local i32 @_Z3fooi(i32 %n) local_unnamed_addr #0 {
+entry:
+  %cmp = icmp slt i32 %n, 0
+  br i1 %cmp, label %if.then, label %if.end
+
+if.then:                                          ; preds = %entry
+  tail call void @exit(i32 -1) #2
+  unreachable
+
+if.end:                                           ; preds = %entry
+  ret i32 0
+}
+
+declare dso_local void @exit(i32) local_unnamed_addr
+
+attributes #0 = {"implicit-section-name"="foo_section" }