[ELF] Export strong defined symbol if it coalesces away a weak symbol defined in a shared library

[atanasyan]

Summary

Now LLD does not export a strong defined symbol if it coalesces away a weak symbol defined in a shared library. This bug affects all ELF architectures and leads to segfault:

% cat foo.c
extern int __attribute__((weak)) flag;
int foo() { return flag; }

% cat main.c
int flag = 1;
int foo();
int main() { return foo() == 1 ? 0 : -1; }

% clang -c -fPIC foo.c main.c
% lld -flavor gnu -target x86_64 -shared -o libfoo.so ... foo.o
% lld -flavor gnu -target x86_64 -o a.out ... main.o libfoo.so
% ./a.out
Segmentation fault

The problem is caused by the fact that we lose all information about coalesced symbols after the Resolver::resolve() method is finished.

The patch solves the problem by overriding the LinkingContext::notifySymbolTableCoalesce() method and saving names of coalesced symbols. Later in the buildDynamicSymbolTable() routine we use this information to export these symbols.

[shankarke]

Could we just store the name in the ELFLinkingContext, that it can be exported ?

lib/ReaderWriter/ELF/DefaultTargetHandler.h

46

Why in the target handler ?

[ruiu]

include/lld/Core/Reference.h

97

I think I don't like this pattern that uses a reference as a boolean attribute for an atom. I understand it's undeniably convenient in some cases, in particular when storing and restoring an atom with the attribute to/from disk using native/yaml format, but it is basically simulating a C++ member in a complicated manner using its own associative list. There is also a performance concern because in order to access the "field" we have to iterate over the reference list.

This can be a boolean field in the ELF class, can't this?

If the reason not to make it a member is native/yaml reader/writer, we should fix them to make it easy to do what we want to do.

[kledzik]

Just add a StringMap ivar to ELFLinkingContext. Have notifySymbolTableCoalesce() add the atom name to the map when overriding a weak atom. In buildDynamicSymbolTable() check for atom->dynamicExport() OR (atom->scope() == scopeGlobal and its name is in the StringMap).

[atanasyan]

This patch implements more trivial approach to fix the bug. It does not create new references to mark symbols need to be exported. Instead of that the patch saves names of the strong defined symbols which coalesce away weak symbols and refers to this name set when build dynamic export table.

[ruiu]

LGTM

[kledzik]

LGTM

[atanasyan]

Thanks for review.

Closed by commit rL217363.

include/lld/Core/Reference.h

enum {
     // pass.
     kindLayoutAfter = 2,
     // kindLayoutBefore is currently used only by PECOFF port, and will
     // be removed soon. To enforce layout, use kindLayoutAfter instead.
     kindLayoutBefore = 3,
     // kindGroupChild is treated as a bidirected edge too.
     kindGroupChild = 4,
     kindAssociate = 5,
+    // kindDynExport marks an ELF atom which needs to be dynamically exported.
+    kindDynExport = 6,
   };
 
   // A value to be added to the value of a target
   typedef int64_t Addend;
 
   /// If the reference is a fixup in the Atom, then this returns the
   /// byte offset into the Atom's content to do the fix up.
   virtual uint64_t offsetInAtom() const = 0;

include/lld/ReaderWriter/ELFLinkingContext.h

public:
   virtual ~TargetHandlerBase() {}
   virtual void registerRelocationNames(Registry &) = 0;
 
   virtual std::unique_ptr<Reader> getObjReader(bool) = 0;
 
   virtual std::unique_ptr<Reader> getDSOReader(bool) = 0;
 
   virtual std::unique_ptr<Writer> getWriter() = 0;
+
+  virtual void createEdge(DefinedAtom *from, const Atom *to,
+                          uint32_t edgeKind) const = 0;
 };
 
 class ELFLinkingContext : public LinkingContext {
 public:
 
   /// \brief The type of ELF executable that the linker
   /// creates.
   enum class OutputMagic : uint8_t {
public:
   virtual bool isLittleEndian() const;
   virtual uint64_t getPageSize() const { return 0x1000; }
   OutputMagic getOutputMagic() const { return _outputMagic; }
   uint16_t getOutputELFType() const { return _outputELFType; }
   uint16_t getOutputMachine() const;
   bool mergeCommonStrings() const { return _mergeCommonStrings; }
   virtual uint64_t getBaseAddress() const { return _baseAddress; }
 
+  void notifySymbolTableCoalesce(const Atom *existingAtom, const Atom *newAtom,
+                                 bool &useNew) const override;
+
   /// This controls if undefined atoms need to be created for undefines that are
   /// present in a SharedLibrary. If this option is set, undefined atoms are
   /// created for every undefined symbol that are present in the dynamic table
   /// in the shared library
   bool useShlibUndefines() const { return _useShlibUndefines; }
   /// @}
 
   /// \brief Does this relocation belong in the dynamic relocation table?

lib/ReaderWriter/ELF/Atoms.h

Scope scope() const override {
     if (_symbol->st_other == llvm::ELF::STV_HIDDEN)
       return scopeLinkageUnit;
     else if (_symbol->getBinding() != llvm::ELF::STB_LOCAL)
       return scopeGlobal;
     else
       return scopeTranslationUnit;
   }
 
+  DynamicExport dynamicExport() const override {
+    for (const auto &r : *this)
+      if (r->kindNamespace() == lld::Reference::KindNamespace::all &&
+          r->kindArch() == lld::Reference::KindArch::all &&
+          r->kindValue() == Reference::kindDynExport)
+        return dynamicExportAlways;
+    return dynamicExportNormal;
+  }
+
   // FIXME: Need to revisit this in future.
   Interposable interposable() const override { return interposeNo; }
 
   // FIXME: What ways can we determine this in ELF?
   Merge merge() const override {
     if (_symbol->getBinding() == llvm::ELF::STB_WEAK)
       return mergeAsWeak;
 
public:
 
   DefinedAtom::reference_iterator begin() const override {
     uintptr_t index = _referenceStartIndex;
     const void *it = reinterpret_cast<const void*>(index);
     return reference_iterator(*this, it);
   }
 
   DefinedAtom::reference_iterator end() const override {
-    uintptr_t index = _referenceEndIndex;
+    uintptr_t index = _referenceEndIndex + _markerReferenceList.size();
     const void *it = reinterpret_cast<const void*>(index);
     return reference_iterator(*this, it);
   }
 
   const Reference *derefIterator(const void *It) const override {
     uintptr_t index = reinterpret_cast<uintptr_t>(It);
     assert(index >= _referenceStartIndex);
-    assert(index < _referenceEndIndex);
+    if (index < _referenceEndIndex)
       return ((_referenceList)[index]);
+    index -= _referenceEndIndex;
+    assert(index < _markerReferenceList.size());
+    return _markerReferenceList[index];
   }
 
   void incrementIterator(const void *&It) const override {
     uintptr_t index = reinterpret_cast<uintptr_t>(It);
     ++index;
     It = reinterpret_cast<const void *>(index);
   }
 
   void addReference(ELFReference<ELFT> *reference) {
     _referenceList.push_back(reference);
     _referenceEndIndex = _referenceList.size();
   }
 
+  void addMarkerReference(ELFReference<ELFT> *reference) {
+    _markerReferenceList.push_back(reference);
+  }
+
   virtual void setOrdinal(uint64_t ord) { _ordinal = ord; }
 
 protected:
   const ELFFile<ELFT> &_owningFile;
   StringRef _symbolName;
   StringRef _sectionName;
   const Elf_Sym *_symbol;
   const Elf_Shdr *_section;
   /// \brief Holds the bits that make up the atom.
   ArrayRef<uint8_t> _contentData;
 
   uint64_t _ordinal;
   unsigned int _referenceStartIndex;
   unsigned int _referenceEndIndex;
   std::vector<ELFReference<ELFT> *> &_referenceList;
+  std::vector<ELFReference<ELFT> *> _markerReferenceList;
   mutable ContentType _contentType;
   mutable ContentPermissions _permissions;
 };
 
 /// \brief This atom stores mergeable Strings
 template <class ELFT> class ELFMergeAtom : public DefinedAtom {
   typedef llvm::object::Elf_Shdr_Impl<ELFT> Elf_Shdr;
 

lib/ReaderWriter/ELF/DefaultTargetHandler.h

virtual std::unique_ptr<Reader> getObjReader(bool atomizeStrings) {
     return std::unique_ptr<Reader>(new ELFObjectReader(atomizeStrings));
   }
 
   virtual std::unique_ptr<Reader> getDSOReader(bool useShlibUndefines) {
     return std::unique_ptr<Reader>(new ELFDSOReader(useShlibUndefines));
   }
 
   virtual std::unique_ptr<Writer> getWriter() = 0;
+
+  void createEdge(DefinedAtom *from, const Atom *to,
+                  uint32_t edgeKind) const override;
 };
 
+template <class ELFT>
+void DefaultTargetHandler<ELFT>::createEdge(DefinedAtom *from, const Atom *to,
+                                            uint32_t edgeKind) const {
+  auto *atom = static_cast<ELFDefinedAtom<ELFT> *>(from);
+  auto &file = const_cast<ELFFile<ELFT> &>(atom->file());
+  file.createEdge(atom, to, edgeKind);
+}
+
 } // end namespace elf
 } // end namespace lld
 #endif

lib/ReaderWriter/ELF/ELFFile.h

public:
   /// set
   virtual std::error_code createMergeableAtoms();
 
   /// \brief Add the symbols that the sections contain. The symbols will be
   /// converted to atoms for
   /// Undefined symbols, absolute symbols
   virtual std::error_code createSymbolsFromAtomizableSections();
 
+  /// \brief Create reference between two atoms.
+  virtual void createEdge(ELFDefinedAtom<ELFT> *from, const Atom *to,
+                          uint32_t edgeKind);
+
   /// \brief Create individual atoms
   virtual std::error_code createAtoms();
 
   const atom_collection<DefinedAtom> &defined() const override {
     return _definedAtoms;
   }
 
   const atom_collection<UndefinedAtom> &undefined() const override {
protected:
   virtual int64_t defaultRelocAddend(const Reference &) const;
 
   /// Returns the symbol's content size. The nextSymbol should be null if the
   /// symbol is the last one in the section.
   virtual uint64_t symbolContentSize(const Elf_Shdr *section,
                                      const Elf_Sym *symbol,
                                      const Elf_Sym *nextSymbol);
 
-  virtual void createEdge(ELFDefinedAtom<ELFT> *from, ELFDefinedAtom<ELFT> *to,
-                          uint32_t edgeKind);
-
   /// Determines if the reader needs to create atoms for the section.
   virtual bool ignoreCreateAtomsForSection(const Elf_Shdr *shdr) {
     return false;
   }
 
   /// Get the section name for a section.
   virtual ErrorOr<StringRef> getSectionName(const Elf_Shdr *shdr) const {
     if (!shdr)
uint64_t ELFFile<ELFT>::symbolContentSize(const Elf_Shdr *section,
                                           const Elf_Sym *symbol,
                                           const Elf_Sym *nextSymbol) {
   // if this is the last symbol, take up the remaining data.
   return nextSymbol ? nextSymbol->st_value - symbol->st_value
                     : section->sh_size - symbol->st_value;
 }
 
 template <class ELFT>
-void ELFFile<ELFT>::createEdge(ELFDefinedAtom<ELFT> *from,
-                               ELFDefinedAtom<ELFT> *to, uint32_t edgeKind) {
+void ELFFile<ELFT>::createEdge(ELFDefinedAtom<ELFT> *from, const Atom *to,
+                               uint32_t edgeKind) {
   auto reference = new (_readerStorage) ELFReference<ELFT>(edgeKind);
   reference->setTarget(to);
-  from->addReference(reference);
+  from->addMarkerReference(reference);
 }
 
 } // end namespace elf
 } // end namespace lld
 
 #endif // LLD_READER_WRITER_ELF_FILE_H

lib/ReaderWriter/ELF/ELFLinkingContext.cpp

std::unique_ptr<File> ELFLinkingContext::createUndefinedSymbolFile() const {
   std::unique_ptr<SimpleFile> undefinedSymFile(
       new SimpleFile("command line option -u"));
   for (auto undefSymStr : _initialUndefinedSymbols)
     undefinedSymFile->addAtom(*(new (_allocator) CommandLineUndefinedAtom(
         *undefinedSymFile, undefSymStr)));
   return std::move(undefinedSymFile);
 }
 
+static bool isSharedWeakAtom(const UndefinedAtom *ua) {
+  return ua->canBeNull() != UndefinedAtom::canBeNullNever &&
+         isa<SharedLibraryFile>(ua->file());
+}
+
+void ELFLinkingContext::notifySymbolTableCoalesce(const Atom *existingAtom,
+                                                  const Atom *newAtom,
+                                                  bool &useNew) const {
+  // First suppose that the `existingAtom` is defined
+  // and the `newAtom` is undefined.
+  auto *da = dyn_cast<DefinedAtom>(existingAtom);
+  auto *ua = dyn_cast<UndefinedAtom>(newAtom);
+  if (!da && !ua) {
+    // Then try to reverse the assumption.
+    da = dyn_cast<DefinedAtom>(newAtom);
+    ua = dyn_cast<UndefinedAtom>(existingAtom);
+  }
+
+  if (da && ua && isSharedWeakAtom(ua))
+    // If strong defined atom coalesces away weak atom declared
+    // in the shared object the strong atom needs to be dynamicaly exported.
+    targetHandler()->createEdge(const_cast<DefinedAtom *>(da), ua,
+                                lld::Reference::kindDynExport);
+}
+
 } // end namespace lld

lib/ReaderWriter/Reader.cpp

 }
 
 static const Registry::KindStrings kindStrings[] = {
     {Reference::kindInGroup, "in-group"},
     {Reference::kindLayoutAfter, "layout-after"},
     {Reference::kindLayoutBefore, "layout-before"},
     {Reference::kindGroupChild, "group-child"},
     {Reference::kindAssociate, "associate"},
+    {Reference::kindDynExport, "dyn-export"},
     LLD_KIND_STRING_END};
 
 Registry::Registry() {
   addKindTable(Reference::KindNamespace::all, Reference::KindArch::all,
                kindStrings);
 }
 
 bool Registry::handleTaggedDoc(llvm::yaml::IO &io,

test/elf/X86_64/dynsym-weak.test

+# Check that a symbol declared as a week in a shared library gets a dynamic
+# symbol table record in an executable file if this executabe file declares the
+# symbol as strong.
+
+# RUN: yaml2obj -format=elf -docnum 1 %s > %t.foo.o
+# RUN: lld -flavor gnu -target x86_64 -shared -o %t.so %t.foo.o
+# RUN: yaml2obj -format=elf -docnum 2 %s > %t.main.o
+#
+# Link executable file with strong symbol. Weak symbol is in the shared lib.
+# RUN: lld -flavor gnu -target x86_64 -e main -o %t1.exe %t.main.o %t.so
+# RUN: llvm-readobj -dyn-symbols %t1.exe | FileCheck -check-prefix=EXE %s
+#
+# Link executable file. Strong and weak symbol come from different object files.
+# RUN: lld -flavor gnu -target x86_64 -e main -o %t2.exe %t.main.o %t.foo.o
+# RUN: llvm-readobj -dyn-symbols %t2.exe | FileCheck -check-prefix=OBJ %s
+#
+# Link shared library. Weak symbol is in the another shared lib.
+# RUN: lld -flavor gnu -target x86_64 -shared -o %t.res.so %t.main.o %t.so
+# RUN: llvm-readobj -dyn-symbols %t.res.so | FileCheck -check-prefix=SO %s
+
+# EXE:      Symbol {
+# EXE:        Name: flag@ ({{[0-9]+}})
+# EXE-NEXT:   Value: 0x{{[0-9A-F]+}}
+# EXE-NEXT:   Size: 4
+# EXE-NEXT:   Binding: Global (0x1)
+# EXE-NEXT:   Type: Object (0x1)
+# EXE-NEXT:   Other: 0
+# EXE-NEXT:   Section: .data (0x{{[0-9A-F]+}})
+# EXE-NEXT: }
+
+# OBJ-NOT:  Name: flag@ ({{[0-9]+}})
+
+# SO:      Symbol {
+# SO:        Name: flag@ ({{[0-9]+}})
+# SO-NEXT:   Value: 0x{{[0-9A-F]+}}
+# SO-NEXT:   Size: 4
+# SO-NEXT:   Binding: Global (0x1)
+# SO-NEXT:   Type: Object (0x1)
+# SO-NEXT:   Other: 0
+# SO-NEXT:   Section: .data (0x{{[0-9A-F]+}})
+# SO-NEXT: }
+
+# foo.o
+---
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_REL
+  Machine:         EM_X86_64
+
+Sections:
+  - Name:            .text
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
+    AddressAlign:    0x04
+    Size:            0x08
+  - Name:            .rela.text
+    Type:            SHT_RELA
+    Link:            .symtab
+    AddressAlign:    0x08
+    Info:            .text
+    Relocations:
+      - Offset:          0x00
+        Symbol:          flag
+        Type:            R_X86_64_GOTPCREL
+        Addend:          -4
+
+Symbols:
+  Global:
+    - Name:            foo
+      Type:            STT_FUNC
+      Section:         .text
+      Size:            0x08
+  Weak:
+    - Name:            flag
+
+# main.o
+---
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_REL
+  Machine:         EM_X86_64
+
+Sections:
+  - Name:            .text
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
+    AddressAlign:    0x04
+    Size:            0x08
+  - Name:            .rela.text
+    Type:            SHT_RELA
+    Link:            .symtab
+    AddressAlign:    0x08
+    Info:            .text
+    Relocations:
+      - Offset:          0x00
+        Symbol:          foo
+        Type:            R_X86_64_PLT32
+        Addend:          -4
+  - Name:            .data
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_WRITE, SHF_ALLOC ]
+    AddressAlign:    0x04
+    Size:            0x04
+
+Symbols:
+  Global:
+    - Name:            flag
+      Type:            STT_OBJECT
+      Section:         .data
+      Size:            0x04
+    - Name:            main
+      Type:            STT_FUNC
+      Section:         .text
+      Size:            0x08
+    - Name:            foo
+...