From 03805955bb8bd7cc406f84f39563b2b2bf6d587e Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Thu, 29 Oct 2026 12:49:02 +0000 Subject: [PATCH] [RF] Don't touch deleted nodes in RooFit::Evaluator destructor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Evaluator resets the data tokens of all nodes of its computation graph in the destructor, through raw pointers and without knowing whether the nodes still exist. Nodes that outlive the Evaluator need this reset, because a node can only be registered with one Evaluator at a time. But nodes can also be deleted before the Evaluator: the compiled NLL graph of a RooMomentMorphFuncND references the original morph function and its RooHistPdf templates owned by the RooWorkspace, and at the exit of a Python script the garbage collector can delete the workspace before the NLL object. The destructor then writes into freed memory, which showed up as a `free(): invalid pointer` abort of tutorial-roofit-roofit-rf616_morphing-py in the v6-42-00-rc1 release build on ubuntu2610 (valgrind: "Invalid write of size 8" in RooFit::Evaluator::~Evaluator()). Track the ownership of the data token: RooAbsArg remembers the Evaluator that set its token, tells it in the destructor that the node is gone (Evaluator::unregisterNode), and the Evaluator skips such nodes when resetting the tokens. Add a unit test for the Evaluator that deletes a node before the Evaluator and checks that tokens can be reused in both orders, and a regression test for the workspace-deleted-before-NLL scenario with the morph pdf. This also means we can remove the workarounds in the tutorials introduced by commits 49fb353caf7 and 06879112f32. 🤖 Done with the help of AI (cherry picked from commit 8242cdc40c1d5dd5a62eb5a2a7e8853dcaa5e0ef) --- .../roofit/test/testRooMomentMorphFuncND.cxx | 68 ++++++++++++++----- roofit/roofitcore/inc/RooAbsArg.h | 10 ++- roofit/roofitcore/inc/RooFit/Evaluator.h | 2 + roofit/roofitcore/src/RooAbsArg.cxx | 34 +++++++--- roofit/roofitcore/src/RooFit/Evaluator.cxx | 17 ++++- roofit/roofitcore/test/CMakeLists.txt | 1 + roofit/roofitcore/test/testRooEvaluator.cxx | 43 ++++++++++++ .../roofit/roofit/rf619_discrete_profiling.py | 5 -- .../roofit/roostats/rs401d_FeldmanCousins.py | 6 -- 9 files changed, 143 insertions(+), 43 deletions(-) create mode 100644 roofit/roofitcore/test/testRooEvaluator.cxx diff --git a/roofit/roofit/test/testRooMomentMorphFuncND.cxx b/roofit/roofit/test/testRooMomentMorphFuncND.cxx index 8033df2170dd9..963e1e84e943f 100644 --- a/roofit/roofit/test/testRooMomentMorphFuncND.cxx +++ b/roofit/roofit/test/testRooMomentMorphFuncND.cxx @@ -3,6 +3,7 @@ #include #include +#include #include #include #include @@ -15,27 +16,17 @@ #include +#include #include namespace { -// For each morph configuration we expect: a normal `getVal(normSet)` on the -// uncompiled pdf and an Evaluator run on the same pdf compiled for that -// normSet must agree. Whether the morph is in pdf mode (sum is a RooAddPdf -// whose components must be normalized for the value to be a proper density) -// or in func mode (sum is a RooRealSumFunc, the value is the raw weighted -// sum of bin contents) is internal to compileForNormSet -- both paths must -// produce the same final value. -void checkBackendConsistency(bool pdfMode) +/// Workspace with the observable "x", the morphing parameter "mu", and the +/// morph pdf "morph" with two RooHistPdf templates. +std::unique_ptr makeMorphWorkspace(bool pdfMode) { - // FATAL silences a transient logEvalError that the morph emits from its - // slope/offset RooFormulaVars at cache-construction time (in func mode the - // initial rms-fractions are zero, so slope = inf for one self-evaluation - // before calculateFractions runs). It does not affect correctness. - // TODO: its not elegant that this happens and should be fixed. - RooHelpers::LocalChangeMsgLevel changeMsgLevel{RooFit::FATAL}; - - RooWorkspace ws; + auto wsPtr = std::make_unique(); + RooWorkspace &ws = *wsPtr; ws.factory("x[-5,15]"); ws.factory("mu[0.5,0.0,1.0]"); RooRealVar &x = *ws.var("x"); @@ -63,7 +54,30 @@ void checkBackendConsistency(bool pdfMode) RooWrapperPdf morph("morph", "morph", morph_func, /*selfNormalized=*/true); ws.import(morph, RooFit::Silence(true)); - RooAbsPdf &morphPdf = *ws.pdf("morph"); + return wsPtr; +} + +// For each morph configuration we expect: a normal `getVal(normSet)` on the +// uncompiled pdf and an Evaluator run on the same pdf compiled for that +// normSet must agree. Whether the morph is in pdf mode (sum is a RooAddPdf +// whose components must be normalized for the value to be a proper density) +// or in func mode (sum is a RooRealSumFunc, the value is the raw weighted +// sum of bin contents) is internal to compileForNormSet -- both paths must +// produce the same final value. +void checkBackendConsistency(bool pdfMode) +{ + // FATAL silences a transient logEvalError that the morph emits from its + // slope/offset RooFormulaVars at cache-construction time (in func mode the + // initial rms-fractions are zero, so slope = inf for one self-evaluation + // before calculateFractions runs). It does not affect correctness. + // TODO: its not elegant that this happens and should be fixed. + RooHelpers::LocalChangeMsgLevel changeMsgLevel{RooFit::FATAL}; + + std::unique_ptr ws = makeMorphWorkspace(pdfMode); + RooRealVar &x = *ws->var("x"); + RooRealVar &mu = *ws->var("mu"); + + RooAbsPdf &morphPdf = *ws->pdf("morph"); RooArgSet normSet(x); mu.setVal(0.5); @@ -89,3 +103,23 @@ TEST(RooMomentMorphFuncND, BackendConsistencyFuncMode) { checkBackendConsistency(/*pdfMode=*/false); } + +/// The compiled computation graph of the morph pdf references the original +/// morph function and the RooHistPdf templates owned by the workspace. If the +/// workspace is deleted before an NLL object, the destructor of the NLL must +/// not touch the deleted nodes anymore. This happened at the exit of the +/// rf616_morphing.py tutorial, where the Python garbage collector deletes the +/// workspace first, and was observed as a `free(): invalid pointer` abort. +TEST(RooMomentMorphFuncND, WorkspaceDeletedBeforeNLL) +{ + RooHelpers::LocalChangeMsgLevel changeMsgLevel{RooFit::FATAL}; + + std::unique_ptr ws = makeMorphWorkspace(/*pdfMode=*/true); + RooAbsPdf &morphPdf = *ws->pdf("morph"); + std::unique_ptr data{ws->pdf("hp0")->generate(*ws->var("x"), 100)}; + std::unique_ptr nll{morphPdf.createNLL(*data)}; + EXPECT_TRUE(std::isfinite(nll->getVal())); + + ws.reset(); + nll.reset(); +} diff --git a/roofit/roofitcore/inc/RooAbsArg.h b/roofit/roofitcore/inc/RooAbsArg.h index b53f27a8185b6..bdc29c720b296 100644 --- a/roofit/roofitcore/inc/RooAbsArg.h +++ b/roofit/roofitcore/inc/RooAbsArg.h @@ -52,6 +52,7 @@ using RooListProxy = RooCollectionProxy; class RooExpensiveObjectCache; class RooWorkspace; namespace RooFit { +class Evaluator; namespace Experimental { class CodegenContext; } @@ -612,8 +613,12 @@ class RooAbsArg : public TNamed, public RooPrintable { /// Returns the token for retrieving results in the BatchMode. For internal use only. std::size_t dataToken() const { return _dataToken; } bool hasDataToken() const { return _dataToken != std::numeric_limits::max(); } - void setDataToken(std::size_t index); - void resetDataToken() { _dataToken = std::numeric_limits::max(); } + void setDataToken(std::size_t index, RooFit::Evaluator *evaluator = nullptr); + void resetDataToken() + { + _dataToken = std::numeric_limits::max(); + _evaluator = nullptr; + } protected: mutable bool _valueDirty = true; // Flag set if value needs recalculating because input values modified @@ -638,6 +643,7 @@ class RooAbsArg : public TNamed, public RooPrintable { std::size_t _dataToken = std::numeric_limits::max(); /// setOperModes(RooAbsArg::OperMode opMode); + void unregisterNode(RooAbsArg const &arg); + private: void processVariable(NodeInfo &nodeInfo); void processCategory(NodeInfo &nodeInfo); diff --git a/roofit/roofitcore/src/RooAbsArg.cxx b/roofit/roofitcore/src/RooAbsArg.cxx index 4f790ee178d7f..7fd7484a10bc5 100644 --- a/roofit/roofitcore/src/RooAbsArg.cxx +++ b/roofit/roofitcore/src/RooAbsArg.cxx @@ -74,6 +74,7 @@ for single nodes. #include #include #include +#include #include #include "RooFitImplHelpers.h" #include @@ -168,6 +169,12 @@ RooAbsArg::RooAbsArg(const RooAbsArg &other, const char *name) RooAbsArg::~RooAbsArg() { + // If a RooFit::Evaluator still holds a data token for this arg, tell it that + // the arg is gone so that it doesn't reset the token in its destructor. + if (_evaluator) { + _evaluator->unregisterNode(*this); + } + // Notify all servers that they no longer need to serve us while (!_serverList.empty()) { removeServer(*_serverList.containedObjects().back(), true); @@ -2164,17 +2171,22 @@ RooAbsArg::compileForNormSet(RooArgSet const &normSet, RooFit::Detail::CompileCo } /// Sets the token for retrieving results in the BatchMode. For internal use only. -void RooAbsArg::setDataToken(std::size_t index) -{ - if (_dataToken == index) { - return; +/// If an `evaluator` is passed, it is the RooFit::Evaluator that resets the +/// token in its destructor, and this arg will notify it if it gets deleted +/// before the Evaluator. +void RooAbsArg::setDataToken(std::size_t index, RooFit::Evaluator *evaluator) +{ + if (_dataToken != index) { + if (hasDataToken()) { + std::stringstream errMsg; + errMsg << "The data token for \"" << GetName() << "\" is already set!" + << " Are you trying to evaluate the same object by multiple RooFit::Evaluator instances?" + << " This is not allowed."; + throw std::runtime_error(errMsg.str()); + } + _dataToken = index; } - if (_dataToken != std::numeric_limits::max()) { - std::stringstream errMsg; - errMsg << "The data token for \"" << GetName() << "\" is already set!" - << " Are you trying to evaluate the same object by multiple RooFit::Evaluator instances?" - << " This is not allowed."; - throw std::runtime_error(errMsg.str()); + if (evaluator) { + _evaluator = evaluator; } - _dataToken = index; } diff --git a/roofit/roofitcore/src/RooFit/Evaluator.cxx b/roofit/roofitcore/src/RooFit/Evaluator.cxx index 6a64e94aa9016..ccd423fe188e0 100644 --- a/roofit/roofitcore/src/RooFit/Evaluator.cxx +++ b/roofit/roofitcore/src/RooFit/Evaluator.cxx @@ -190,7 +190,7 @@ Evaluator::Evaluator(const RooAbsReal &absReal, bool useGPU) if (dynamic_cast(arg)) { nodeInfo.isVariable = true; } else { - arg->setDataToken(iNode); + arg->setDataToken(iNode, this); } if (dynamic_cast(arg)) { nodeInfo.isCategory = true; @@ -351,7 +351,8 @@ void Evaluator::updateOutputSizes() Evaluator::~Evaluator() { for (auto &info : _nodes) { - if (!info.isVariable) { + // Nodes that were deleted before the Evaluator were unregistered + if (!info.isVariable && info.absArg) { info.absArg->resetDataToken(); } } @@ -360,6 +361,18 @@ Evaluator::~Evaluator() } } +/// Called from RooAbsArg::~RooAbsArg() when a node of the computation graph is +/// deleted while this Evaluator is still alive. The Evaluator can't be used +/// anymore after that, but its destructor must not touch the deleted node. +/// For internal use only. +void Evaluator::unregisterNode(RooAbsArg const &arg) +{ + auto found = _nodesMap.find(arg.namePtr()); + if (found != _nodesMap.end() && found->second->absArg == &arg) { + found->second->absArg = nullptr; + } +} + void Evaluator::computeCPUNode(const RooAbsArg *node, NodeInfo &info) { using namespace Detail; diff --git a/roofit/roofitcore/test/CMakeLists.txt b/roofit/roofitcore/test/CMakeLists.txt index 21204155cf6e8..0a1268a4d05aa 100644 --- a/roofit/roofitcore/test/CMakeLists.txt +++ b/roofit/roofitcore/test/CMakeLists.txt @@ -84,6 +84,7 @@ endif() ROOT_ADD_GTEST(testNaNPacker testNaNPacker.cxx LIBRARIES RooFitCore) ROOT_ADD_GTEST(testRooBatchCompute testRooBatchCompute.cxx LIBRARIES RooBatchCompute) ROOT_ADD_GTEST(testRooEfficiency testRooEfficiency.cxx LIBRARIES RooFitCore RooFit) +ROOT_ADD_GTEST(testRooEvaluator testRooEvaluator.cxx LIBRARIES RooFitCore RooFit) ROOT_ADD_GTEST(testRooExtendedBinding testRooExtendedBinding.cxx LIBRARIES RooFitCore RooFit) ROOT_ADD_GTEST(testRooMCStudy testRooMCStudy.cxx LIBRARIES RooFitCore RooFit) ROOT_ADD_GTEST(testRooMinimizer testRooMinimizer.cxx LIBRARIES RooFitCore RooFit) diff --git a/roofit/roofitcore/test/testRooEvaluator.cxx b/roofit/roofitcore/test/testRooEvaluator.cxx new file mode 100644 index 0000000000000..84fa3ff904209 --- /dev/null +++ b/roofit/roofitcore/test/testRooEvaluator.cxx @@ -0,0 +1,43 @@ +// Tests for RooFit::Evaluator +// Author: Jonas Rembser, CERN 10/2026 + +#include +#include +#include + +#include + +#include + +/// Deleting nodes of the computation graph before the Evaluator must not +/// crash: the Evaluator used to reset the data tokens of its nodes in the +/// destructor without checking whether the nodes still exist (use after free +/// that was observed as a `free(): invalid pointer` abort at the exit of +/// Python scripts, where the garbage collector can delete the RooWorkspace +/// owning the model before the NLL object). +TEST(RooEvaluator, NodesDeletedBeforeEvaluator) +{ + auto x = std::make_unique("x", "x", 0.0, -5.0, 5.0); + auto mu = std::make_unique("mu", "mu", 0.0, -1.0, 1.0); + auto sigma = std::make_unique("sigma", "sigma", 1.0, 0.1, 10.0); + auto gauss = std::make_unique("gauss", "gauss", *x, *mu, *sigma); + + auto evaluator = std::make_unique(*gauss); + EXPECT_FLOAT_EQ(evaluator->run()[0], gauss->getVal()); + + // Delete the pdf before the Evaluator that holds a data token for it. + gauss.reset(); + evaluator.reset(); + + // A new model with the same node name must get a fresh data token. + auto gauss2 = std::make_unique("gauss", "gauss", *x, *mu, *sigma); + { + RooFit::Evaluator evaluator2{*gauss2}; + EXPECT_FLOAT_EQ(evaluator2.run()[0], gauss2->getVal()); + } + + // The other order: the node outlives the Evaluator. Its data token must + // have been reset such that it can be used by a new Evaluator. + RooFit::Evaluator evaluator3{*gauss2}; + EXPECT_FLOAT_EQ(evaluator3.run()[0], gauss2->getVal()); +} diff --git a/tutorials/roofit/roofit/rf619_discrete_profiling.py b/tutorials/roofit/roofit/rf619_discrete_profiling.py index 1f7c3a5738e55..9a68a351e641e 100644 --- a/tutorials/roofit/roofit/rf619_discrete_profiling.py +++ b/tutorials/roofit/roofit/rf619_discrete_profiling.py @@ -166,11 +166,6 @@ minim.minimize("Minuit2", "Migrad") profileGraph.SetPoint(i, meanVal, nll.getVal()) -# The fit objects are not needed anymore. Delete them already before the -# plotting phase, so that the NLL evaluation machinery is not torn down at -# interpreter shutdown, where the order of cleanups is less controlled. -del minim -del nll c = ROOT.TCanvas("c_rf619", "NLL vs Mean for Different Discrete Combinations", 1200, 400) c.Divide(3, 1) diff --git a/tutorials/roofit/roostats/rs401d_FeldmanCousins.py b/tutorials/roofit/roostats/rs401d_FeldmanCousins.py index 9db04dacfebe5..55a21e81a9b6f 100644 --- a/tutorials/roofit/roostats/rs401d_FeldmanCousins.py +++ b/tutorials/roofit/roostats/rs401d_FeldmanCousins.py @@ -164,12 +164,6 @@ def rs401d_FeldmanCousins(doFeldmanCousins=False, doMCMC=True): dataCanvas.Draw() dataCanvas.SaveAs("3.png") - # The profile likelihood and the NLL are not needed anymore. Delete them - # already here, so that the NLL evaluation machinery is not torn down at - # interpreter shutdown, where the order of cleanups is less controlled. - del pll - del nll - # -------------------------------------------------------------- # show use of Feldman-Cousins utility in RooStats # set the distribution creator, which encodes the test statistic