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..243473c6b4f79 --- /dev/null +++ b/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> 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 >", 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() 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!"); 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"