From 7d93e498e5cd208d82d60fc54383c672e11c7782 Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Mon, 26 Oct 2026 19:59:19 +0000 Subject: [PATCH 1/4] [cppinterop] Move wrapper caches to per-interpreter storage The JIT-compiled call and destructor wrappers were cached in function-local static maps in make_wrapper() and make_dtor_wrapper(). The destructor of such a static is registered with the runtime at its first use, and C++ runs these destructors in reverse order of registration. When the first use happened after ROOT registered its atexit handlers, the cache was already destroyed by the time TROOT::EndOfProcessCleanups() ran, and deleting an object of a class without compiled dictionary from there (e.g. the object held by a TBranchElement of a dictionary-less collection) called find()/insert() on a dead std::map: std::map<...>::insert (this=) CppImpl::(anonymous namespace)::make_dtor_wrapper CppImpl::Destruct TClingCallFunc::ExecDestructor TClingClassInfo::Delete TBranchElement::ReleaseObject ... TROOT::EndOfProcessCleanups In PyROOT this is what happens as soon as the script constructs and drops any object of a class without compiled dictionary (for example a new template instantiation such as std::vector): its destructor is the first use of the cache, well after `import ROOT`. Store the caches in InterpreterInfo instead. The interpreter registry is an llvm::ManagedStatic, so the caches now live as long as the interpreter they belong to, and are also freed together with it. This is a backport of upstream CppInterOp commit b8ae9f666c ("Move wrapper caches from function-local statics to per-interpreter storage"), which ROOT master received with the CppInterOp upgrade in c8626da00fd. Fixes #23728 Assisted-by: claude-code:claude-fable-5-1 --- .../CppInterOp/lib/CppInterOp/CppInterOp.cpp | 36 +++++++++++++------ 1 file changed, 25 insertions(+), 11 deletions(-) diff --git a/interpreter/CppInterOp/lib/CppInterOp/CppInterOp.cpp b/interpreter/CppInterOp/lib/CppInterOp/CppInterOp.cpp index 9c6f00432765a..e4676c2fefce5 100755 --- a/interpreter/CppInterOp/lib/CppInterOp/CppInterOp.cpp +++ b/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 WrapperStore; + std::map 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> 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(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 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 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" From b480342b8ccd9b1270a7d64c1268c3826803fba3 Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Mon, 26 Oct 2026 19:59:20 +0000 Subject: [PATCH 2/4] [metacling] Pass withFree and nary to Cpp::Destruct in the right order Cpp::Destruct takes (object, scope, withFree, count), but TClingCallFunc::ExecDestructor passed (nary, withFree). Through the implicit conversions, TClingClassInfo::Delete (nary=0, withFree=true) ran the destructor but never freed the memory, so every object of a class without compiled dictionary deleted through TClass::Destructor leaked, and TClingClassInfo::DeleteArray (nary=1, withFree=true) used `delete` instead of `delete[]`. Measured with 180 interpreted objects of 1 MiB created with TClass::New and deleted with TClass::Destructor: RSS grew by 179 MiB before, 0 MiB after. Master has the right order since 17c57aa2fdc. Assisted-by: claude-code:claude-fable-5-1 --- core/metacling/src/TClingCallFunc.cxx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/metacling/src/TClingCallFunc.cxx b/core/metacling/src/TClingCallFunc.cxx index 7ac8f8b879c55..2a6b326d03df3 100644 --- a/core/metacling/src/TClingCallFunc.cxx +++ b/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!"); From ef5c2b946ddf4b88d88a6d73b40219e9e92a1511 Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Tue, 27 Oct 2026 09:25:33 +0000 Subject: [PATCH 3/4] [Python] Add regression test for the exit-time crash of issue #23728 Reads a tree with a branch of a dictionary-less collection in a fresh Python process, lets TROOT::EndOfProcessCleanups delete it at exit, and constructs another dictionary-less object before exiting. This crashed in the JIT-compiled destructor lookup before the CppInterOp upgrade. The child process runs with MALLOC_PERTURB_ set, because a use of destroyed objects only crashes reliably when freed memory is poisoned, and with ROOT_OBJECT_AUTO_REGISTRATION=1 plus explicit checks that the file is registered with gROOT and owns the tree, so the test keeps exercising the scenario when object auto-registration is disabled. Assisted-by: claude-code:claude-fable-5-1 (cherry picked from commit a41307c1d22acbc53aeed4ef7cbfc4a37b6874f1) --- .../pyroot/pythonizations/test/CMakeLists.txt | 2 + .../pythonizations/test/regression_23728.py | 81 +++++++++++++++++++ 2 files changed, 83 insertions(+) create mode 100644 bindings/pyroot/pythonizations/test/regression_23728.py diff --git a/bindings/pyroot/pythonizations/test/CMakeLists.txt b/bindings/pyroot/pythonizations/test/CMakeLists.txt index 5e5ce5cfd5b87..d2c7051de83fd 100644 --- a/bindings/pyroot/pythonizations/test/CMakeLists.txt +++ b/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: diff --git a/bindings/pyroot/pythonizations/test/regression_23728.py b/bindings/pyroot/pythonizations/test/regression_23728.py new file mode 100644 index 0000000000000..0371a664c691f --- /dev/null +++ b/bindings/pyroot/pythonizations/test/regression_23728.py @@ -0,0 +1,81 @@ +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> 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]]() + t.Branch("vv", v) + 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") + + # Make the interpreter JIT-compile a destructor before exit. + ROOT.std.vector[ROOT.TNamed]() + + 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") + + res = subprocess.run( + [sys.executable, "-c", script], + stdout=subprocess.PIPE, + stderr=subprocess.STDOUT, + text=True, + env=env, + ) + + self.assertEqual(res.returncode, 0, msg=res.stdout) + self.assertIn("end of script", res.stdout) + + +if __name__ == "__main__": + unittest.main() From 40157abcdc4b37af052ecef44dccbe5e6a6ba65e Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Wed, 28 Oct 2026 19:42:22 +0000 Subject: [PATCH 4/4] [Python] Make the regression test for #23728 work effectively The test left the destructor of the branch class uncompiled until exit, so that cling had to JIT-compile it from TROOT::EndOfProcessCleanups(). On Windows, that cleanup runs from libCore's DLL_PROCESS_DETACH, after libCling's statics have already been torn down, so compiling anything there crashes with or without the fix (the no-fix run in #23850 fails identically). Pre-compile the exact destructor instead: the exit-time call is then a pure cache lookup, which is the regular exit path on Windows. The bug was in that lookup on the destroyed function-local static, so the test still fails without the fix. Write the branch through the overload taking the class name and the address. The templated Branch() that the pythonization on master calls refuses collections without a compiled CollectionProxy, which silently left the tree without the branch and the test without its purpose. Also give the child process a timeout: without the fix, ROOT's crash handler can hang instead of terminating the process. Assisted-by: claude-code:claude-fable-5-1 (cherry picked from commit 98526e5303004fc5d11c4ae8f281324d3b049179) --- .../pythonizations/test/regression_23728.py | 29 +++++++++++++++++-- 1 file changed, 26 insertions(+), 3 deletions(-) diff --git a/bindings/pyroot/pythonizations/test/regression_23728.py b/bindings/pyroot/pythonizations/test/regression_23728.py index 0371a664c691f..243473c6b4f79 100644 --- a/bindings/pyroot/pythonizations/test/regression_23728.py +++ b/bindings/pyroot/pythonizations/test/regression_23728.py @@ -29,7 +29,12 @@ def test_exit_cleanup_after_interpreted_destructor(self): with ROOT.TFile(fname, "RECREATE") as f: t = ROOT.TTree("t", "t") v = ROOT.std.vector[ROOT.std.vector[int]]() - t.Branch("vv", v) + # 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 >", 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() @@ -53,8 +58,22 @@ def test_exit_cleanup_after_interpreted_destructor(self): if t.GetDirectory() != f: raise RuntimeError("the tree is not owned by the file") - # Make the interpreter JIT-compile a destructor before exit. - ROOT.std.vector[ROOT.TNamed]() + # 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") """ @@ -65,12 +84,16 @@ def test_exit_cleanup_after_interpreted_destructor(self): # 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)