[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

855

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

524

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,
   if (!S)
     return nullptr;
   if (!S->isUndefined() && !S->isShared())
     return S->symbol();
   return 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)
     return;
   StringRef S = Config->Rela ? "__rela_iplt_start" : "__rel_iplt_start";
if (!Out<ELFT>::EhFrame->empty())
     Add(Out<ELFT>::EhFrameHdr);
   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) {
+  auto Add = [](StringRef Name, OutputSectionBase<ELFT> *OS, uintX_t Value) {
     if (OS) {
-      Symtab<ELFT>::X->addSynthetic(Start, OS, 0);
-      Symtab<ELFT>::X->addSynthetic(End, OS,
-                                    DefinedSynthetic<ELFT>::SectionEnd);
+      SymbolBody *B = Symtab<ELFT>::X->find(Name);
+      if (!B || B->isUndefined() || B->isShared())
+        Symtab<ELFT>::X->addSynthetic(Name, OS, Value);
     } else {
-      addOptionalSynthetic(Start, (OutputSectionBase<ELFT> *)nullptr, 0);
-      addOptionalSynthetic(End, (OutputSectionBase<ELFT> *)nullptr, 0);
+      addOptionalSynthetic(Name, OS, Value);
     }
   };
 
+  auto Define = [&](StringRef Start, StringRef End,
+                    OutputSectionBase<ELFT> *OS) {
+    Add(Start, OS, 0);
+    Add(End, OS, OS ? DefinedSynthetic<ELFT>::SectionEnd : 0);
+  };
+
   Define("__preinit_array_start", "__preinit_array_end",
          Out<ELFT>::PreinitArray);
   Define("__init_array_start", "__init_array_end", Out<ELFT>::InitArray);
   Define("__fini_array_start", "__fini_array_end", Out<ELFT>::FiniArray);
 }
 
 // If a section name is valid as a C identifier (which is rare because of
 // the leading '.'), linkers are expected to define __start_<secname> and

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