[v640] Fixes and unit test for Python exit-time crash issue #23728

bindings/pyroot/pythonizations/test/CMakeLists.txt

@@ -186,6 +186,8 @@ ROOT_ADD_PYUNITTEST(regression_18441 regression_18441.py)
 
 ROOT_ADD_PYUNITTEST(regression_pickle_no_cppyy regression_pickle_no_cppyy.py)
 
+ROOT_ADD_PYUNITTEST(regression_23728 regression_23728.py)
+
 if(NOT MSVC OR CMAKE_SIZEOF_VOID_P EQUAL 8 OR win_broken_tests)
     # Test the conversion and low level views of interpreter-defined C++ arrays to NumPy
     # We do not run this test on Windows x86 due to an interpreter issue with arrays:

bindings/pyroot/pythonizations/test/regression_23728.py

@@ -0,0 +1,104 @@
+import os
+import subprocess
+import sys
+import tempfile
+import textwrap
+import unittest
+
+
+class Regression23728(unittest.TestCase):
+    """
+    Deleting an object of a class without compiled dictionary during
+    TROOT::EndOfProcessCleanups() must not crash, even if the interpreter
+    already JIT-compiled a destructor earlier in the process. The cache of
+    compiled destructors used to be a function-local static, destroyed before
+    the end-of-process cleanup ran because its first use came after ROOT
+    registered its atexit handlers.
+
+    See https://github.com/root-project/root/issues/23728
+    """
+
+    def test_exit_cleanup_after_interpreted_destructor(self):
+        import ROOT
+
+        with tempfile.TemporaryDirectory() as tmpdir:
+            fname = os.path.join(tmpdir, "vv.root")
+
+            # vector<vector<int>> has no compiled dictionary, so its destructor
+            # has to be JIT-compiled when the branch object is deleted.
+            with ROOT.TFile(fname, "RECREATE") as f:
+                t = ROOT.TTree("t", "t")
+                v = ROOT.std.vector[ROOT.std.vector[int]]()
+                # Use the overload taking the class name and the address: the
+                # templated Branch() refuses collections without a compiled
+                # CollectionProxy, which would silently leave the tree without
+                # the branch and the test without its purpose.
+                branch = t.Branch("vv", "vector<vector<int> >", ROOT.AddressOf(v))
+                self.assertTrue(branch, "Branch() refused to create the branch")
+                v.push_back(ROOT.std.vector[int]([1, 2]))
+                t.Fill()
+                f.Write()
+
+            script = textwrap.dedent(
+                f"""
+                import ROOT
+
+                f = ROOT.TFile({fname!r})
+                # Let TROOT::EndOfProcessCleanups() delete the file, and with it
+                # the tree and its branch object, from the C atexit handler.
+                ROOT.SetOwnership(f, False)
+                t = f.Get("t")
+                t.GetEntry(0)
+
+                # That only happens if the file is registered with gROOT and owns
+                # the tree. Check it, so that the test fails loudly instead of
+                # silently losing its purpose if these defaults change.
+                if not ROOT.gROOT.GetListOfFiles().FindObject(f):
+                    raise RuntimeError("the file is not in gROOT->GetListOfFiles()")
+                if t.GetDirectory() != f:
+                    raise RuntimeError("the tree is not owned by the file")
+
+                # Sanity checks that the branch exists and that bytes are
+                # actually read, which is required for the reproducer.
+                if not t.GetBranch("vv"):
+                    raise RuntimeError("the tree has no 'vv' branch")
+                if t.GetEntry(0) <= 0:
+                    raise RuntimeError("no bytes read for entry 0: no branch object was created")
+
+                # Make the interpreter JIT-compile the destructor of the branch
+                # class before exit. This creates the destructor cache before the
+                # end-of-process cleanup needs it (and, before the fix, destroyed
+                # it again before that). Using the very same class keeps the
+                # exit-time destructor call a pure cache lookup: compiling code
+                # at that point is not possible on Windows, where the atexit
+                # handler of libCore runs only when libCling's statics have
+                # already been torn down by its DLL_PROCESS_DETACH.
+                ROOT.std.vector[ROOT.std.vector[int]]()
+
+                print("end of script")
+                """
+            )
+
+            # Poison freed memory, so that a use of destroyed objects (like the
+            # dead destructor cache of the issue) crashes deterministically,
+            # whatever the heap layout and the interpreter internals.
+            env = dict(os.environ, MALLOC_PERTURB_="85")
+
+            # Without the fix, ROOT's crash handler can hang on the way out
+            # of the segfault instead of terminating the process: fail
+            # instead of waiting for the ctest timeout.
+            res = subprocess.run(
+                [sys.executable, "-c", script],
+                stdout=subprocess.PIPE,
+                stderr=subprocess.STDOUT,
+                text=True,
+                env=env,
+                timeout=300,
+            )
+
+            self.assertEqual(res.returncode, 0, msg=res.stdout)
+            self.assertIn("end of script", res.stdout)
+
+
+if __name__ == "__main__":
+    unittest.main()

core/metacling/src/TClingCallFunc.cxx

@@ -1271,7 +1271,7 @@ void TClingCallFunc::ExecDestructor(const TClingClassInfo *info, void *address /
 
    R__LOCKGUARD_CLING(gInterpreterMutex);
 
-   if (Cpp::Destruct(address, info->GetDecl(), nary, withFree))
+   if (Cpp::Destruct(address, info->GetDecl(), withFree, nary))
       return;
 
    ::Error("TClingCallFunc::ExecDestructor", "Called with no wrapper, not implemented!");

interpreter/CppInterOp/lib/CppInterOp/CppInterOp.cpp

@@ -128,6 +128,11 @@ using namespace llvm;
 struct InterpreterInfo {
   compat::Interpreter* Interpreter = nullptr;
   bool isOwned = true;
+  // Per-interpreter wrapper caches. Keyed on AST nodes that belong to this
+  // interpreter, so the caches must be destroyed together with it.
+  std::map<const FunctionDecl*, void*> WrapperStore;
+  std::map<const Decl*, void*> DtorWrapperStore;
+
   InterpreterInfo(compat::Interpreter* I, bool Owned)
       : Interpreter(I), isOwned(Owned) {}
 
@@ -165,12 +170,21 @@ struct InterpreterInfo {
 // std::deque avoids relocations and calling the dtor of InterpreterInfo.
 static llvm::ManagedStatic<std::deque<InterpreterInfo>> sInterpreters;
 
+static InterpreterInfo& getInterpInfo(compat::Interpreter* I = nullptr) {
+  assert(!sInterpreters->empty() &&
+         "Interpreter instance must be set before calling this!");
+  if (I) {
+    for (auto& Info : *sInterpreters)
+      if (Info.Interpreter == I)
+        return Info;
+  }
+  return sInterpreters->back();
+}
+
 static compat::Interpreter& getInterp(TInterp_t I = nullptr) {
   if (I)
     return *static_cast<compat::Interpreter*>(I);
-  assert(!sInterpreters->empty() &&
-         "Interpreter instance must be set before calling this!");
-  return *sInterpreters->back().Interpreter;
+  return *getInterpInfo().Interpreter;
 }
 
 static clang::Sema& getSema() { return getInterp().getCI()->getSema(); }
@@ -3003,10 +3017,10 @@ int get_wrapper_code(compat::Interpreter& I, const FunctionDecl* FD,
 
 JitCall::GenericCall make_wrapper(compat::Interpreter& I,
                                   const FunctionDecl* FD) {
-  static std::map<const FunctionDecl*, void*> gWrapperStore;
+  auto& WrapperStore = getInterpInfo(&I).WrapperStore;
 
-  auto R = gWrapperStore.find(FD);
-  if (R != gWrapperStore.end())
+  auto R = WrapperStore.find(FD);
+  if (R != WrapperStore.end())
     return (JitCall::GenericCall)R->second;
 
   std::string wrapper_name;
@@ -3025,7 +3039,7 @@ JitCall::GenericCall make_wrapper(compat::Interpreter& I,
   void* wrapper =
       compile_wrapper(I, wrapper_name, wrapper_code, withAccessControl);
   if (wrapper) {
-    gWrapperStore.insert(std::make_pair(FD, wrapper));
+    WrapperStore.insert(std::make_pair(FD, wrapper));
   } else {
     llvm::errs() << "TClingCallFunc::make_wrapper"
                  << ":"
@@ -3095,10 +3109,10 @@ static JitCall::DestructorCall make_dtor_wrapper(compat::Interpreter& interp,
   //
   //--
 
-  static std::map<const Decl*, void*> gDtorWrapperStore;
+  auto& DtorWrapperStore = getInterpInfo(&interp).DtorWrapperStore;
 
-  auto I = gDtorWrapperStore.find(D);
-  if (I != gDtorWrapperStore.end())
+  auto I = DtorWrapperStore.find(D);
+  if (I != DtorWrapperStore.end())
     return (JitCall::DestructorCall)I->second;
 
   //
@@ -3199,7 +3213,7 @@ static JitCall::DestructorCall make_dtor_wrapper(compat::Interpreter& interp,
   void* F = compile_wrapper(interp, wrapper_name, wrapper,
                             /*withAccessControl=*/false);
   if (F) {
-    gDtorWrapperStore.insert(std::make_pair(D, F));
+    DtorWrapperStore.insert(std::make_pair(D, F));
   } else {
     llvm::errs() << "make_dtor_wrapper"
                  << "Failed to compile\n"