Use owning pointers instead of raw pointers for Atom's to fix leaks.

This is a re-commit of r264022 with a fix for MSVC.  The issue there was
that the code was running DefinedAtom::~Atom() for some value and instead
needed to cast to Atom before running ~Atom.  Original commit message follows.

Currently each File contains an BumpPtrAllocator in which Atom's are
allocated.  Some Atom's contain data structures like std::vector which
leak as we don't run ~Atom when they are BumpPtrAllocate'd.

Now each File actually owns its Atom's using an OwningAtomPtr.  This
is analygous to std::unique_ptr and may be replaced by it if possible.

An Atom can therefore only be owned by a single File, so the Resolver now
moves them from one File to another.  The MachOLinkingContext owns the File's
and so clears all the Atom's in ~MachOLinkingContext, then delete's all the
File's.  This makes sure all Atom's have been destructed before any of the
BumpPtrAllocator's in which they run have gone away.

Should hopefully fix the remaining leaks.  Will keep an eye on the bots to
make sure.

llvm-svn: 264067

lld/include/lld/Core/Atom.h

@@ -16,6 +16,9 @@ namespace lld {
 
 class File;
 
+template<typename T>
+class OwningAtomPtr;
+
 ///
 /// The linker has a Graph Theory model of linking. An object file is seen
 /// as a set of Atoms with References to other Atoms.  Each Atom is a node
@@ -24,6 +27,7 @@ class File;
 /// undefined symbol (extern declaration).
 ///
 class Atom {
+  template<typename T> friend class OwningAtomPtr;
 public:
   /// Whether this atom is defined or a proxy for an undefined symbol
   enum Definition {
@@ -71,6 +75,53 @@ class Atom {
   Definition _definition;
 };
 
+/// Class which owns an atom pointer and runs the atom destructor when the
+/// owning pointer goes out of scope.
+template<typename T>
+class OwningAtomPtr {
+private:
+  OwningAtomPtr(const OwningAtomPtr &) = delete;
+  void operator=(const OwningAtomPtr&) = delete;
+public:
+  OwningAtomPtr() : atom(nullptr) { }
+  OwningAtomPtr(T *atom) : atom(atom) { }
+
+  ~OwningAtomPtr() {
+    if (atom)
+      runDestructor(atom);
+  }
+
+  void runDestructor(Atom *atom) {
+    atom->~Atom();
+  }
+
+  OwningAtomPtr(OwningAtomPtr &&ptr) : atom(ptr.atom) {
+    ptr.atom = nullptr;
+  }
+
+  void operator=(OwningAtomPtr&& ptr) {
+    atom = ptr.atom;
+    ptr.atom = nullptr;
+  }
+
+  T *const &get() const {
+    return atom;
+  }
+
+  T *&get() {
+    return atom;
+  }
+
+  T *release() {
+    auto *v = atom;
+    atom = nullptr;
+    return v;
+  }
+
+private:
+  T *atom;
+};
+
 } // namespace lld
 
 #endif // LLD_CORE_ATOM_H

lld/include/lld/Core/DefinedAtom.h

@@ -363,6 +363,8 @@ class DefinedAtom : public Atom {
   // constructor.
   DefinedAtom() : Atom(definitionRegular) { }
 
+  ~DefinedAtom() override = default;
+
   /// \brief Returns a pointer to the Reference object that the abstract
   /// iterator "points" to.
   virtual const Reference *derefIterator(const void *iter) const = 0;

lld/include/lld/Core/File.h

@@ -15,6 +15,7 @@
 #include "lld/Core/SharedLibraryAtom.h"
 #include "lld/Core/UndefinedAtom.h"
 #include "llvm/ADT/Optional.h"
+#include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/Twine.h"
 #include "llvm/Support/ErrorHandling.h"
 #include <functional>
@@ -39,6 +40,10 @@ class LinkingContext;
 /// The Atom objects in a File are owned by the File object.  The Atom objects
 /// are destroyed when the File object is destroyed.
 class File {
+protected:
+  /// The type of atom mutable container.
+  template <typename T> using AtomVector = std::vector<OwningAtomPtr<T>>;
+
 public:
   virtual ~File();
 
@@ -104,38 +109,93 @@ class File {
     return _allocator;
   }
 
-  /// The type of atom mutable container.
-  template <typename T> using AtomVector = std::vector<const T *>;
-
-  /// The range type for the atoms. It's backed by a std::vector, but hides
-  /// its member functions so that you can only call begin or end.
+  /// The range type for the atoms.
   template <typename T> class AtomRange {
   public:
-    AtomRange(AtomVector<T> v) : _v(v) {}
-    typename AtomVector<T>::const_iterator begin() const { return _v.begin(); }
-    typename AtomVector<T>::const_iterator end() const { return _v.end(); }
-    typename AtomVector<T>::iterator begin() { return _v.begin(); }
-    typename AtomVector<T>::iterator end() { return _v.end(); }
+    AtomRange(AtomVector<T> &v) : _v(v) {}
+    AtomRange(const AtomVector<T> &v) : _v(const_cast<AtomVector<T> &>(v)) {}
+
+    typedef std::pointer_to_unary_function<const OwningAtomPtr<T>&,
+                                           const T*> ConstDerefFn;
+
+    typedef std::pointer_to_unary_function<OwningAtomPtr<T>&, T*> DerefFn;
+
+    typedef llvm::mapped_iterator<typename AtomVector<T>::const_iterator,
+                                  ConstDerefFn> ConstItTy;
+    typedef llvm::mapped_iterator<typename AtomVector<T>::iterator,
+                                  DerefFn> ItTy;
+
+    static const T* DerefConst(const OwningAtomPtr<T> &p) {
+      return p.get();
+    }
+
+    static T* Deref(OwningAtomPtr<T> &p) {
+      return p.get();
+    }
+
+    ConstItTy begin() const {
+      return ConstItTy(_v.begin(), ConstDerefFn(DerefConst));
+    }
+    ConstItTy end() const {
+      return ConstItTy(_v.end(), ConstDerefFn(DerefConst));
+    }
+
+    ItTy begin() {
+      return ItTy(_v.begin(), DerefFn(Deref));
+    }
+    ItTy end() {
+      return ItTy(_v.end(), DerefFn(Deref));
+    }
+
+    llvm::iterator_range<typename AtomVector<T>::iterator> owning_ptrs() {
+      return llvm::make_range(_v.begin(), _v.end());
+    }
+
+    llvm::iterator_range<typename AtomVector<T>::iterator> owning_ptrs() const {
+      return llvm::make_range(_v.begin(), _v.end());
+    }
+
+    bool empty() const {
+      return _v.empty();
+    }
+
+    size_t size() const {
+      return _v.size();
+    }
+
+    const OwningAtomPtr<T> &operator[](size_t idx) const {
+      return _v[idx];
+    }
+
+    OwningAtomPtr<T> &operator[](size_t idx) {
+      return _v[idx];
+    }
 
   private:
     AtomVector<T> &_v;
   };
 
   /// \brief Must be implemented to return the AtomVector object for
   /// all DefinedAtoms in this File.
-  virtual const AtomVector<DefinedAtom> &defined() const = 0;
+  virtual const AtomRange<DefinedAtom> defined() const = 0;
 
   /// \brief Must be implemented to return the AtomVector object for
   /// all UndefinedAtomw in this File.
-  virtual const AtomVector<UndefinedAtom> &undefined() const = 0;
+  virtual const AtomRange<UndefinedAtom> undefined() const = 0;
 
   /// \brief Must be implemented to return the AtomVector object for
   /// all SharedLibraryAtoms in this File.
-  virtual const AtomVector<SharedLibraryAtom> &sharedLibrary() const = 0;
+  virtual const AtomRange<SharedLibraryAtom> sharedLibrary() const = 0;
 
   /// \brief Must be implemented to return the AtomVector object for
   /// all AbsoluteAtoms in this File.
-  virtual const AtomVector<AbsoluteAtom> &absolute() const = 0;
+  virtual const AtomRange<AbsoluteAtom> absolute() const = 0;
+
+  /// Drop all of the atoms owned by this file.  This will result in all of
+  /// the atoms running their destructors.
+  /// This is required because atoms may be allocated on a BumpPtrAllocator
+  /// of a different file.  We need to destruct all atoms before any files.
+  virtual void clearAtoms() = 0;
 
   /// \brief If a file is parsed using a different method than doParse(),
   /// one must use this method to set the last error status, so that
@@ -194,19 +254,22 @@ class ErrorFile : public File {
 
   std::error_code doParse() override { return _ec; }
 
-  const AtomVector<DefinedAtom> &defined() const override {
+  const AtomRange<DefinedAtom> defined() const override {
     llvm_unreachable("internal error");
   }
-  const AtomVector<UndefinedAtom> &undefined() const override {
+  const AtomRange<UndefinedAtom> undefined() const override {
     llvm_unreachable("internal error");
   }
-  const AtomVector<SharedLibraryAtom> &sharedLibrary() const override {
+  const AtomRange<SharedLibraryAtom> sharedLibrary() const override {
     llvm_unreachable("internal error");
   }
-  const AtomVector<AbsoluteAtom> &absolute() const override {
+  const AtomRange<AbsoluteAtom> absolute() const override {
     llvm_unreachable("internal error");
   }
 
+  void clearAtoms() override {
+  }
+
 private:
   std::error_code _ec;
 };

lld/include/lld/Core/Resolver.h

@@ -35,10 +35,10 @@ class Resolver {
   Resolver(LinkingContext &ctx) : _ctx(ctx), _result(new MergedFile()) {}
 
   // InputFiles::Handler methods
-  void doDefinedAtom(const DefinedAtom&);
-  bool doUndefinedAtom(const UndefinedAtom &);
-  void doSharedLibraryAtom(const SharedLibraryAtom &);
-  void doAbsoluteAtom(const AbsoluteAtom &);
+  void doDefinedAtom(OwningAtomPtr<DefinedAtom> atom);
+  bool doUndefinedAtom(OwningAtomPtr<UndefinedAtom> atom);
+  void doSharedLibraryAtom(OwningAtomPtr<SharedLibraryAtom> atom);
+  void doAbsoluteAtom(OwningAtomPtr<AbsoluteAtom> atom);
 
   // Handle files, this adds atoms from the current file thats
   // being processed by the resolver
@@ -71,17 +71,16 @@ class Resolver {
                                  UndefCallback callback);
 
   void markLive(const Atom *atom);
-  void addAtoms(const std::vector<const DefinedAtom *>&);
 
   class MergedFile : public SimpleFile {
   public:
     MergedFile() : SimpleFile("<linker-internal>", kindResolverMergedObject) {}
-    void addAtoms(std::vector<const Atom*>& atoms);
+    void addAtoms(llvm::MutableArrayRef<OwningAtomPtr<Atom>> atoms);
   };
 
   LinkingContext &_ctx;
   SymbolTable _symbolTable;
-  std::vector<const Atom *>     _atoms;
+  std::vector<OwningAtomPtr<Atom>>     _atoms;
   std::set<const Atom *>        _deadStripRoots;
   llvm::DenseSet<const Atom *>  _liveAtoms;
   llvm::DenseSet<const Atom *>  _deadAtoms;

lld/include/lld/Core/SharedLibraryAtom.h

@@ -44,6 +44,8 @@ class SharedLibraryAtom : public Atom {
 
 protected:
   SharedLibraryAtom() : Atom(definitionSharedLibrary) {}
+
+  ~SharedLibraryAtom() override = default;
 };
 
 } // namespace lld

lld/include/lld/Core/SharedLibraryFile.h

@@ -27,28 +27,35 @@ class SharedLibraryFile : public File {
   /// Check if the shared library exports a symbol with the specified name.
   /// If so, return a SharedLibraryAtom which represents that exported
   /// symbol.  Otherwise return nullptr.
-  virtual const SharedLibraryAtom *exports(StringRef name,
+  virtual OwningAtomPtr<SharedLibraryAtom> exports(StringRef name,
                                            bool dataSymbolOnly) const = 0;
 
   // Returns the install name.
   virtual StringRef getDSOName() const = 0;
 
-  const AtomVector<DefinedAtom> &defined() const override {
+  const AtomRange<DefinedAtom> defined() const override {
     return _definedAtoms;
   }
 
-  const AtomVector<UndefinedAtom> &undefined() const override {
+  const AtomRange<UndefinedAtom> undefined() const override {
     return _undefinedAtoms;
   }
 
-  const AtomVector<SharedLibraryAtom> &sharedLibrary() const override {
+  const AtomRange<SharedLibraryAtom> sharedLibrary() const override {
     return _sharedLibraryAtoms;
   }
 
-  const AtomVector<AbsoluteAtom> &absolute() const override {
+  const AtomRange<AbsoluteAtom> absolute() const override {
     return _absoluteAtoms;
   }
 
+  void clearAtoms() override {
+    _definedAtoms.clear();
+    _undefinedAtoms.clear();
+    _sharedLibraryAtoms.clear();
+    _absoluteAtoms.clear();
+  }
+
 protected:
   /// only subclasses of SharedLibraryFile can be instantiated
   explicit SharedLibraryFile(StringRef path) : File(path, kindSharedLibrary) {}