[lld] [mach-o]: Initial support for reading dylibs during link.

[t.p.northover]

Summary

Hi,

The attached patch is my initial attempts to get lld to read .dylib files correctly during link. I mostly followed ELF, which seems to have the following effects:

+ No undefinedAtoms, definedAtoms etc
+ SharedLibraryAtoms are produced on demand (so not all exported symbols end up in the final image).
+ the sharedLibraryAtoms function returns an empty set (I'm not sure why this is. If anyone does know the reasoning, I'd be glad to hear it).

On the MachO specific side, experiments with ld64 suggest that all N_EXT (even N_PEXT) symbols that aren't N_UNDF are should be eligible to be resolved in the dylib.

Does it look reasonable as a first pass? Or should I change some things?

Cheers.

Tim.

[kledzik]

lib/ReaderWriter/MachO/Atoms.h

128

There is nothing to fix now. But long term, what we need to have happen is when the Resolver/SymbolTable replaces an UndefinedAtom with a SharedLibraryAtom, if the UndefinedAtom was marked canBeNullAtRuntime, then the SharedLibraryAtom needs to be marked that way too.

151

_dylib should be renamed _dylibInstallName. It is not the path where the dylib was loaded from (that might be in an SDK). It is the string from the dylib's LC_ID_DYLIB load command.

lib/ReaderWriter/MachO/File.h

100

Add FIXME note that we need to track whether a symbol is data or code to implement dataSymbolOnly correctly.

102–106

In the case where exports() has been called again with the same name, we should return the previously allocated atom (or assert). This code allocates another copy of the atom.

lib/ReaderWriter/MachO/MachONormalizedFileToAtoms.cpp

439–441

Probably should check visibility of symbol. In dylibs, only global symbols should be in globalSymbols. On the other hand, in .o files, hidden (scopeLinkageUnit) symbols are also in globalSymbols.

test/mach-o/lit.local.cfg

4 ↗

(On Diff #10885)

Why the separate file for the dylib? You can put multiple yaml "documents" in one .yaml file. Just separate them with "---". That is one of the reasons for using yaml for test cases in lld.

You can also prune down that dylib yaml. It does not need content - just symbols.

[t.p.northover]

Hi Nick,

Most of the comments seem fine, but...

+ for (auto &sym : normalizedFile.globalSymbols) {
+ file->addSharedLibraryAtom(sym.name, copyRefs);
+ }

+

Probably should check visibility of symbol. In dylibs, only global symbols should be in globalSymbols. On the other hand, in .o files, hidden (scopeLinkageUnit) symbols are also in globalSymbols.

Are you sure about this one? As far as I can tell, scopeLinkageUnit
corresponds to N_PEXT. I specifically ran some tests on .dylibs with
N_PEXT symbols and ld64 was quite happy to resolve them to that
.dylib. Did I cock up my tests, or are we talking about different
things?

Cheers.

Tim.

[t.p.northover]

Hi Nick,

Hopefully this addresses your comments. Does it look more reasonable now?

Cheers.

Tim.

[kledzik]

LGTM.

I follow on patch would be to add support for install-name: key in dylib yaml files and plumb that to _dylibInstallName.

[t.p.northover]

Thanks Nick. Committed as r212027.

Tim.

lib/ReaderWriter/MachO/Atoms.h

 
 private:
   const StringRef _name;
   const Scope _scope;
   const uint64_t _size;
   const DefinedAtom::Alignment _align;
 };
 
+class MachOSharedLibraryAtom : public SharedLibraryAtom {
+public:
+  MachOSharedLibraryAtom(const File &file, StringRef name,
+                         StringRef dylibInstallName)
+      : SharedLibraryAtom(), _file(file), _name(name),
+        _dylibInstallName(dylibInstallName) {}
+  virtual ~MachOSharedLibraryAtom() {}
+
+  virtual StringRef loadName() const override {
+    return _dylibInstallName;
+  }
+
+  virtual bool canBeNullAtRuntime() const override {
+    // FIXME: this may actually be changeable. For now, all symbols are strongly
+    // defined though.
+    return false;
+  }
+
+  virtual const File& file() const override {
+    return _file;
+  }
+
+  virtual StringRef name() const override {
+    return _name;
+  }
+
+  virtual Type type() const override {
+    // Unused in MachO (I think).
+    return Type::Unknown;
+  }
+
+  virtual uint64_t size() const override {
+    // Unused in MachO (I think)
+    return 0;
+  }
+
+private:
+  const File &_file;
+  StringRef _name;
+  StringRef _dylibInstallName;
+};
+
 
 } // mach_o
 } // lld
 
 #endif

lib/ReaderWriter/MachO/File.h

 //===- lib/ReaderWriter/MachO/File.h --------------------------------------===//
 //
 //                             The LLVM Linker
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLD_READER_WRITER_MACHO_FILE_H
 #define LLD_READER_WRITER_MACHO_FILE_H
 
 #include "Atoms.h"
 
 #include "lld/Core/Simple.h"
+#include "lld/Core/SharedLibraryFile.h"
+
+#include <unordered_map>
 
 namespace lld {
 namespace mach_o {
 
 class MachOFile : public SimpleFile {
 public:
   MachOFile(StringRef path) : SimpleFile(path) {}
 
void addTentativeDefAtom(StringRef name, Atom::Scope scope, uint64_t size,
     addAtom(*atom);
   }
 
 
 private:
   llvm::BumpPtrAllocator _allocator;
 };
 
+class MachODylibFile : public SharedLibraryFile {
+public:
+  MachODylibFile(StringRef path) : SharedLibraryFile(path), _dylib_name(path) {}
+
+  virtual const SharedLibraryAtom *exports(StringRef name,
+                                           bool dataSymbolOnly) const {
+    // FIXME: we obviously need to record code/data if we're going to make
+    // proper use of dataSymbolOnly.
+    auto sym = _nameToAtom.find(name);
+
+    if (sym == _nameToAtom.end())
+      return nullptr;
+
+    if (!sym->second)
+      sym->second =
+          new (_allocator) MachOSharedLibraryAtom(*this, name, _dylib_name);
+
+    return sym->second;
+  }
+
+  const atom_collection<DefinedAtom> &defined() const override {
+    return _definedAtoms;
+  }
+
+  const atom_collection<UndefinedAtom> &undefined() const override {
+    return _undefinedAtoms;
+  }
+
+  const atom_collection<SharedLibraryAtom> &sharedLibrary() const override {
+    return _sharedLibraryAtoms;
+  }
+
+  const atom_collection<AbsoluteAtom> &absolute() const override {
+    return _absoluteAtoms;
+  }
+
+  void addSharedLibraryAtom(StringRef name, bool copyRefs) {
+    if (copyRefs) {
+      name = name.copy(_allocator);
+    }
+
+    _nameToAtom[name] = nullptr;
+  }
+
+private:
+  StringRef _dylib_name;
+  atom_collection_vector<DefinedAtom>        _definedAtoms;
+  atom_collection_vector<UndefinedAtom>      _undefinedAtoms;
+  atom_collection_vector<SharedLibraryAtom>  _sharedLibraryAtoms;
+  atom_collection_vector<AbsoluteAtom>       _absoluteAtoms;
+
+  mutable std::unordered_map<StringRef, SharedLibraryAtom *> _nameToAtom;
+  mutable llvm::BumpPtrAllocator _allocator;
+};
+
 } // end namespace mach_o
 } // end namespace lld
 
 #endif

lib/ReaderWriter/MachO/MachONormalizedFileBinaryReader.cpp

 
 
 class MachOReader : public Reader {
 public:
   MachOReader(MachOLinkingContext::Arch arch) : _arch(arch) {}
 
   bool canParse(file_magic magic, StringRef ext,
                 const MemoryBuffer &mb) const override {
-    if (magic != llvm::sys::fs::file_magic::macho_object)
+    if (magic != llvm::sys::fs::file_magic::macho_object &&
+        magic != llvm::sys::fs::file_magic::macho_dynamically_linked_shared_lib)
       return false;
     if (mb.getBufferSize() < 32)
       return false;
     const char *start = mb.getBufferStart();
     const mach_header *mh = reinterpret_cast<const mach_header *>(start);
     const bool swap = (mh->magic == llvm::MachO::MH_CIGAM) ||
                       (mh->magic == llvm::MachO::MH_CIGAM_64);
     const uint32_t filesCpuType = read32(swap, mh->cputype);

lib/ReaderWriter/MachO/MachONormalizedFileToAtoms.cpp

void sectionParseInfo(DefinedAtom::ContentType atomType,
   atomizeModel = atomizeAtSymbols;
 }
 
 
 Atom::Scope atomScope(uint8_t scope) {
   switch (scope) {
   case N_EXT:
     return Atom::scopeGlobal;
+  case N_PEXT:
   case N_PEXT | N_EXT:
     return Atom::scopeLinkageUnit;
   case 0:
     return Atom::scopeTranslationUnit;
   }
   llvm_unreachable("unknown scope value!");
 }
 
if (sym.value == Hex64(0)) {
       file->addTentativeDefAtom(sym.name, atomScope(sym.scope), sym.value,
                                DefinedAtom::Alignment(sym.desc >> 8), copyRefs);
     }
   }
 
   return std::unique_ptr<File>(std::move(file));
 }
 
+ErrorOr<std::unique_ptr<lld::File>>
+normalizedDylibToAtoms(const NormalizedFile &normalizedFile, StringRef path,
+                       bool copyRefs) {
+  std::unique_ptr<MachODylibFile> file(new MachODylibFile(path));
+
+  for (auto &sym : normalizedFile.globalSymbols) {
+    assert((sym.scope & N_EXT) && "only expect external symbols here");
+    file->addSharedLibraryAtom(sym.name, copyRefs);
+  }
+
+  return std::unique_ptr<File>(std::move(file));
+}
+
 } // anonymous namespace
 
 namespace normalized {
 
 void relocatableSectionInfoForContentType(DefinedAtom::ContentType atomType,
                                           StringRef &segmentName,
                                           StringRef &sectionName,
                                           SectionType &sectionType,
void relocatableSectionInfoForContentType(DefinedAtom::ContentType atomType,
   }
   llvm_unreachable("content type not yet supported");
 }
 
 ErrorOr<std::unique_ptr<lld::File>>
 normalizedToAtoms(const NormalizedFile &normalizedFile, StringRef path,
                   bool copyRefs) {
   switch (normalizedFile.fileType) {
+  case MH_DYLIB:
+    return normalizedDylibToAtoms(normalizedFile, path, copyRefs);
   case MH_OBJECT:
     return normalizedObjectToAtoms(normalizedFile, path, copyRefs);
   default:
     llvm_unreachable("unhandled MachO file type!");
   }
 }
 
 } // namespace normalized
 } // namespace mach_o
 } // namespace lld

lib/ReaderWriter/MachO/MachONormalizedFileYAML.cpp

bool MachOYamlIOTaggedDocumentHandler::handledDocTag(llvm::yaml::IO &io,
   YamlContext *info = reinterpret_cast<YamlContext *>(io.getContext());
   assert(info != nullptr);
   assert(info->_normalizeMachOFile == nullptr);
   info->_normalizeMachOFile = &nf;
   MappingTraits<NormalizedFile>::mapping(io, nf);
   // Step 2: parse normalized mach-o struct into atoms.
   ErrorOr<std::unique_ptr<lld::File>> foe = normalizedToAtoms(nf, info->_path,
                                                               true);
+
+  info->_normalizeMachOFile = nullptr;
   if (foe) {
     // Transfer ownership to "out" File parameter.
     std::unique_ptr<lld::File> f = std::move(foe.get());
     file = f.release();
     return true;
   } else {
     io.setError(foe.getError().message());
     return false;

test/mach-o/use-simple-dylib.yaml

+# RUN: lld -flavor darwin -arch x86_64 -output_filetype atomic-yaml -r %s -o - | FileCheck %s
+# lld -flavor darwin -arch x86_64 -output_filetype atomic-yaml -r %s %p/Inputs/simple-dylib.yaml -o - | FileCheck %s
+
+--- !mach-o
+arch:            x86_64
+file-type:       MH_OBJECT
+flags:           [  ]
+has-UUID:        false
+OS:              unknown
+sections:
+  - segment:         __TEXT
+    section:         __text
+    type:            S_REGULAR
+    attributes:      [ S_ATTR_PURE_INSTRUCTIONS ]
+    address:         0x0000000000000000
+    content:         [ 0x55, 0x48, 0x89, 0xE5, 0xE8, 0x00, 0x00, 0x00,
+                       0x00, 0xE8, 0x00, 0x00, 0x00, 0x00, 0xE8, 0x00,
+                       0x00, 0x00, 0x00, 0xE8, 0x00, 0x00, 0x00, 0x00,
+                       0xE8, 0x00, 0x00, 0x00, 0x00, 0x5D, 0xE9, 0x00,
+                       0x00, 0x00, 0x00 ]
+global-symbols:
+  - name:            _foo
+    type:            N_SECT
+    scope:           [ N_EXT ]
+    sect:            1
+    value:           0x0000000000000000
+undefined-symbols:
+  - name:            _myGlobal
+    type:            N_UNDF
+    scope:           [ N_EXT ]
+    value:           0x0000000000000000
+  - name:            _myGlobalWeak
+    type:            N_UNDF
+    scope:           [ N_EXT ]
+    value:           0x0000000000000000
+  - name:            _myHidden
+    type:            N_UNDF
+    scope:           [ N_EXT ]
+    value:           0x0000000000000000
+  - name:            _myHiddenWeak
+    type:            N_UNDF
+    scope:           [ N_EXT ]
+    value:           0x0000000000000000
+  - name:            _myResolver
+    type:            N_UNDF
+    scope:           [ N_EXT ]
+    value:           0x0000000000000000
+  - name:            _myStatic
+    type:            N_UNDF
+    scope:           [ N_EXT ]
+    value:           0x0000000000000000
+  - name:            _myVariablePreviouslyKnownAsPrivateExtern
+    type:            N_UNDF
+    scope:           [ N_EXT ]
+    value:           0x0000000000000000
+
+--- !mach-o
+arch:            x86_64
+file-type:       MH_DYLIB
+flags:           [ MH_SUBSECTIONS_VIA_SYMBOLS ]
+has-UUID:        false
+OS:              unknown
+sections:
+  - segment:         __TEXT
+    section:         __text
+    type:            S_REGULAR
+    attributes:      [ S_ATTR_PURE_INSTRUCTIONS, S_ATTR_SOME_INSTRUCTIONS ]
+    alignment:       4
+    address:         0x0000000000000000
+    content:         [ 0xCC, 0xC3, 0x90, 0xC3, 0x90, 0x90, 0xC3, 0x90,
+                       0x90, 0x90, 0xC3, 0x90, 0x90, 0x90, 0x90, 0xC3,
+                       0x31, 0xC0, 0xC3 ]
+local-symbols:
+  - name:            _myStatic
+    type:            N_SECT
+    sect:            1
+    value:           0x000000000000000B
+  - name:            _myVariablePreviouslyKnownAsPrivateExtern
+    type:            N_SECT
+    scope:           [ N_PEXT ]
+    sect:            1
+    desc:            [ N_SYMBOL_RESOLVER ]
+    value:           0x0000000000000011
+global-symbols:
+  - name:            _myGlobal
+    type:            N_SECT
+    scope:           [ N_EXT ]
+    sect:            1
+    value:           0x0000000000000001
+  - name:            _myGlobalWeak
+    type:            N_SECT
+    scope:           [ N_EXT ]
+    sect:            1
+    desc:            [ N_WEAK_DEF ]
+    value:           0x0000000000000002
+  - name:            _myHidden
+    type:            N_SECT
+    scope:           [ N_EXT, N_PEXT ]
+    sect:            1
+    value:           0x0000000000000004
+  - name:            _myHiddenWeak
+    type:            N_SECT
+    scope:           [ N_EXT, N_PEXT ]
+    sect:            1
+    desc:            [ N_WEAK_DEF ]
+    value:           0x0000000000000007
+  - name:            _myResolver
+    type:            N_SECT
+    scope:           [ N_EXT ]
+    sect:            1
+    desc:            [ N_SYMBOL_RESOLVER ]
+    value:           0x0000000000000010
+...
+
+
+# CHECK: undefined-atoms:
+# CHECK:   - name:            _myStatic
+# CHECK:   - name:            _myVariablePreviouslyKnownAsPrivateExtern
+# CHECK: shared-library-atoms:
+# CHECK:   - name:            _myHidden
+# CHECK:     load-name:       {{.*}}use-simple-dylib.yaml
+# CHECK:   - name:            _myGlobal
+# CHECK:     load-name:       {{.*}}use-simple-dylib.yaml
+# CHECK:   - name:            _myHiddenWeak
+# CHECK:     load-name:       {{.*}}use-simple-dylib.yaml
+# CHECK:   - name:            _myGlobalWeak
+# CHECK:     load-name:       {{.*}}use-simple-dylib.yaml
+# CHECK:   - name:            _myResolver
+# CHECK:     load-name:       {{.*}}use-simple-dylib.yaml