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

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

+//--------------------------------------------------------------------*- 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