diff --git a/bindings/pyroot/pythonizations/test/CMakeLists.txt b/bindings/pyroot/pythonizations/test/CMakeLists.txt index 7738dfd103769..c793432addeeb 100644 --- a/bindings/pyroot/pythonizations/test/CMakeLists.txt +++ b/bindings/pyroot/pythonizations/test/CMakeLists.txt @@ -198,6 +198,8 @@ endif() ROOT_ADD_PYUNITTEST(regression_18441 regression_18441.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 7d4b93c8810e0..45583f9b78e09 100644 --- a/core/metacling/src/TClingCallFunc.cxx +++ b/core/metacling/src/TClingCallFunc.cxx @@ -79,6 +79,19 @@ using std::string, std::map, std::ostringstream, std::make_pair; static unsigned long long gWrapperSerial = 0LL; static const string kIndentString(" "); +// Caches of the JIT-compiled call, constructor and destructor wrappers, keyed on +// the declaration they were generated for. They are globals of libCling on +// purpose: a function-local static would register its destructor on first use, +// which for the destructor wrappers can be in the middle of the user's program, +// i.e. after ROOT registered its atexit handlers. The cache would then be +// destroyed before TROOT::EndOfProcessCleanups() releases the remaining objects +// (e.g. a TTree branch of a class without compiled dictionary), and the lookup +// of the destructor wrapper would run on a dead map, see +// https://github.com/root-project/root/issues/23728. +static map gWrapperStore; +static map gCtorWrapperStore; +static map gDtorWrapperStore; + static inline void @@ -1009,8 +1022,6 @@ void TClingCallFunc::make_narg_call_with_return(const unsigned N, const string & tcling_callfunc_Wrapper_t TClingCallFunc::make_wrapper() { - static map gWrapperStore; - R__LOCKGUARD_CLING(gInterpreterMutex); const Decl *D = GetFunctionOrShadowDecl(); @@ -1137,8 +1148,6 @@ tcling_callfunc_ctor_Wrapper_t TClingCallFunc::make_ctor_wrapper(const TClingCla // //-- - static map gCtorWrapperStore; - R__LOCKGUARD_CLING(gInterpreterMutex); auto D = info->GetDecl(); @@ -1287,8 +1296,6 @@ TClingCallFunc::make_dtor_wrapper(const TClingClassInfo *info) // //-- - static map gDtorWrapperStore; - R__LOCKGUARD_CLING(gInterpreterMutex); const Decl *D = info->GetDecl();