Fix registration of other atexit functions during an atexit handler. …

…Recursive registration of atexit handlers is legal and should be handled, not ignored.

interpreter/cling/include/cling/Utils/OrderedMap.h

@@ -0,0 +1,116 @@
+//--------------------------------------------------------------------*- C++ -*-
+// CLING - the C++ LLVM-based InterpreterG :)
+// author: Roman Zulak
+//
+// This file is dual-licensed: you can choose to license it under the University
+// of Illinois Open Source License or the GNU Lesser General Public License. See
+// LICENSE.TXT for details.
+//------------------------------------------------------------------------------
+
+#ifndef CLING_ORDERED_MAP_H
+#define CLING_ORDERED_MAP_H
+
+#include <unordered_map>
+#include <cassert>
+
+namespace cling {
+namespace utils {
+
+///\brief Thin wrapper class for tracking the order of insertion into a
+/// std::unordered_map.
+///
+/// Only supports 'emplace' and '[Key]' for insertion of values, and adds an
+/// additional parameter to 'erase' so that a mapped value can be moved into
+/// local storage before erasing the iterator occurs.
+///
+template <typename Key, typename Value>
+class OrderedMap {
+  typedef std::unordered_map<Key, Value> map_t;
+  // Would this be faster as a std::unoredered_map<Key, size_t> for erasure?
+  typedef std::vector<typename map_t::const_iterator> order_t;
+
+  map_t m_Map;
+  order_t m_Order;
+
+public:
+  typedef typename map_t::iterator iterator;
+  typedef typename map_t::const_iterator const_iterator;
+  typedef typename map_t::mapped_type mapped_type;
+
+  template <typename... Args>
+  std::pair<iterator, bool> emplace(Args&&... args) {
+    auto Rval = m_Map.emplace(std::forward<Args>(args)...);
+    if (Rval.second) m_Order.emplace_back(Rval.first);
+    return Rval;
+  }
+
+  Value& operator[](const Key& K) {
+    iterator Itr = find(K);
+    if (Itr == end()) {
+      Itr = m_Map.emplace(K, Value()).first;
+      m_Order.emplace_back(Itr);
+    }
+    return Itr->second;
+  }
+
+  Value& operator[](Key&& K) {
+    iterator Itr = find(K);
+    if (Itr == end()) {
+      Itr = m_Map.emplace(K, Value()).first;
+      m_Order.emplace_back(Itr);
+    }
+    return Itr->second;
+  }
+
+  ///\brief Erase a mapping from this object.
+  ///
+  ///\param [in] Itr - The iterator to erase.
+  ///\param [out] Move - Move the mapped object to this pointer before erasing.
+  ///
+  void erase(const_iterator Itr, mapped_type* Move = nullptr) {
+    assert(std::find(m_Order.begin(), m_Order.end(), Itr) != m_Order.end());
+    for (auto Otr = m_Order.begin(), End = m_Order.end(); Otr != End; ++Otr) {
+      if (Itr == *Otr) {
+        m_Order.erase(Otr);
+        break;
+      }
+    }
+    assert(std::find(m_Order.begin(), m_Order.end(), Itr) == m_Order.end());
+    if (Move) *Move = std::move(Itr->second);
+    m_Map.erase(Itr);
+  }
+
+  iterator find(const Key& K) { return m_Map.find(K); }
+  const_iterator find(const Key& K) const { return m_Map.find(K); }
+
+  iterator end() { return m_Map.end(); }
+  const_iterator end() const { return m_Map.end(); }
+
+  void swap(OrderedMap& Other) {
+    m_Map.swap(Other.m_Map);
+    m_Order.swap(Other.m_Order);
+  }
+
+  void clear() {
+    m_Map.clear();
+    m_Order.clear();
+  }
+
+  bool empty() const {
+    assert(m_Map.empty() == m_Order.empty() && "Not synchronized");
+    return m_Order.empty();
+  }
+
+  void size() const {
+    assert(m_Map.size() == m_Order.size() && "Not synchronized");
+    return m_Order.size();
+  }
+
+  const order_t& ordered() const { return m_Order; }
+  order_t& ordered() { return m_Order; }
+};
+
+} // namespace utils
+} // namespace cling
+
+#endif // CLING_PLATFORM_H

interpreter/cling/lib/Interpreter/IncrementalExecutor.cpp

@@ -92,10 +92,6 @@ IncrementalExecutor::IncrementalExecutor(clang::DiagnosticsEngine& diags,
   // MSVC doesn't support m_AtExitFuncsSpinLock=ATOMIC_FLAG_INIT; in the class definition
   std::atomic_flag_clear( &m_AtExitFuncsSpinLock );
 
-  // No need to protect this access of m_AtExitFuncs, since nobody
-  // can use this object yet.
-  m_AtExitFuncs.reserve(256);
-
   std::unique_ptr<TargetMachine> TM(CreateHostTargetMachine(CI));
   m_BackendPasses.reset(new BackendPasses(CI.getCodeGenOpts(),
                                           CI.getTargetOpts(),
@@ -107,18 +103,33 @@ IncrementalExecutor::IncrementalExecutor(clang::DiagnosticsEngine& diags,
 // Keep in source: ~unique_ptr<ClingJIT> needs ClingJIT
 IncrementalExecutor::~IncrementalExecutor() {}
 
+
 void IncrementalExecutor::shuttingDown() {
-  // No need to protect this access, since hopefully there is no concurrent
-  // shutdown request.
-  for (auto& AtExit : llvm::reverse(m_AtExitFuncs))
-    AtExit();
+  // It is legal to register an atexit handler from within another atexit
+  // handler and furthor-more the standard says they need to run in reverse
+  // order, so this function must be recursion safe.
+  AtExitFunctions Local;
+  {
+    cling::internal::SpinLockGuard slg(m_AtExitFuncsSpinLock);
+    // Check this case first, to avoid the swap all-together.
+    if (m_AtExitFuncs.empty())
+      return;
+    Local.swap(m_AtExitFuncs);
+  }
+  for (auto&& Ordered: llvm::reverse(Local.ordered())) {
+    for (auto&& AtExit : llvm::reverse(Ordered->second))
+      AtExit();
+      // The standard says that they need to run in reverse order, which means
+      // anything added from 'AtExit()' must now be run!
+      shuttingDown();
+  }
 }
 
 void IncrementalExecutor::AddAtExitFunc(void (*func) (void*), void* arg,
                                         llvm::Module* M) {
   // Register a CXAAtExit function
   cling::internal::SpinLockGuard slg(m_AtExitFuncsSpinLock);
-  m_AtExitFuncs.push_back(CXAAtExitElement(func, arg, M));
+  m_AtExitFuncs[M].emplace_back(func, arg);
 }
 
 void unresolvedSymbol()
@@ -286,23 +297,21 @@ IncrementalExecutor::runStaticInitializersOnce(const Transaction& T) {
 void IncrementalExecutor::runAndRemoveStaticDestructors(Transaction* T) {
   assert(T && "Must be set");
   // Collect all the dtors bound to this transaction.
-  AtExitFunctions boundToT;
-
+  AtExitFunctions::mapped_type Local;
   {
     cling::internal::SpinLockGuard slg(m_AtExitFuncsSpinLock);
-    for (AtExitFunctions::iterator I = m_AtExitFuncs.begin();
-         I != m_AtExitFuncs.end();)
-      if (I->getModule() == T->getModule()) {
-        boundToT.push_back(*I);
-        I = m_AtExitFuncs.erase(I);
-      }
-      else
-        ++I;
+    auto Itr = m_AtExitFuncs.find(T->getModule());
+    if (Itr == m_AtExitFuncs.end())
+      return;
+    m_AtExitFuncs.erase(Itr, &Local);
   } // end of spin lock lifetime block.
 
-  // 'Unload' the cxa_atexit entities.
-  for (auto&& AtExit : llvm::reverse(boundToT))
+  // 'Unload' the cxa_atexit, atexit entities.
+  for (auto&& AtExit : llvm::reverse(Local)) {
     AtExit();
+    // Run anything that was just registered in 'AtExit()'
+    runAndRemoveStaticDestructors(T);
+  }
 }
 
 void

interpreter/cling/lib/Interpreter/IncrementalExecutor.h

@@ -16,6 +16,7 @@
 #include "cling/Interpreter/Transaction.h"
 #include "cling/Interpreter/Value.h"
 #include "cling/Utils/Casting.h"
+#include "cling/Utils/OrderedMap.h"
 
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
@@ -73,10 +74,6 @@ namespace cling {
       ///
       void* m_Arg;
 
-      ///\brief The module whose unloading will trigger the call to this atexit
-      /// function.
-      ///
-      const llvm::Module* m_FromM;
     public:
       ///\brief Constructs an element, whose destruction time will be managed by
       /// the interpreter. (By registering a function to be called by exit
@@ -96,12 +93,10 @@ namespace cling {
       ///\param [in] fromT - The unloading of this transaction will trigger the
       ///                    atexit function.
       ///
-      CXAAtExitElement(void (*func) (void*), void* arg,
-                       const llvm::Module* fromM):
-        m_Func(func), m_Arg(arg), m_FromM(fromM) {}
+      CXAAtExitElement(void (*func) (void*), void* arg):
+        m_Func(func), m_Arg(arg) {}
 
       void operator () () const { (*m_Func)(m_Arg); }
-      const llvm::Module* getModule() const { return m_FromM; }
     };
 
     ///\brief Atomic used as a spin lock to protect the access to m_AtExitFuncs
@@ -112,10 +107,11 @@ namespace cling {
     /// again multiple conccurent access.
     std::atomic_flag m_AtExitFuncsSpinLock; // MSVC doesn't support = ATOMIC_FLAG_INIT;
 
-    typedef llvm::SmallVector<CXAAtExitElement, 128> AtExitFunctions;
-    ///\brief Static object, which are bound to unloading of certain declaration
-    /// to be destructed.
+    ///\brief Function registered via __cxa_atexit, atexit, or one of
+    /// it's C++ overloads that should be run when a module is unloaded.
     ///
+    typedef utils::OrderedMap<llvm::Module*, std::vector<CXAAtExitElement>>
+        AtExitFunctions;
     AtExitFunctions m_AtExitFuncs;
 
     ///\brief Modules to emit upon the next call to the JIT.

interpreter/cling/test/CodeUnloading/AtExit.C

@@ -35,6 +35,20 @@ at_quick_exit(atexit_2);
 // Make sure at_quick_exit is resolved correctly (mangling issues on gcc < 5)
 // CHECK-NEXT: atexit_2
 
+// Test reverse ordering in a single transaction.
+static void atexitA() { printf("atexitA\n"); }
+static void atexitB() { printf("atexitB\n"); }
+static void atexitC() { printf("atexitC\n"); }
+{
+  std::atexit(atexitA);
+  std::atexit(atexitB);
+  std::atexit(atexitC);
+}
+.undo
+// CHECK-NEXT: atexitC
+// CHECK-NEXT: atexitB
+// CHECK-NEXT: atexitA
+
 atexit(atexit_3);
 
 cling::Interpreter * gChild = 0;
@@ -53,9 +67,58 @@ static void atexit_f() {
 }
 at_quick_exit(atexit_f);
 
+void atExit0 () {
+  printf("atExit0\n");
+}
+void atExit1 () {
+  printf("atExit1\n");
+}
+void atExit2 () {
+  printf("atExit2\n");
+}
+void atExitA () {
+  printf("atExitA\n");
+  std::atexit(&atExit0);
+}
+void atExitB () {
+  printf("atExitB\n");
+  std::atexit(&atExit1);
+  std::atexit(&atExit2);
+}
+// Recursion in a Transaction
+{
+  std::atexit(&atExitA);
+  std::atexit(&atExitB);
+}
+.undo
+// CHECK-NEXT: atExitB
+// CHECK-NEXT: atExit2
+// CHECK-NEXT: atExit1
+// CHECK-NEXT: atExitA
+// CHECK-NEXT: atExit0
+
+// Recusion at shutdown
+struct ShutdownRecursion {
+  static void DtorAtExit0() { printf("ShutdownRecursion0\n"); }
+  static void DtorAtExit1() { printf("ShutdownRecursion1\n"); }
+  static void DtorAtExit2() { printf("ShutdownRecursion2\n"); }
+  ~ShutdownRecursion() {
+    printf("~ShutdownRecursion\n");
+    atexit(&DtorAtExit0);
+    atexit(&DtorAtExit1);
+    atexit(&DtorAtExit2);
+  }
+} Out;
+
 // expected-no-diagnostics
 .q
 
 // Reversed registration order
+
+// CHECK-NEXT: ~ShutdownRecursion
+// CHECK-NEXT: ShutdownRecursion2
+// CHECK-NEXT: ShutdownRecursion1
+// CHECK-NEXT: ShutdownRecursion0
+
 // CHECK-NEXT: atexit_f true
 // CHECK-NEXT: atexit_3