[ELF] - Give automatically generated __start_* and __stop_* symbols default visibility.

[grimar]

Summary

This patch is opposite to D19024, which made this symbols to be hidden by default.

Unfortunately as was pointed by Sean Silva,
looks that FreeBSD loader wants to see start_set_modmetadata_set/stop_set_modmetadata_set in the dynamic symbol table.
They were not placed there because had hidden visibility.

Patch makes them to have default visibility again.

[silvas]

The argument for making them hidden in D19024 is pretty reasonable. But I think it is also reasonable to be compatible if real code relies on this behavior.

Considering how difficult it can be to debug the type of issues that occur due to our incompatible behavior (e.g. kernel does not boot), it is not a good experience for a user of LLD.

[ruiu]

ELF/SymbolTable.cpp

425 ↗

(On Diff #68164)

I'm not the type of person who wants to eliminate all constants, but 3? It's too magical.

[grimar]

ELF/SymbolTable.cpp

425 ↗

(On Diff #68164)

Actually it is used in many places in lld. I count 4 more places in this file (SymbolTable.cpp) and also once in Symbols.h:

uint8_t getVisibility() const { return StOther & 0x3; }

I suggest to leave it for now and fix once for all places if you want it.

[ruiu]

ELF/SymbolTable.cpp

425 ↗

(On Diff #68164)

The use of 0x3 in getVisibility is fine because it is obvious that it is extracting visibility bits. It actually hides the use of 0x3 and wraps it with a better name (getVisibility). But in this context there's no information as to what you are doing.

[grimar]

ELF/SymbolTable.cpp

425 ↗

(On Diff #68164)

What about adding comment then ?

bool WasInserted;
std::tie(S, WasInserted) =
    insert(N, STT_NOTYPE, StOther & 0x3 /*Visibility*/, /*CanOmitFromDynSym*/ false,

Or do you prefer static method for that ?

static unsigned getVisibility(unsigned O) { 
  return O & 0x3;
}

...

  std::tie(S, WasInserted) =
      insert(N, STT_NOTYPE, getVisibility(StOther), /*CanOmitFromDynSym*/ false,

[ruiu]

ELF/SymbolTable.cpp

425 ↗

(On Diff #68164)

A quick google search showed me that there's macros to get the value, ELF32_ST_VISIBILITY and ELF64_ST_VISIBILITY. Unfortunately they are as confusing as the mask because of ELF32/ELF64 prefixes (although what they do is the same.) So I'd go with a comment.

[grimar]

ELF/SymbolTable.cpp

425 ↗

(On Diff #68164)

Yes, that is why I did not use them + also do not sure if we were using macroses in lld before.

[ruiu]

LGTM with a comment. Thanks!

lld/trunk/ELF/LinkerScript.cpp

 template <class ELFT>
 static void addRegular(SymbolAssignment *Cmd) {
   Symbol *Sym = Symtab<ELFT>::X->addRegular(Cmd->Name, STB_GLOBAL, STV_DEFAULT);
   Sym->Visibility = Cmd->Hidden ? STV_HIDDEN : STV_DEFAULT;
   Cmd->Sym = Sym->body();
 }
 
 template <class ELFT> static void addSynthetic(SymbolAssignment *Cmd) {
-  Symbol *Sym = Symtab<ELFT>::X->addSynthetic(Cmd->Name, nullptr, 0);
-  Sym->Visibility = Cmd->Hidden ? STV_HIDDEN : STV_DEFAULT;
+  Symbol *Sym = Symtab<ELFT>::X->addSynthetic(
+      Cmd->Name, nullptr, 0, Cmd->Hidden ? STV_HIDDEN : STV_DEFAULT);
   Cmd->Sym = Sym->body();
 }
 
 // If a symbol was in PROVIDE(), we need to define it only when
 // it is an undefined symbol.
 template <class ELFT> static bool shouldDefine(SymbolAssignment *Cmd) {
   if (Cmd->Name == ".")
     return false;

lld/trunk/ELF/SymbolTable.h

public:
   Symbol *addUndefined(StringRef Name);
   Symbol *addUndefined(StringRef Name, uint8_t Binding, uint8_t StOther,
                        uint8_t Type, bool CanOmitFromDynSym, InputFile *File);
 
   Symbol *addRegular(StringRef Name, const Elf_Sym &Sym,
                      InputSectionBase<ELFT> *Section);
   Symbol *addRegular(StringRef Name, uint8_t Binding, uint8_t StOther);
   Symbol *addSynthetic(StringRef N, OutputSectionBase<ELFT> *Section,
-                       uintX_t Value);
+                       uintX_t Value, uint8_t StOther);
   void addShared(SharedFile<ELFT> *F, StringRef Name, const Elf_Sym &Sym,
                  const typename ELFT::Verdef *Verdef);
 
   void addLazyArchive(ArchiveFile *F, const llvm::object::Archive::Symbol S);
   void addLazyObject(StringRef Name, LazyObjectFile &Obj);
   Symbol *addBitcode(StringRef Name, bool IsWeak, uint8_t StOther, uint8_t Type,
                      bool CanOmitFromDynSym, BitcodeFile *File);
 

lld/trunk/ELF/SymbolTable.cpp

Symbol *SymbolTable<ELFT>::addRegular(StringRef Name, uint8_t Binding,
   else if (Cmp == 0)
     reportDuplicate(S->body(), nullptr);
   return S;
 }
 
 template <typename ELFT>
 Symbol *SymbolTable<ELFT>::addSynthetic(StringRef N,
                                         OutputSectionBase<ELFT> *Section,
-                                        uintX_t Value) {
+                                        uintX_t Value, uint8_t StOther) {
   Symbol *S;
   bool WasInserted;
-  std::tie(S, WasInserted) =
-      insert(N, STT_NOTYPE, STV_HIDDEN, /*CanOmitFromDynSym*/ false,
+  std::tie(S, WasInserted) = insert(N, STT_NOTYPE, /*Visibility*/ StOther & 0x3,
+                                    /*CanOmitFromDynSym*/ false,
                                     /*IsUsedInRegularObj*/ true, nullptr);
   int Cmp = compareDefinedNonCommon<ELFT>(S, WasInserted, STB_GLOBAL);
   if (Cmp > 0)
     replaceBody<DefinedSynthetic<ELFT>>(S, N, Value, Section);
   else if (Cmp == 0)
     reportDuplicate(S->body(), nullptr);
   return S;
 }
 

lld/trunk/ELF/Writer.cpp

 static Symbol *addOptionalSynthetic(StringRef Name,
                                     OutputSectionBase<ELFT> *Sec,
                                     typename ELFT::uint Val) {
   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);
+  return Symtab<ELFT>::X->addSynthetic(Name, Sec, Val, STV_HIDDEN);
 }
 
 template <class ELFT>
 static void addSynthetic(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);
+    Symtab<ELFT>::X->addSynthetic(Name, Sec, Val, STV_HIDDEN);
 }
 // 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() {

 // The linker is expected to define some symbols depending on
 // the linking result. This function defines such symbols.
 template <class ELFT> void Writer<ELFT>::addReservedSymbols() {
   if (Config->EMachine == EM_MIPS && !Config->Relocatable) {
     // Define _gp for MIPS. st_value of _gp symbol will be updated by Writer
     // so that it points to an absolute address which is relative to GOT.
     // See "Global Data Symbols" in Chapter 6 in the following document:
     // ftp://www.linux-mips.org/pub/linux/mips/doc/ABI/mipsabi.pdf
-    Symtab<ELFT>::X->addSynthetic("_gp", Out<ELFT>::Got, MipsGPOffset);
+    Symtab<ELFT>::X->addSynthetic("_gp", Out<ELFT>::Got, MipsGPOffset,
+                                  STV_HIDDEN);
 
     // On MIPS O32 ABI, _gp_disp is a magic symbol designates offset between
     // start of function and 'gp' pointer into GOT.
     Symbol *Sym =
         addOptionalSynthetic("_gp_disp", Out<ELFT>::Got, MipsGPOffset);
     if (Sym)
       ElfSym<ELFT>::MipsGpDisp = Sym->body();
 
for (OutputSectionBase<ELFT> *Sec : OutputSections)
       addStartStopSymbols(Sec);
   }
 
   // Add _DYNAMIC symbol. Unlike GNU gold, our _DYNAMIC symbol has no type.
   // It should be okay as no one seems to care about the type.
   // Even the author of gold doesn't remember why gold behaves that way.
   // https://sourceware.org/ml/binutils/2002-03/msg00360.html
   if (Out<ELFT>::DynSymTab)
-    Symtab<ELFT>::X->addSynthetic("_DYNAMIC", Out<ELFT>::Dynamic, 0);
+    Symtab<ELFT>::X->addSynthetic("_DYNAMIC", Out<ELFT>::Dynamic, 0,
+                                  STV_HIDDEN);
 
   // Define __rel[a]_iplt_{start,end} symbols if needed.
   addRelIpltSymbols();
 
   if (!Out<ELFT>::EhFrame->empty()) {
     OutputSections.push_back(Out<ELFT>::EhFrame);
     Out<ELFT>::EhFrame->finalize();
   }
void Writer<ELFT>::addStartStopSymbols(OutputSectionBase<ELFT> *Sec) {
   StringRef S = Sec->getName();
   if (!isValidCIdentifier(S))
     return;
   StringSaver Saver(Alloc);
   StringRef Start = Saver.save("__start_" + S);
   StringRef Stop = Saver.save("__stop_" + S);
   if (SymbolBody *B = Symtab<ELFT>::X->find(Start))
     if (B->isUndefined())
-      Symtab<ELFT>::X->addSynthetic(Start, Sec, 0);
+      Symtab<ELFT>::X->addSynthetic(Start, Sec, 0, B->getVisibility());
   if (SymbolBody *B = Symtab<ELFT>::X->find(Stop))
     if (B->isUndefined())
-      Symtab<ELFT>::X->addSynthetic(Stop, Sec,
-                                    DefinedSynthetic<ELFT>::SectionEnd);
+      Symtab<ELFT>::X->addSynthetic(
+          Stop, Sec, DefinedSynthetic<ELFT>::SectionEnd, B->getVisibility());
 }
 
 template <class ELFT>
 OutputSectionBase<ELFT> *Writer<ELFT>::findSection(StringRef Name) {
   for (OutputSectionBase<ELFT> *Sec : OutputSections)
     if (Sec->getName() == Name)
       return Sec;
   return nullptr;

lld/trunk/test/ELF/startstop-shared.s

 // REQUIRES: x86
 // RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t.o
 // RUN: ld.lld %t.o -o %t.so -shared
 // RUN: llvm-readobj -r -t %t.so | FileCheck  %s
 
         .data
         .quad __start_foo
         .section foo,"aw"
-// By default the symbol is hidden.
-// CHECK:      R_X86_64_RELATIVE - 0x[[ADDR1:.*]]
 
         .hidden __start_bar
         .quad __start_bar
         .section bar,"a"
-// References do not affect the visibility.
-// CHECK:      R_X86_64_RELATIVE - 0x[[ADDR2:.*]]
+
+// Test that we are able to hide the symbol.
+// CHECK:      R_X86_64_RELATIVE - 0x[[ADDR:.*]]
+
+// By default the symbol is visible and we need a dynamic reloc.
+// CHECK:  R_X86_64_64 __start_foo 0x0
 
 // CHECK:      Name: __start_bar
-// CHECK-NEXT: Value: 0x[[ADDR2]]
+// CHECK-NEXT: Value: 0x[[ADDR]]
 // CHECK-NEXT: Size:
 // CHECK-NEXT: Binding: Local
 
 // CHECK:      Name: __start_foo
-// CHECK-NEXT: Value: 0x[[ADDR1]]
+// CHECK-NEXT: Value:
 // CHECK-NEXT: Size:
-// CHECK-NEXT: Binding: Local
+// CHECK-NEXT: Binding: Global

lld/trunk/test/ELF/startstop.s

 // DISASM: __start_bar:
 // DISASM:    1012:       90      nop
 // DISASM:    1013:       90      nop
 // DISASM:    1014:       90      nop
 
 
 // SYMBOL:      Relocations [
 // SYMBOL-NEXT:   Section ({{.*}}) .rela.dyn {
-// SYMBOL-NEXT:     0x3000 R_X86_64_RELATIVE - 0x3020
-// SYMBOL-NEXT:     0x3008 R_X86_64_RELATIVE - 0x3021
-// SYMBOL-NEXT:     0x3010 R_X86_64_RELATIVE - 0x3010
-// SYMBOL-NEXT:     0x3018 R_X86_64_RELATIVE - 0x3011
+// SYMBOL-NEXT:     0x3010 R_X86_64_64 __stop_zed1 0x0
+// SYMBOL-NEXT:     0x3018 R_X86_64_64 __stop_zed1 0x1
+// SYMBOL-NEXT:     0x3000 R_X86_64_64 __stop_zed2 0x0
+// SYMBOL-NEXT:     0x3008 R_X86_64_64 __stop_zed2 0x1
 // SYMBOL-NEXT:   }
 // SYMBOL-NEXT: ]
 
 // SYMBOL: Symbol {
 // SYMBOL:   Name: __start_bar
 // SYMBOL:   Value: 0x1012
+// SYMBOL:   STV_HIDDEN
 // SYMBOL:   Section: bar
 // SYMBOL: }
 // SYMBOL-NOT:   Section: __stop_bar
 // SYMBOL: Symbol {
 // SYMBOL:   Name: __start_foo
 // SYMBOL:   Value: 0x100F
+// SYMBOL:   STV_HIDDEN
 // SYMBOL:   Section: foo
 // SYMBOL: }
 // SYMBOL: Symbol {
 // SYMBOL:   Name: __stop_foo
 // SYMBOL:   Value: 0x1012
+// STMBOL:   STV_HIDDEN
 // SYMBOL:   Section: foo
 // SYMBOL: }
 
+// SYMBOL: Symbol {
+// SYMBOL:   Name: __stop_zed1
+// SYMBOL:   Value: 0x3010
+// STMBOL:   Other: 0
+// SYMBOL:   Section: zed1
+// SYMBOL: }
+// SYMBOL: Symbol {
+// SYMBOL:   Name: __stop_zed2
+// SYMBOL:   Value: 0x3020
+// STMBOL:   Other: 0
+// SYMBOL:   Section: zed2
+// SYMBOL: }
+
 .hidden __start_foo
 .hidden __stop_foo
 .hidden __start_bar
 .global _start
 .text
 _start:
   call __start_foo
   call __stop_foo