[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 ↗

(On Diff #13192)

Why in the target handler ?

[ruiu]

include/lld/Core/Reference.h

97 ↗

(On Diff #13192)

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/LinkingContext.h

public:
   virtual void notifySymbolTableAdd(const Atom *atom) const {
   }
 
   /// Notify the LinkingContext when the symbol table found a name collision.
   /// The useNew parameter specifies which the symbol table plans to keep,
   /// but that can be changed by the LinkingContext.  This is also an
   /// opportunity for flavor specific processing.
   virtual void notifySymbolTableCoalesce(const Atom *existingAtom,
-                                         const Atom *newAtom,
-                                         bool &useNew) const {
-  }
+                                         const Atom *newAtom, bool &useNew) {}
 
   /// This method adds undefined symbols specified by the -u option to the to
   /// the list of undefined symbols known to the linker. This option essentially
   /// forces an undefined symbol to be created. You may also need to call
   /// addDeadStripRoot() for the symbol if your platform supports dead
   /// stripping, so that the symbol will not be removed from the output.
   void addInitialUndefinedSymbol(StringRef symbolName) {
     _initialUndefinedSymbols.push_back(symbolName);

include/lld/Core/SymbolTable.h

 
 /// \brief The SymbolTable class is responsible for coalescing atoms.
 ///
 /// All atoms coalescable by-name or by-content should be added.
 /// The method replacement() can be used to find the replacement atom
 /// if an atom has been coalesced away.
 class SymbolTable {
 public:
-  explicit SymbolTable(const LinkingContext &);
+  explicit SymbolTable(LinkingContext &);
 
   /// @brief add atom to symbol table
   bool add(const DefinedAtom &);
 
   /// @brief add atom to symbol table
   bool add(const UndefinedAtom &);
 
   /// @brief add atom to symbol table
struct AtomMappingInfo {
     static bool isEqual(const DefinedAtom * const LHS,
                         const DefinedAtom * const RHS);
   };
   typedef llvm::DenseSet<const DefinedAtom*, AtomMappingInfo> AtomContentSet;
 
   bool addByName(const Atom &);
   bool addByContent(const DefinedAtom &);
 
-  const LinkingContext &_context;
+  LinkingContext &_context;
   AtomToAtom _replacedAtoms;
   NameToAtom _nameTable;
   NameToAtom _groupTable;
   AtomContentSet _contentTable;
 };
 
 } // namespace lld
 
 #endif // LLD_CORE_SYMBOL_TABLE_H

include/lld/ReaderWriter/ELFLinkingContext.h

 #include "lld/Core/PassManager.h"
 #include "lld/Core/Pass.h"
 #include "lld/Core/range.h"
 #include "lld/Core/STDExtras.h"
 
 #include "lld/ReaderWriter/Reader.h"
 #include "lld/ReaderWriter/Writer.h"
 
+#include "llvm/ADT/StringSet.h"
 #include "llvm/ADT/Triple.h"
 #include "llvm/Object/ELF.h"
 #include "llvm/Support/ELF.h"
 
 #include <map>
 #include <memory>
 
 namespace lld {
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) 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?
public:
   }
 
   // add search path to list.
   virtual bool addSearchPath(StringRef ref) {
     _inputSearchPaths.push_back(ref);
     return true;
   }
 
+  bool hasCoalescedWeakPair(StringRef name) const {
+    return _weakCoalescedSymbols.count(name) != 0;
+  }
+
 private:
   ELFLinkingContext() LLVM_DELETED_FUNCTION;
 
 protected:
   ELFLinkingContext(llvm::Triple, std::unique_ptr<TargetHandlerBase>);
 
   Writer &writer() const override;
 
protected:
   StringRef _dynamicLinkerPath;
   StringRefVector _initFunctions;
   StringRefVector _finiFunctions;
   StringRef _sysrootPath;
   StringRef _soname;
   StringRefVector _rpathList;
   StringRefVector _rpathLinkList;
   std::map<std::string, uint64_t> _absoluteSymbols;
+  llvm::StringSet<> _weakCoalescedSymbols;
 };
 } // end namespace lld
 
 #endif

lib/Core/SymbolTable.cpp

 #include "llvm/Support/raw_ostream.h"
 
 #include <algorithm>
 #include <cassert>
 #include <cstdlib>
 #include <vector>
 
 namespace lld {
-SymbolTable::SymbolTable(const LinkingContext &context) : _context(context) {}
+SymbolTable::SymbolTable(LinkingContext &context) : _context(context) {}
 
 bool SymbolTable::add(const UndefinedAtom &atom) { return addByName(atom); }
 
 bool SymbolTable::add(const SharedLibraryAtom &atom) { return addByName(atom); }
 
 bool SymbolTable::add(const AbsoluteAtom &atom) { return addByName(atom); }
 
 bool SymbolTable::add(const DefinedAtom &atom) {

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) {
+  // 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 && da->scope() == Atom::scopeGlobal && isSharedWeakAtom(ua))
+    // If strong defined atom coalesces away weak atom declared
+    // in the shared object the strong atom needs to be dynamicaly exported.
+    // Save its name.
+    _weakCoalescedSymbols.insert(ua->name());
+}
+
 } // end namespace lld

lib/ReaderWriter/ELF/OutputELFWriter.h

 
 template <class ELFT>
 void OutputELFWriter<ELFT>::buildDynamicSymbolTable(const File &file) {
   ScopedTask task(getDefaultDomain(), "buildDynamicSymbolTable");
   for (auto sec : this->_layout.sections())
     if (auto section = dyn_cast<AtomSection<ELFT>>(sec))
       for (const auto &atom : section->atoms()) {
         const DefinedAtom *da = dyn_cast<const DefinedAtom>(atom->_atom);
-        if (da && da->dynamicExport() == DefinedAtom::dynamicExportAlways)
+        if (da && (da->dynamicExport() == DefinedAtom::dynamicExportAlways ||
+                   _context.hasCoalescedWeakPair(da->name())))
           _dynamicSymbolTable->addSymbol(atom->_atom, section->ordinal(),
                                          atom->_virtualAddr, atom);
       }
   for (const auto &sla : file.sharedLibrary()) {
     if (isDynSymEntryRequired(sla))
       _dynamicSymbolTable->addSymbol(sla, ELF::SHN_UNDEF);
     if (isNeededTagRequired(sla))
       _soNeeded.insert(sla->loadName());

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
+...