ELF: Move sections referred to by __start_/__stop_ symbols into the main partition.

[pcc]

Summary

In the case where C identifier sections have SHF_LINK_ORDER they will most
likely be placed in the same partition as the section that they are associated
with. But unless this happens to be the main partition, this will cause them
to be excluded from the range covered by the start_ and stop_ symbols,
which may lead to incorrect program behaviour. So we need to move them
all into the main partition so that they will be covered by the start_
and stop_ symbols.

We may want to refine this approach later and allow different start_/stop_
symbol values for different partitions. This would only make sense for
relocations from SHT_NOTE sections since they are duplicated into each
partition.

[pcc]

• Add test

[MaskRay]

Makes sense. A couple of nits:

lld/ELF/MarkLive.cpp

299 ↗

(On Diff #214019)

DenseSet<StringRef> can avoid allocation for strings.

308 ↗

(On Diff #214019)

if (name.consume_front("__start_") || name.consume_front("__stop_"))
  startStopSymbolNames.insert(name);

lld/test/ELF/partition-move-to-main-startstop.s

17 ↗

(On Diff #214019)

You can also use FileCheck %s --implicit-check-not=has_startstop and delete CHECK-NOT:

[ruiu]

lld/ELF/MarkLive.cpp

299 ↗

(On Diff #214019)

In this patch you scan over all input files and their symbol tables to see if there's a symbol starting with "start_" or "stop_", but do you have to do this that way? I mean

1. most section names are not a valid C identifier in the first place,
2. if section whose name "foo" exists, look up the symbol table with "start_foo" and "end_foo" to see if there's a symbol that refer to the start/end of the sectoin

Am I missing somehting?

[MaskRay]

lld/ELF/MarkLive.cpp

299 ↗

(On Diff #214019)

The

for (InputFile *file : objectFiles)
  for (Symbol *s : file->getSymbols())

loop already exists because STT_GNU_IFUNC and STT_TLS are special cased. The loop gets reused to check __start_ and __stop_. I am not sure an alternative approach will optimize it.

[ruiu]

lld/ELF/MarkLive.cpp

299 ↗

(On Diff #214019)

Yeah, performance-wise I don't know which is better, but I feel like looking up the symbol table is more natural than scanning all symbols if we are handling symbol names.

[pcc]

• Switch to a symbol table lookup
• Use --implicit-check-not

lld/ELF/MarkLive.cpp

299 ↗

(On Diff #214019)

With regard to point 1, in certain sanitizer builds every global will have an associated C identifier named section. So depending on your build mode the number may not be that small.

That said, I couldn't measure a significant difference in link speed between the two options even in a HWASAN build of Chromium (if anything, the symbol lookup seems to be a little faster), so I'd be fine with either option. The symbol lookup turns out to be less code, so let's go with that.

[ruiu]

LGTM

lld/trunk/ELF/MarkLive.cpp

template <class ELFT> void MarkLive<ELFT>::mark() {
   }
 }
 
 // Move the sections for some symbols to the main partition, specifically ifuncs
 // (because they can result in an IRELATIVE being added to the main partition's
 // GOT, which means that the ifunc must be available when the main partition is
 // loaded) and TLS symbols (because we only know how to correctly process TLS
 // relocations for the main partition).
+//
+// We also need to move sections whose names are C identifiers that are referred
+// to from __start_/__stop_ symbols because there will only be one set of
+// symbols for the whole program.
 template <class ELFT> void MarkLive<ELFT>::moveToMain() {
   for (InputFile *file : objectFiles)
     for (Symbol *s : file->getSymbols())
       if (auto *d = dyn_cast<Defined>(s))
         if ((d->type == STT_GNU_IFUNC || d->type == STT_TLS) && d->section &&
             d->section->isLive())
           markSymbol(s);
 
+  for (InputSectionBase *sec : inputSections) {
+    if (!sec->isLive() || !isValidCIdentifier(sec->name))
+      continue;
+    if (symtab->find(("__start_" + sec->name).str()) ||
+        symtab->find(("__stop_" + sec->name).str()))
+      enqueue(sec, 0);
+  }
+
   mark();
 }
 
 // Before calling this function, Live bits are off for all
 // input sections. This function make some or all of them on
 // so that they are emitted to the output file.
 template <class ELFT> void elf::markLive() {
   // If -gc-sections is not given, no sections are removed.

lld/trunk/test/ELF/partition-move-to-main-startstop.s

+// REQUIRES: x86
+// RUN: llvm-mc %s -o %t.o -filetype=obj --triple=x86_64-unknown-linux
+// RUN: ld.lld %t.o -o %t --export-dynamic --gc-sections
+// RUN: llvm-readelf -S %t | FileCheck --implicit-check-not=has_startstop %s
+
+// We can't let the has_startstop section be split by partition because it is
+// referenced by __start_ and __stop_ symbols, so the split could result in
+// some sections being moved out of the __start_/__stop_ range. Make sure that
+// that didn't happen by checking that there is only one section.
+//
+// It's fine for us to split no_startstop because of the lack of
+// __start_/__stop_ symbols.
+
+// CHECK: has_startstop
+// CHECK: no_startstop
+
+// CHECK: no_startstop
+
+.section .llvm_sympart.f1,"",@llvm_sympart
+.asciz "part1"
+.quad f1
+
+.section .text._start,"ax",@progbits
+.globl _start
+_start:
+call __start_has_startstop
+call __stop_has_startstop
+
+.section .text.f1,"ax",@progbits
+.globl f1
+f1:
+
+.section has_startstop,"ao",@progbits,.text._start,unique,1
+.quad 1
+
+.section has_startstop,"ao",@progbits,.text.f1,unique,2
+.quad 2
+
+.section no_startstop,"ao",@progbits,.text._start,unique,1
+.quad 3
+
+.section no_startstop,"ao",@progbits,.text.f1,unique,2
+.quad 4