ELF: Create dynamic symbols for symbol aliases of copy relocations.

[pcc]

Summary

For each copy relocation that we create, look through the DSO's symbol table
for aliases and create a dynamic symbol for each one. This causes the copy
relocation to correctly interpose any aliases.

Copy relocations are relatively uncommon (on my machine, 56% of binaries in
/usr/bin have no copy relocations probably due to being PIEs, 97% of them
have <10, and the binary with the largest number of them has 97) so it's
probably fine to do this in a relatively inefficient way.

[ruiu]

ELF/Writer.cpp

388 ↗

(On Diff #52479)

How is it guaranteed that this function is not called more than once for each shared symbol?

[pcc]

ELF/Writer.cpp

388 ↗

(On Diff #52479)

We set the NeedsCopyOrPltAddr flag on each alias. That causes the test on line 385 to succeed.

[pcc]

Use a sorted list of object aliases

[pcc]

For comparison, here's a version of the patch that produces a list of object aliases for the DSO sorted by symbol address upon first encountering a copy relocation, and uses that to optimize lookups. Perf improvement was ~1% for CMake's ctest binary, one of the heavier users of copy relocations on my system. Not sure if this is worth the added code complexity, though.

[• rafael]

I will take a look at the patch, but I agree that when copy relocations are
involved we should push for simplicity.

[ruiu]

I think the original one is better because as you said copy relocations are rather rare.

ELF/OutputSections.cpp

842 ↗

(On Diff #52479)

What is this change for?

ELF/Writer.cpp

764 ↗

(On Diff #52479)

You set MustBeInDynSym true here for all symbols at the same offset. Assume that you have symbols A and B where B is a weak alias to A. If your program uses symbol A, then will both A and B be in the program's dynsym? Is this expected?

[• rafael]

Please check all relocations in the test.

Please check the section of the symbols.

[pcc]

Okay, I'll revert to the previous version.

ELF/OutputSections.cpp

842 ↗

(On Diff #52617)

This change moves copy relocation allocation from after offset assignment to before assignment. This function needs to take into account any space allocated that way.

ELF/Writer.cpp

816 ↗

(On Diff #52617)

Yes, both symbols will be in the dynsym. I think we probably need to create dynsyms for all symbols at the same offset regardless of whether they are strong or weak, as otherwise the DSO will be able to observe breakage.

e.g. if the DSO looks like this:

int a;
__attribute__((weak, alias("a"))) int b;

bool f() {
  return &a == &b;
}

the function f should return true regardless of which symbols are used in the main executable (if they're being overridden, that's another matter, though).

[ruiu]

ELF/Writer.cpp

816 ↗

(On Diff #52617)

Makes sense, although it's very subtle. But I don't know how to fix it. Probably the very notion of the copy relocation is subtle.

[pcc]

Please check all relocations in the test.

I don't understand what you're asking for. There are two relocations, and I'm checking both of them (and that there are no others).

Please check the section of the symbols.

Done.

[pcc]

• Revert to previous version of the patch; improve test

[pcc]

• Use CHECK-NEXT to check the whole relocation block instead of CHECK-NOT

[• rafael]

I am pretty sure this is OK. Just give me a minute to dig why gold treats weak specially.

ELF/Writer.cpp

752 ↗

(On Diff #52627)

Just merge this on the previous line now that the loop is gone.

764 ↗

(On Diff #52627)

I think this is expected. Consider the case:

Program uses LibA and LibB.
LibA also uses LibB.
Program has copy relocation to X of LibB.
LibA assumes that X and Y have the same address.

Under these conditions the Program has to have both X and Y.

Adding something like that to the comment would be nice :-)

[pcc]

• Simplify

[ruiu]

LGTM

ELF/Writer.cpp

759–760 ↗

(On Diff #52629)

Let's use a shorter name and early continue.

for (SharedSymbol<ELFT> &S : SS->File->getSharedSymbols()) {
  if (S.Sym.st_shndx != Shndx || S.Sym.st_value != Value)
    continue;

[• rafael]

ELF/Writer.cpp

765 ↗

(On Diff #52629)

Why do you need this?

Can you leave it out and just pass SS to addReloc?

[pcc]

ELF/Writer.cpp

765 ↗

(On Diff #52629)

I observed that gold was creating a copy relocation to the strong symbol if the referenced symbol was weak. That's not a very good justification though, and nothing seems to break if I do as you suggest, so let's do that.

lld/trunk/ELF/OutputSections.cpp

static int getPriority(StringRef S) {
   if (S.substr(Pos + 1).getAsInteger(10, V))
     return 65536;
   return V;
 }
 
 // This function is called after we sort input sections
 // and scan relocations to setup sections' offsets.
 template <class ELFT> void OutputSection<ELFT>::assignOffsets() {
-  uintX_t Off = 0;
+  uintX_t Off = this->Header.sh_size;
   for (InputSection<ELFT> *S : Sections) {
     Off = alignTo(Off, S->Align);
     S->OutSecOff = Off;
     Off += S->getSize();
   }
   this->Header.sh_size = Off;
 }
 

lld/trunk/ELF/Writer.cpp

bool isOutputDynamic() const {
     return !Symtab.getSharedFiles().empty() || Config->Pic;
   }
   template <class RelTy>
   void scanRelocsForThunks(const elf::ObjectFile<ELFT> &File,
                            iterator_range<const RelTy *> Rels);
 
   void ensureBss();
   void addCommonSymbols(std::vector<DefinedCommon *> &Syms);
-  void addCopyRelSymbols(std::vector<SharedSymbol<ELFT> *> &Syms);
+  void addCopyRelSymbol(SharedSymbol<ELFT> *Sym);
 
   std::unique_ptr<llvm::FileOutputBuffer> Buffer;
 
   BumpPtrAllocator Alloc;
   std::vector<OutputSectionBase<ELFT> *> OutputSections;
   std::vector<std::unique_ptr<OutputSectionBase<ELFT>>> OwningSections;
 
   // We create a section for the ELF header and one for the program headers.
if (Target->needsDynRelative(Type))
                                     &Body, getAddend<ELFT>(RI)});
 
     // If a symbol in a DSO is referenced directly instead of through GOT,
     // we need to create a copy relocation for the symbol.
     if (auto *B = dyn_cast<SharedSymbol<ELFT>>(&Body)) {
       if (B->needsCopy())
         continue;
       if (Target->needsCopyRel<ELFT>(Type, *B)) {
-        B->NeedsCopyOrPltAddr = true;
-        Out<ELFT>::RelaDyn->addReloc(
-            {Target->CopyRel, DynamicReloc<ELFT>::Off_Bss, B});
+        addCopyRelSymbol(B);
         continue;
       }
     }
 
     // If a relocation needs PLT, we create a PLT and a GOT slot
     // for the symbol.
     TargetInfo::PltNeed NeedPlt = Target->needsPlt(Type, Body);
     if (NeedPlt) {
template <class ELFT> static uint32_t getAlignment(SharedSymbol<ELFT> *SS) {
 
   uintX_t SecAlign = SS->File->getSection(SS->Sym)->sh_addralign;
   uintX_t SymValue = SS->Sym.st_value;
   int TrailingZeros = std::min(countTrailingZeros(SecAlign),
                                countTrailingZeros(SymValue));
   return 1 << TrailingZeros;
 }
 
-// Reserve space in .bss for copy relocations.
+// Reserve space in .bss for copy relocation.
 template <class ELFT>
-void Writer<ELFT>::addCopyRelSymbols(std::vector<SharedSymbol<ELFT> *> &Syms) {
-  if (Syms.empty())
-    return;
+void Writer<ELFT>::addCopyRelSymbol(SharedSymbol<ELFT> *SS) {
   ensureBss();
   uintX_t Off = Out<ELFT>::Bss->getSize();
-  uintX_t MaxAlign = Out<ELFT>::Bss->getAlign();
-  for (SharedSymbol<ELFT> *SS : Syms) {
-    uintX_t Align = getAlignment(SS);
-    Off = alignTo(Off, Align);
-    SS->OffsetInBss = Off;
-    Off += SS->template getSize<ELFT>();
-    MaxAlign = std::max(MaxAlign, Align);
+  Out<ELFT>::Bss->setSize(Off + SS->template getSize<ELFT>());
+  Out<ELFT>::Bss->updateAlign(Align);
+  Out<ELFT>::RelaDyn->addReloc(
+      {Target->CopyRel, DynamicReloc<ELFT>::Off_Bss, SS});
+  uintX_t Shndx = SS->Sym.st_shndx;
+  uintX_t Value = SS->Sym.st_value;
+  // Look through the DSO's dynamic symbol for aliases and create a dynamic
+  // symbol for each one. This causes the copy relocation to correctly interpose
+  // any aliases.
+  for (SharedSymbol<ELFT> &S : SS->File->getSharedSymbols()) {
+    if (S.Sym.st_shndx != Shndx || S.Sym.st_value != Value)
+      continue;
+    S.OffsetInBss = Off;
+    S.NeedsCopyOrPltAddr = true;
+    S.setUsedInRegularObj();
+    S.MustBeInDynSym = true;
   }
-  Out<ELFT>::Bss->setSize(Off);
-  Out<ELFT>::Bss->updateAlign(MaxAlign);
 }
 
 template <class ELFT>
 StringRef Writer<ELFT>::getOutputSectionName(InputSectionBase<ELFT> *S) const {
   StringRef Dest = Script->getOutputSection<ELFT>(S);
   if (!Dest.empty())
     return Dest;
 
template <class ELFT> void Writer<ELFT>::createSections() {
   }
 
   for (OutputSectionBase<ELFT> *Sec : getSections())
     Sec->assignOffsets();
 
   // Now that we have defined all possible symbols including linker-
   // synthesized ones. Visit all symbols to give the finishing touches.
   std::vector<DefinedCommon *> CommonSymbols;
-  std::vector<SharedSymbol<ELFT> *> CopyRelSymbols;
   for (auto &P : Symtab.getSymbols()) {
     SymbolBody *Body = P.second->Body;
     if (auto *U = dyn_cast<Undefined>(Body))
       if (!U->isWeak() && !U->canKeepUndefined())
         reportUndefined<ELFT>(Symtab, Body);
 
     if (auto *C = dyn_cast<DefinedCommon>(Body))
       CommonSymbols.push_back(C);
-    if (auto *SC = dyn_cast<SharedSymbol<ELFT>>(Body))
-      if (SC->needsCopy())
-        CopyRelSymbols.push_back(SC);
 
     if (!includeInSymtab<ELFT>(*Body))
       continue;
     if (Out<ELFT>::SymTab)
       Out<ELFT>::SymTab->addSymbol(Body);
 
     if (isOutputDynamic() && includeInDynsym(*Body))
       Out<ELFT>::DynSymTab->addSymbol(Body);
   }
 
   // Do not proceed if there was an undefined symbol.
   if (HasError)
     return;
 
   addCommonSymbols(CommonSymbols);
-  addCopyRelSymbols(CopyRelSymbols);
 
   // So far we have added sections from input object files.
   // This function adds linker-created Out<ELFT>::* sections.
   addPredefinedSections();
 
   std::stable_sort(OutputSections.begin(), OutputSections.end(),
                    compareSections<ELFT>);
 

lld/trunk/test/ELF/Inputs/relocation-copy-alias.s

+.data
+
+.globl a1
+.type a1, @object
+.size a1, 1
+a1:
+.weak a2
+.type a2, @object
+.size a2, 1
+a2:
+.byte 1
+
+.weak b1
+.type b1, @object
+.size b1, 1
+b1:
+.weak b2
+.type b2, @object
+.size b2, 1
+b2:
+.globl b3
+.type b3, @object
+.size b3, 1
+b3:
+.byte 1

lld/trunk/test/ELF/relocation-copy-alias.s

+// REQUIRES: x86
+// RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t.o
+// RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %p/Inputs/relocation-copy-alias.s -o %t2.o
+// RUN: ld.lld -shared %t2.o -o %t.so
+// RUN: ld.lld %t.o %t.so -o %t3
+// RUN: llvm-readobj --dyn-symbols -r --expand-relocs %t3 | FileCheck %s
+
+.global _start
+_start:
+movl $5, a1
+movl $5, b1
+movl $5, b2
+
+// CHECK:      .rela.dyn {
+// CHECK-NEXT:   Relocation {
+// CHECK-NEXT:     Offset:
+// CHECK-NEXT:     Type: R_X86_64_COPY
+// CHECK-NEXT:     Symbol: a1
+// CHECK-NEXT:     Addend: 0x0
+// CHECK-NEXT:   }
+// CHECK-NEXT:   Relocation {
+// CHECK-NEXT:     Offset:
+// CHECK-NEXT:     Type: R_X86_64_COPY
+// CHECK-NEXT:     Symbol: b1
+// CHECK-NEXT:     Addend: 0x0
+// CHECK-NEXT:   }
+// CHECK-NEXT: }
+
+// CHECK:      Name: a1
+// CHECK-NEXT: Value: [[A:.*]]
+// CHECK-NEXT: Size: 1
+// CHECK-NEXT: Binding: Global (0x1)
+// CHECK-NEXT: Type: Object (0x1)
+// CHECK-NEXT: Other: 0
+// CHECK-NEXT: Section: .bss (0x7)
+
+// CHECK:      Name: b1
+// CHECK-NEXT: Value: [[B:.*]]
+// CHECK-NEXT: Size: 1
+// CHECK-NEXT: Binding: Weak (0x2)
+// CHECK-NEXT: Type: Object (0x1)
+// CHECK-NEXT: Other: 0
+// CHECK-NEXT: Section: .bss
+
+// CHECK:      Name: b2
+// CHECK-NEXT: Value: [[B]]
+// CHECK-NEXT: Size: 1
+// CHECK-NEXT: Binding: Weak (0x2)
+// CHECK-NEXT: Type: Object (0x1)
+// CHECK-NEXT: Other: 0
+// CHECK-NEXT: Section: .bss
+
+// CHECK:      Name: a2
+// CHECK-NEXT: Value: [[A]]
+// CHECK-NEXT: Size: 1
+// CHECK-NEXT: Binding: Weak (0x2)
+// CHECK-NEXT: Type: Object (0x1)
+// CHECK-NEXT: Other: 0
+// CHECK-NEXT: Section: .bss
+
+// CHECK:      Name: b3
+// CHECK-NEXT: Value: [[B]]
+// CHECK-NEXT: Size: 1
+// CHECK-NEXT: Binding: Global (0x1)
+// CHECK-NEXT: Type: Object (0x1)
+// CHECK-NEXT: Other: 0
+// CHECK-NEXT: Section: .bss