[ELF] Do not add start and end symbols in case they are defined in linker script

[evgeny777]

Summary

Without this patch, the following script triggers link error

SECTIONS {
   .init_array : { __init_array_start = .; *(.init_array) __init_array_end = .; }
}

[ruiu]

This change seems too subtle. Why don't you not call addStartEndSymbols if Scripts->HasContents?

[evgeny777]

I thought it would make sense to add those symbols if linker script does not provide them for some reason. For example .init_array can be an orphan section, right? Also I wonder what would happen if I define __init_array_start somewhere in my program.

[ruiu]

Ah, okay, so this change is not really specific to the linker script, but to not define _start/_end symbols if there are already definitions. You want to add a comment about that.

ELF/Writer.cpp

864

Looks like this is getting unnecessarily complicated. Two stages of lambdas? Can you simplify this function if you rewrite it?

[evgeny777]

Addressed review comments

[evgeny777]

Simplified

[ruiu]

LGTM

ELF/Writer.cpp

525

Is Required a good name? Maybe I'd name this simply addSynthetic and add a comment to describe its semantics (defines a symbol unless there's already a defined symbol with the same name.)

ELF/Writer.cpp

static Symbol *addOptionalSynthetic(StringRef Name,
   SymbolBody *S = Symtab<ELFT>::X->find(Name);
   if (!S)
     return nullptr;
   if (!S->isUndefined() && !S->isShared())
     return S->symbol();
   return Symtab<ELFT>::X->addSynthetic(Name, Sec, Val);
 }
 
+template <class ELFT>
+static void addRequiredSynthetic(StringRef Name, OutputSectionBase<ELFT> *Sec,
+                                 typename ELFT::uint Val) {
+  SymbolBody *S = Symtab<ELFT>::X->find(Name);
+  if (!S || S->isUndefined() || S->isShared())
+    Symtab<ELFT>::X->addSynthetic(Name, Sec, Val);
+}
 // The beginning and the ending of .rel[a].plt section are marked
 // with __rel[a]_iplt_{start,end} symbols if it is a statically linked
 // executable. The runtime needs these symbols in order to resolve
 // all IRELATIVE relocs on startup. For dynamic executables, we don't
 // need these symbols, since IRELATIVE relocs are resolved through GOT
 // and PLT. For details, see http://www.airs.com/blog/archives/403.
 template <class ELFT> void Writer<ELFT>::addRelIpltSymbols() {
   if (Out<ELFT>::DynSymTab || !Out<ELFT>::RelaPlt)
template <class ELFT> void Writer<ELFT>::addPredefinedSections() {
   if (Out<ELFT>::Bss->getSize() > 0)
     Add(Out<ELFT>::Bss);
 }
 
 // The linker is expected to define SECNAME_start and SECNAME_end
 // symbols for a few sections. This function defines them.
 template <class ELFT> void Writer<ELFT>::addStartEndSymbols() {
   auto Define = [&](StringRef Start, StringRef End,
                     OutputSectionBase<ELFT> *OS) {
     if (OS) {
-      Symtab<ELFT>::X->addSynthetic(Start, OS, 0);
-      Symtab<ELFT>::X->addSynthetic(End, OS,
-                                    DefinedSynthetic<ELFT>::SectionEnd);
+      addRequiredSynthetic(Start, OS, 0);
+      addRequiredSynthetic(End, OS, DefinedSynthetic<ELFT>::SectionEnd);
     } else {
       addOptionalSynthetic(Start, (OutputSectionBase<ELFT> *)nullptr, 0);
       addOptionalSynthetic(End, (OutputSectionBase<ELFT> *)nullptr, 0);
     }
   };
 
   Define("__preinit_array_start", "__preinit_array_end",
          Out<ELFT>::PreinitArray);

test/ELF/linkerscript/linkerscript-start-end.s

+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t.o
+# RUN: echo "SECTIONS { \
+# RUN:      .init_array : { \
+# RUN:        __init_array_start = .; \
+# RUN:        *(.init_array) \
+# RUN:        __init_array_end = .; } }" > %t.script
+# RUN: ld.lld %t.o -script %t.script -o %t 2>&1
+
+.globl _start
+.text
+_start:
+  nop
+
+.section .init_array, "aw"
+  .quad 0