Create unique, but identically-named ELF sections for explicitly-sectioned functions and globals when using -function-sections and -data-sections.

[rahmanl]

Summary

This allows functions and globals to to be reordered later in the linking phase (using the -symbol-ordering-file) even though reordering will be limited to the scope of the explicit section.

[pcc]

I am in favour of this change.

Should we do the same for globals+data sections?

lib/CodeGen/TargetLoweringObjectFileImpl.cpp

596

clang-format

test/CodeGen/X86/explicit-function-sections.ll

20 ↗

(On Diff #212437)

Shouldn't there be a ,unique,1 here?

[rahmanl]

Updated the patch to handle data sections as well (thanks to @pcc for the suggestion).

[rahmanl]

Fix regular expression in explicit-elf-sections.ll test.

[pcc]

Looks like you uploaded a diff without the code changes.

[pcc]

LGTM

lib/CodeGen/TargetLoweringObjectFileImpl.cpp

591

nit: s/section/sections/g

610

Remove braces

[rahmanl]

Fixed the comment and removed the braces.

[rahmanl]

Thanks for the LGTM @ppc
Please go ahead and commit this change as I don't have commit access yet.
Differential Revision: https://reviews.llvm.org/D65478

[inglorion]

Unfortunately, this causes some problems. It looks like the sections are created with different attributes than the original. This causes problems such as sections being writable when the original was read-only, including https://crbug.com/990942 where that causes program crashes. There is some more discussion on that bug report.

I'll revert this to unbreak the Chromium build, and we can come up with a way forward from there.

[rahmanl]

In D65478#1619713, @inglorion wrote:

Unfortunately, this causes some problems. It looks like the sections are created with different attributes than the original. This causes problems such as sections being writable when the original was read-only, including https://crbug.com/990942 where that causes program crashes. There is some more discussion on that bug report.

I'll revert this to unbreak the Chromium build, and we can come up with a way forward from there.

Hi Bob,
We plan to gate this feature behind a condition: D65837
I think in the Chromium case, what it needs is the -funique-section-names. More precisely, Chromium needs this flag only for the explicitly defined section "protected_memory", but I don't think it hurts to do it globally ( Chromium build may actually be using this flag anyways. Please confirm.)
If you think that the chromium build could use this flag, I will change D65837 to do the check.

[pcc]

In D65478#1619990, @rahmanl wrote:

In D65478#1619713, @inglorion wrote:

Unfortunately, this causes some problems. It looks like the sections are created with different attributes than the original. This causes problems such as sections being writable when the original was read-only, including https://crbug.com/990942 where that causes program crashes. There is some more discussion on that bug report.

I'll revert this to unbreak the Chromium build, and we can come up with a way forward from there.

Hi Bob,
We plan to gate this feature behind a condition: D65837
I think in the Chromium case, what it needs is the -funique-section-names. More precisely, Chromium needs this flag only for the explicitly defined section "protected_memory", but I don't think it hurts to do it globally ( Chromium build may actually be using this flag anyways. Please confirm.)
If you think that the chromium build could use this flag, I will change D65837 to do the check.

Chromium is depending on an implementation detail here and the revert is temporary until we can land a compiler feature that will let us avoid the implementation detail dependence. It doesn't seem necessary to change the condition that you've already implemented in D65837.

[rahmanl]

In D65478#1620005, @pcc wrote:

In D65478#1619990, @rahmanl wrote:

In D65478#1619713, @inglorion wrote:

Unfortunately, this causes some problems. It looks like the sections are created with different attributes than the original. This causes problems such as sections being writable when the original was read-only, including https://crbug.com/990942 where that causes program crashes. There is some more discussion on that bug report.

I'll revert this to unbreak the Chromium build, and we can come up with a way forward from there.

Hi Bob,
We plan to gate this feature behind a condition: D65837
I think in the Chromium case, what it needs is the -funique-section-names. More precisely, Chromium needs this flag only for the explicitly defined section "protected_memory", but I don't think it hurts to do it globally ( Chromium build may actually be using this flag anyways. Please confirm.)
If you think that the chromium build could use this flag, I will change D65837 to do the check.

Chromium is depending on an implementation detail here and the revert is temporary until we can land a compiler feature that will let us avoid the implementation detail dependence. It doesn't seem necessary to change the condition that you've already implemented in D65837.

Agreed.
However, I was thinking that the implementation detail relies on the assumption that only one section exists of the name "protected_memory" and if we somehow let the programmer specify that a particular section name can only appear once, it would resolve the problem. Enforcing the use of funique-section-names does look overkill.

[pcc]

In D65478#1620027, @rahmanl wrote:

In D65478#1620005, @pcc wrote:

In D65478#1619990, @rahmanl wrote:

In D65478#1619713, @inglorion wrote:

Unfortunately, this causes some problems. It looks like the sections are created with different attributes than the original. This causes problems such as sections being writable when the original was read-only, including https://crbug.com/990942 where that causes program crashes. There is some more discussion on that bug report.

I'll revert this to unbreak the Chromium build, and we can come up with a way forward from there.

Hi Bob,
We plan to gate this feature behind a condition: D65837
I think in the Chromium case, what it needs is the -funique-section-names. More precisely, Chromium needs this flag only for the explicitly defined section "protected_memory", but I don't think it hurts to do it globally ( Chromium build may actually be using this flag anyways. Please confirm.)
If you think that the chromium build could use this flag, I will change D65837 to do the check.

Chromium is depending on an implementation detail here and the revert is temporary until we can land a compiler feature that will let us avoid the implementation detail dependence. It doesn't seem necessary to change the condition that you've already implemented in D65837.

Agreed.
However, I was thinking that the implementation detail relies on the assumption that only one section exists of the name "protected_memory" and if we somehow let the programmer specify that a particular section name can only appear once, it would resolve the problem. Enforcing the use of funique-section-names does look overkill.

Yeah, I feel like we should avoid letting this be toggleable though unless we really need to, and if we do add a toggle for it, it should be a separate flag from -funique-section-names.

lib/CodeGen/TargetLoweringObjectFileImpl.cpp

     Flags |= ELF::SHF_GROUP;
   }
 
+  bool EmitUniqueSection = false;
+
+  // If we have -ffunction-section or -fdata-section then we should emit the
+  // global value to a uniqued section of the same name.
+  if (!(Flags & ELF::SHF_MERGE) && !Kind.isCommon()) {
+    if (Kind.isText())
+      EmitUniqueSection = TM.getFunctionSections();
+    else
+      EmitUniqueSection = TM.getDataSections();
+  }
+  EmitUniqueSection |= GO->hasComdat();
+
   // 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 *AssociatedSymbol = getAssociatedSymbol(GO, TM);
   if (AssociatedSymbol) {
-    UniqueID = NextUniqueID++;
+    EmitUniqueSection = true;
     Flags |= ELF::SHF_LINK_ORDER;
   }
 
+  unsigned UniqueID = MCContext::GenericSectionID;
+  if (EmitUniqueSection){
+    UniqueID = NextUniqueID++;
+  }
+
   MCSectionELF *Section = getContext().getELFSection(
       SectionName, getELFSectionType(SectionName, Kind), Flags,
       getEntrySizeForKind(Kind), Group, UniqueID, AssociatedSymbol);

test/CodeGen/X86/elf-associated.ll

-; RUN: llc -data-sections=1 -mtriple x86_64-pc-linux-gnu < %s | FileCheck %s
-; RUN: llc -data-sections=0 -mtriple x86_64-pc-linux-gnu < %s | FileCheck %s
+; RUN: llc -data-sections=1 -mtriple x86_64-pc-linux-gnu < %s | FileCheck %s --check-prefix=DSECTIONS --check-prefix=CHECK
+; RUN: llc -data-sections=0 -mtriple x86_64-pc-linux-gnu < %s | FileCheck %s --check-prefix=NDSECTIONS --check-prefix=CHECK
 
 @a = global i32 1
 @b = global i32 2, !associated !0

 @j = global i32 1, section "bbb", !associated !4
 @k = global i32 1, !associated !4
 !4 = !{i32* @h}
-; CHECK-DAG: .section aaa,"aw",@progbits
-; CHECK-DAG: .section bbb,"awo",@progbits,h,unique,1
-; CHECK-DAG: .section bbb,"awo",@progbits,h,unique,2
+; NDSECTIONS-DAG: .section  aaa,"aw",@progbits
+; NDSECTIONS-DAG: .section  bbb,"awo",@progbits,h,unique,1
+; NDSECTIONS-DAG: .section  bbb,"awo",@progbits,h,unique,2
+
+; DSECTIONS-DAG: .section aaa,"aw",@progbits,unique,1
+; DSECTIONS-DAG: .section bbb,"awo",@progbits,h,unique,2
+; DSECTIONS-DAG: .section bbb,"awo",@progbits,h,unique,3
+
 ; CHECK-DAG: .section .data.k,"awo",@progbits,h
 
 ; Non-GlobalValue metadata.

 @n = alias i32, i32* inttoptr (i64 add (i64 ptrtoint (i32* @a to i64), i64 1297036692682702848) to i32*)
 @o = global i32 1, section "eee", !associated !7
 !7 = !{i32* @n}
-; CHECK-DAG: .section eee,"awo",@progbits,n,unique,3
+; NDSECTIONS-DAG: .section  eee,"awo",@progbits,n,unique,3
+; DSECTIONS-DAG: .section eee,"awo",@progbits,n,unique,6

test/CodeGen/X86/explicit-elf-sections.ll

+; RUN: llc < %s -mtriple=x86_64-pc-linux -function-sections=1 -data-sections=1 | FileCheck %s -check-prefix=SECTIONS
+; RUN: llc < %s -mtriple=x86_64-pc-linux -function-sections=0 -data-sections=0 | FileCheck %s -check-prefix=NSECTIONS
+
+define void @f() section "aaa" { ret void }
+define void @g() section "aaa" { ret void }
+
+define void @h() { ret void }
+
+@x = global i32 1, section "aaa"
+@y = global i32 1, section "aaa"
+@z = global i32 1
+
+define void @i() section "aaa" { ret void }
+
+@w = global i32 1, section "aaa"
+
+; NSECTIONS:    .section  aaa,"ax",@progbits
+; NSECTIONS-NOT:  {{.section|.text|.data}}
+; NSECTIONS:    f:
+; NSECTIONS-NOT:  {{.section|.text|.data}}
+; NSECTIONS:    g:
+; NSECTIONS:    .text
+; NSECTIONS-NOT:  {{.section|.text|.data}}
+; NSECTIONS:    h:
+; NSECTIONS:    .section  aaa,"ax",@progbits
+; NSECTIONS-NOT:  {{.section|.text|.data}}
+; NSECTIONS:    i:
+; NSECTIONS-NOT:  {{.section|.text|.data}}
+; NSECTIONS:    x:
+; NSECTIONS-NOT:  {{.section|.text|.data}}
+; NSECTIONS:    y:
+; NSECTIONS:    .data
+; NSECTIONS-NOT:  {{.section|.text|.data}}
+; NSECTIONS:    z:
+; NSECTIONS:    .section  aaa,"ax",@progbits
+; NSECTIONS-NOT:  {{.section|.text|.data}}
+; NSECTIONS:    w:
+
+
+; SECTIONS: .section  aaa,"ax",@progbits,unique,1
+; SECTIONS-NOT: {{.section|.text|.data}}
+; SECTIONS: f:
+; SECTIONS: .section  aaa,"ax",@progbits,unique,2
+; SECTIONS-NOT: {{.section|.text|.data}}
+; SECTIONS: g:
+; SECTIONS: .section        .text.h,"ax",@progbits
+; SECTIONS-NOT: {{.section|.text|.data}}
+; SECTIONS: h:
+; SECTIONS: .section  aaa,"ax",@progbits,unique,3
+; SECTIONS-NOT: {{.section|.text|.data}}
+; SECTIONS: i:
+; SECTIONS: .section  aaa,"aw",@progbits,unique,4
+; SECTIONS-NOT: {{.section|.text|.data}}
+; SECTIONS: x:
+; SECTIONS: .section  aaa,"aw",@progbits,unique,5
+; SECTIONS-NOT: {{.section|.text|.data}}
+; SECTIONS: y:
+; SECTIONS: .section        .data.z,"aw",@progbits
+; SECTIONS-NOT: {{.section|.text|.data}}
+; SECTIONS: z:
+; SECTIONS: .section  aaa,"aw",@progbits,unique,6
+; SECTIONS-NOT: {{.section|.text|.data}}
+; SECTIONS: w:
+
+