ELF: Give automatically generated __start_* and __stop_* symbols hidden visibility.

[pcc]

Summary

These symbols describe a property of a linkage unit, so it seems reasonable
to limit their visibility to the linkage unit. Furthermore the use cases I
am aware of do not require more than hidden visibility.

This is a departure from the behavior of the bfd and gold linkers. However,
it is unclear that the decision to give these symbols default visibility
in those linkers was made deliberately. The start_*/stop_* feature
was added to the bfd linker in 1994 [1], while the visibility feature was
added about five years later [2], so it may have been that the visibility
of these symbols was not considered. The feature was implemented in gold
[3] in the same way; the behavior may have simply been copied from bfd.

The only related discussion I could find on the binutils mailing list [4]
was a user issue which would most likely not have occurred if the symbols
had hidden visibility.

[1] https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=5efddb2e7c3229b569a862205f61d42860af678b
[2] https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=0fc731e447cd01e7fc35197b487ff0e4fd25afca
[3] https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=bfd58944a64b0997a310b95fbe0423338961e71c
[4] https://sourceware.org/ml/binutils/2014-05/msg00011.html

[ruiu]

Software archaeology is always interesting, and I agree with you that not making these symbols hidden was by accident. I'm not against doing this, but do you have any specific motivation of doing this?

[pcc]

I tracked down breakage in one of Chromium's test binaries to code that was doing something like this (in both an executable and a DSO):

__attribute__((section("foo"))) void f() { .... }

void g() {
  ... __start_foo ...
}

In this case, the definition of __start_foo in the executable was coming from the DSO because we only check for undefined symbols when creating the synthetic symbol. If the executable is non-PIC, we cannot relocate the reference to __start_foo in g correctly, and we end up emitting a dynamic relocation for the .text section which causes a segfault at startup.

One way we could solve that problem is by replacing shared definitions with our synthetic symbols (that would make the behavior consistent with other synthetic symbols). But separate from that, I had to ask myself why we are making these symbols default visible in the first place, and I could not see a good reason.

[ruiu]

LGTM.

Thank you for the explanation, and I agree that this is essentially the direction we'd like to see (eliminating a problem from the root cause instead of resolving a problem by adding more and more features). Let's give it a shot.

lld/trunk/ELF/Writer.cpp

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.find(Start))
     if (B->isUndefined())
-      Symtab.addSynthetic(Start, *Sec, 0, STV_DEFAULT);
+      Symtab.addSynthetic(Start, *Sec, 0, STV_HIDDEN);
   if (SymbolBody *B = Symtab.find(Stop))
     if (B->isUndefined())
       Symtab.addSynthetic(Stop, *Sec, DefinedSynthetic<ELFT>::SectionEnd,
-                          STV_DEFAULT);
+                          STV_HIDDEN);
 }
 
 template <class ELFT> static bool needsPtLoad(OutputSectionBase<ELFT> *Sec) {
   if (!(Sec->getFlags() & SHF_ALLOC))
     return false;
 
   // Don't allocate VA space for TLS NOBITS sections. The PT_TLS PHDR is
   // responsible for allocating space for them, not the PT_LOAD that

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
 
         .quad __start_foo
         .section foo,"a"
-// By default the symbol is visible and we need a dynamic reloc.
-// CHECK:  R_X86_64_64 __start_foo 0x0
+// By default the symbol is hidden.
+// CHECK:      R_X86_64_RELATIVE - 0x[[ADDR1:.*]]
 
         .hidden __start_bar
         .quad __start_bar
         .section bar,"a"
-// Test that we are able to hide the symbol.
-// CHECK:      R_X86_64_RELATIVE - 0x[[ADDR:.*]]
+// References do not affect the visibility.
+// CHECK:      R_X86_64_RELATIVE - 0x[[ADDR2:.*]]
 
 // CHECK:      Name: __start_bar
-// CHECK-NEXT: Value: 0x[[ADDR]]
+// CHECK-NEXT: Value: 0x[[ADDR2]]
 // CHECK-NEXT: Size:
 // CHECK-NEXT: Binding: Local
 
 // CHECK:      Name: __start_foo
-// CHECK-NEXT: Value:
+// CHECK-NEXT: Value: 0x[[ADDR1]]
 // CHECK-NEXT: Size:
-// CHECK-NEXT: Binding: Global
+// CHECK-NEXT: Binding: Local