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

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

MCSection *TargetLoweringObjectFileELF::getExplicitSectionGlobal(
   unsigned Flags = getELFSectionFlags(Kind);
   if (const Comdat *C = getELFComdat(GO)) {
     Group = C->getName();
     Flags |= ELF::SHF_GROUP;
   }
 
   // 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++;
     Flags |= ELF::SHF_LINK_ORDER;
+  } else if(F && TM.getFunctionSections()){
+    // Also, if -ffunction-sections is used and the global is a function, then
+    // emit a unique section.
+    UniqueID = NextUniqueID++;
   }
 
   MCSectionELF *Section = getContext().getELFSection(
       SectionName, getELFSectionType(SectionName, Kind), Flags,
       getEntrySizeForKind(Kind), Group, UniqueID, AssociatedSymbol);
   // 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->getAssociatedSymbol() == AssociatedSymbol &&
          "Associated symbol mismatch between sections");
   return Section;
 }
 
 /// Return the section prefix name used by options FunctionsSections and
 /// DataSections.
 static StringRef getSectionPrefixForGlobal(SectionKind Kind) {
   if (Kind.isText())
     return ".text";
   if (Kind.isReadOnly())
     return ".rodata";

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

+; RUN: llc < %s -mtriple=x86_64-pc-linux -function-sections | FileCheck %s -check-prefix=FSECTIONS
+; RUN: llc < %s -mtriple=x86_64-pc-linux | FileCheck %s -check-prefix=NOFSECTIONS
+
+define void @f() section "special_section" {
+  ret void
+}
+define void @g() section "special_section" {
+  ret void
+}
+
+; NOFSECTIONS:    .section  special_section,"ax",@progbits
+; NOFSECTIONS-NOT:  section
+; NOFSECTIONS:    f:
+; NOFSECTIONS-NOT:  section
+; NOFSECTIONS:    g:
+
+; FSECTIONS:    .section  special_section,"ax",@progbits
+; FSECTIONS-NOT:  section
+; FSECTIONS:    f:
+; FSECTIONS:    .section  special_section,"ax",@progbits
+; FSECTIONS-NOT:  section
+; FSECTIONS:    g:
+
+