[lld] [ELF] Fix inclusion of weak symbols in the dynamic symbol table

[rafaelauler]

Summary

This patch implements in the X86_64 ELF lld backend yet another feature that was
only available in the MIPS backend. However, this patch changes generic ELF
classes to make it trivial for other ELF backends to use this logic too. When
creating a dynamic executable that has dynamic relocations against weak
undefined symbols, these symbols must be exported to the dynamic symbol table
to seek a possible resolution at run time.

A common use case is the gmon_start weak glibc undefined symbol.

[shankarke]

lib/ReaderWriter/ELF/DefaultLayout.h

567–587

If we are creating a dynamic relocation for a symbol, that symbol should go in the dynamic symbol table. Looks like this code is removing undefined symbols that are targets of dynamic relocations.

[rafaelauler]

Hi Shankar,

Thanks for looking this code. I answered your question below.

lib/ReaderWriter/ELF/DefaultLayout.h

567–587

If the symbol is the target of a dynamic relocation, that relocation should be reported by the backend by "isDynamicRelocation" or "isPLTRelocation", shouldn't it? This code should separate regular relocations from dynamic ones, and I think that a good way to figure out which are the dynamic relocations is to ask the backend.

[rafaelauler]

Ping and rebase.

[rafaelauler]

Committed r219349.

lib/ReaderWriter/ELF/DefaultLayout.h

public:
   struct FindByName {
     const std::string _name;
     FindByName(StringRef name) : _name(name) {}
     bool operator()(const lld::AtomLayout *j) { return j->_atom->name() == _name; }
   };
 
   typedef typename std::vector<lld::AtomLayout *>::iterator AbsoluteAtomIterT;
 
+  typedef llvm::DenseSet<const Atom *> AtomSetT;
+
   DefaultLayout(const ELFLinkingContext &context) : _context(context) {}
 
   /// \brief Return the section order for a input section
   SectionOrder getSectionOrder(StringRef name, int32_t contentType,
                                int32_t contentPermissions) override;
 
   /// \brief This maps the input sections to the output section names
   virtual StringRef getSectionName(const DefinedAtom *da) const;
public:
 
   uint64_t getTLSSize() const {
     for (const auto &phdr : *_programHeader)
       if (phdr->p_type == llvm::ELF::PT_TLS)
         return phdr->p_memsz;
     return 0;
   }
 
-  bool isReferencedByDefinedAtom(const SharedLibraryAtom *sla) const {
-    return _referencedDynAtoms.count(sla);
+  bool isReferencedByDefinedAtom(const Atom *a) const {
+    return _referencedDynAtoms.count(a);
   }
 
 protected:
   /// \brief Allocate a new section.
   virtual AtomSection<ELFT> *createSection(
       StringRef name, int32_t contentType,
       DefinedAtom::ContentPermissions contentPermissions,
       SectionOrder sectionOrder);
 
 protected:
   llvm::BumpPtrAllocator _allocator;
   SectionMapT _sectionMap;
   MergedSectionMapT _mergedSectionMap;
   AdditionalSegmentMapT _additionalSegmentMap;
   SegmentMapT _segmentMap;
   std::vector<Chunk<ELFT> *> _sections;
   std::vector<Segment<ELFT> *> _segments;
   std::vector<MergedSections<ELFT> *> _mergedSections;
   ELFHeader<ELFT> *_elfHeader;
   ProgramHeader<ELFT> *_programHeader;
   LLD_UNIQUE_BUMP_PTR(RelocationTable<ELFT>) _dynamicRelocationTable;
   LLD_UNIQUE_BUMP_PTR(RelocationTable<ELFT>) _pltRelocationTable;
   std::vector<lld::AtomLayout *> _absoluteAtoms;
-  llvm::DenseSet<const SharedLibraryAtom *> _referencedDynAtoms;
+  AtomSetT _referencedDynAtoms;
   const ELFLinkingContext &_context;
 };
 
 /// \brief Handle linker scripts. TargetLayouts would derive
 /// from this class to override some of the functionalities.
 template<class ELFT>
 class ScriptLayout: public DefaultLayout<ELFT> {
 public:
if (definedAtom->contentType() == DefinedAtom::typeUnknown)
       return make_error_code(llvm::errc::invalid_argument);
     const DefinedAtom::ContentPermissions permissions =
         definedAtom->permissions();
     const DefinedAtom::ContentType contentType = definedAtom->contentType();
 
     StringRef sectionName = getSectionName(definedAtom);
     AtomSection<ELFT> *section =
         getSection(sectionName, contentType, permissions);
+
     // Add runtime relocations to the .rela section.
     for (const auto &reloc : *definedAtom) {
-      if (_context.isDynamicRelocation(*definedAtom, *reloc))
+      bool isLocalReloc = true;
+      if (_context.isDynamicRelocation(*definedAtom, *reloc)) {
         getDynamicRelocationTable()->addRelocation(*definedAtom, *reloc);
-      else if (_context.isPLTRelocation(*definedAtom, *reloc))
+        isLocalReloc = false;
+      } else if (_context.isPLTRelocation(*definedAtom, *reloc)) {
         getPLTRelocationTable()->addRelocation(*definedAtom, *reloc);
-      if (const auto *sla = dyn_cast<SharedLibraryAtom>(reloc->target()))
-        _referencedDynAtoms.insert(sla);
+        isLocalReloc = false;
       }
+
+      if (!reloc->target())
+        continue;
+
+      //Ignore undefined atoms that are not target of dynamic relocations
+      if (isa<UndefinedAtom>(reloc->target()) && isLocalReloc)
+        continue;
+
+      _referencedDynAtoms.insert(reloc->target());
+    }
+
     return section->appendAtom(atom);
   } else if (const AbsoluteAtom *absoluteAtom = dyn_cast<AbsoluteAtom>(atom)) {
     // Absolute atoms are not part of any section, they are global for the whole
     // link
     _absoluteAtoms.push_back(new (_allocator)
         lld::AtomLayout(absoluteAtom, 0, absoluteAtom->value()));
     return *_absoluteAtoms.back();
   } else {

lib/ReaderWriter/ELF/ExecutableWriter.h

if (auto section = dyn_cast<AtomSection<ELFT>>(sec))
             !this->_context.isDynamicallyExportedSymbol(da->name()) &&
             !(this->_context.shouldExportDynamic() &&
               da->scope() == Atom::Scope::scopeGlobal))
           continue;
         this->_dynamicSymbolTable->addSymbol(atom->_atom, section->ordinal(),
                                              atom->_virtualAddr, atom);
       }
 
+  // Put weak symbols in the dynamic symbol table.
+  if (this->_context.isDynamic()) {
+    for (const UndefinedAtom *a : file.undefined()) {
+      if (this->_layout.isReferencedByDefinedAtom(a) &&
+          a->canBeNull() != UndefinedAtom::canBeNullNever)
+        this->_dynamicSymbolTable->addSymbol(a, ELF::SHN_UNDEF);
+    }
+  }
+
   OutputELFWriter<ELFT>::buildDynamicSymbolTable(file);
 }
 
 /// \brief Add absolute symbols by default. These are linker added
 /// absolute symbols
 template<class ELFT>
 void ExecutableWriter<ELFT>::addDefaultAtoms() {
   _runtimeFile->addUndefinedAtom(this->_context.entrySymbolName());

lib/ReaderWriter/ELF/Mips/MipsExecutableWriter.h

 
 template <class ELFT>
 void MipsExecutableWriter<ELFT>::buildDynamicSymbolTable(const File &file) {
   // MIPS ABI requires to add to dynsym even undefined symbols
   // if they have a corresponding entries in a global part of GOT.
   for (auto sec : this->_layout.sections())
     if (auto section = dyn_cast<AtomSection<ELFT>>(sec))
       for (const auto &atom : section->atoms()) {
-        if (_writeHelper.hasGlobalGOTEntry(atom->_atom))
-          this->_dynamicSymbolTable->addSymbol(atom->_atom, section->ordinal(),
-                                               atom->_virtualAddr, atom);
+        if (_writeHelper.hasGlobalGOTEntry(atom->_atom)) {
+          this->_dynamicSymbolTable->addSymbol(atom->_atom, section->ordinal(),
+                                               atom->_virtualAddr, atom);
+          continue;
+        }
+
+        const DefinedAtom *da = dyn_cast<const DefinedAtom>(atom->_atom);
+        if (!da)
+          continue;
+
+        if (da->dynamicExport() != DefinedAtom::dynamicExportAlways &&
+            !this->_context.isDynamicallyExportedSymbol(da->name()) &&
+            !(this->_context.shouldExportDynamic() &&
+              da->scope() == Atom::Scope::scopeGlobal))
+          continue;
+
       }
 
   for (const UndefinedAtom *a : file.undefined())
     // FIXME (simon): Consider to move this check to the
     // MipsELFUndefinedAtom class method. That allows to
     // handle more complex coditions in the future.
     if (_writeHelper.hasGlobalGOTEntry(a))
       this->_dynamicSymbolTable->addSymbol(a, ELF::SHN_UNDEF);
 
-  ExecutableWriter<ELFT>::buildDynamicSymbolTable(file);
+  // Skip our immediate parent class method
+  // ExecutableWriter<ELFT>::buildDynamicSymbolTable because we replaced it
+  // with our own version. Call OutputELFWriter directly.
+  OutputELFWriter<ELFT>::buildDynamicSymbolTable(file);
 }
 
 template <class ELFT>
 bool MipsExecutableWriter<ELFT>::createImplicitFiles(
     std::vector<std::unique_ptr<File>> &result) {
   ExecutableWriter<ELFT>::createImplicitFiles(result);
   result.push_back(std::move(_writeHelper.createRuntimeFile()));
   return true;

lib/ReaderWriter/ELF/X86_64/X86_64RelocationPass.cpp

std::error_code handlePLT32(const Reference &ref) {
     // If it is undefined at link time, push the work to the dynamic linker by
     // creating a PLT entry
     if (isa<SharedLibraryAtom>(ref.target()) ||
         isa<UndefinedAtom>(ref.target()))
       const_cast<Reference &>(ref).setTarget(getPLTEntry(ref.target()));
     return std::error_code();
   }
 
-  const GOTAtom *getSharedGOT(const SharedLibraryAtom *sla) {
-    auto got = _gotMap.find(sla);
+  const GOTAtom *getSharedGOT(const Atom *a) {
+    auto got = _gotMap.find(a);
     if (got == _gotMap.end()) {
       auto g = new (_file._alloc) X86_64GOTAtom(_file, ".got.dyn");
-      g->addReferenceELF_x86_64(R_X86_64_GLOB_DAT, 0, sla, 0);
+      g->addReferenceELF_x86_64(R_X86_64_GLOB_DAT, 0, a, 0);
 #ifndef NDEBUG
       g->_name = "__got_";
-      g->_name += sla->name();
+      g->_name += a->name();
 #endif
-      _gotMap[sla] = g;
+      _gotMap[a] = g;
       _gotVector.push_back(g);
       return g;
     }
     return got->second;
   }
 
   std::error_code handleGOT(const Reference &ref) {
-    if (isa<UndefinedAtom>(ref.target()))
-      const_cast<Reference &>(ref).setTarget(getNullGOT());
-    else if (const DefinedAtom *da = dyn_cast<const DefinedAtom>(ref.target()))
+    if (const DefinedAtom *da = dyn_cast<const DefinedAtom>(ref.target()))
       const_cast<Reference &>(ref).setTarget(getGOT(da));
-    else if (const auto sla = dyn_cast<const SharedLibraryAtom>(ref.target()))
-      const_cast<Reference &>(ref).setTarget(getSharedGOT(sla));
+    // Handle undefined atoms in the same way as shared lib atoms: to be
+    // resolved at run time.
+    else if (isa<SharedLibraryAtom>(ref.target()) ||
+             isa<UndefinedAtom>(ref.target()))
+      const_cast<Reference &>(ref).setTarget(getSharedGOT(ref.target()));
     return std::error_code();
   }
 };
 } // end anon namespace
 
 std::unique_ptr<Pass>
 lld::elf::createX86_64RelocationPass(const X86_64LinkingContext &ctx) {
   switch (ctx.getOutputELFType()) {

test/elf/X86_64/weaksym.test

+# Tests that an executable with a weak undefine will put this symbol in the
+# dynamic symbol table if the executable has a dynamic relocation against this
+# symbol.
+
+#RUN: yaml2obj --format elf %s -o %t.o
+#RUN: lld -flavor gnu  -target x86_64 -e main %t.o -o %t1
+#RUN: llvm-readobj -dt %t1 | FileCheck -check-prefix CHECKSYMS %s
+
+#CHECKSYMS:        Name: x@
+#CHECKSYMS-NEXT:   Value: 0x0
+#CHECKSYMS-NEXT:   Size: 0
+#CHECKSYMS-NEXT:   Binding: Weak (0x2)
+#CHECKSYMS-NEXT:   Type: None (0x0)
+#CHECKSYMS-NEXT:   Other: 0
+#CHECKSYMS-NEXT:   Section: Undefined (0x0)
+
+# The object file above corresponds to the following C program compiled with
+# -fPIC:
+# extern int *x __attribute__((weak));
+#
+# int main() {
+#     if (x)
+#         return 1;
+#     return 0;
+# }
+
+---
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  OSABI:           ELFOSABI_GNU
+  Type:            ET_REL
+  Machine:         EM_X86_64
+Sections:
+  - Name:            .text
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
+    AddressAlign:    0x0000000000000010
+    Content:         554889E5488B0500000000C745FC00000000488138000000000F840C000000C745FC01000000E907000000C745FC000000008B45FC5DC3
+  - Name:            .rela.text
+    Type:            SHT_RELA
+    Link:            .symtab
+    AddressAlign:    0x0000000000000008
+    Info:            .text
+    Relocations:
+      - Offset:          0x0000000000000007
+        Symbol:          x
+        Type:            R_X86_64_GOTPCREL
+        Addend:          -4
+  - Name:            .data
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_WRITE, SHF_ALLOC ]
+    AddressAlign:    0x0000000000000004
+    Content:         ''
+  - Name:            .bss
+    Type:            SHT_NOBITS
+    Flags:           [ SHF_WRITE, SHF_ALLOC ]
+    AddressAlign:    0x0000000000000004
+    Content:         ''
+Symbols:
+  Local:
+    - Name:            .text
+      Type:            STT_SECTION
+      Section:         .text
+    - Name:            .data
+      Type:            STT_SECTION
+      Section:         .data
+    - Name:            .bss
+      Type:            STT_SECTION
+      Section:         .bss
+  Global:
+    - Name:            main
+      Type:            STT_FUNC
+      Section:         .text
+      Size:            0x0000000000000037
+    - Name:            _GLOBAL_OFFSET_TABLE_
+  Weak:
+    - Name:            x