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 ab6a659165f71..52ed6596af55c 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; } @@ -634,8 +635,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 @@ -660,6 +665,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 ab4297ad9cad2..0d7ecbb6dc0c8 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 @@ -169,6 +170,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); @@ -2261,17 +2268,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 06ced37a724a4..d8cefc7db1a00 100644 --- a/roofit/roofitcore/src/RooFit/Evaluator.cxx +++ b/roofit/roofitcore/src/RooFit/Evaluator.cxx @@ -189,7 +189,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; @@ -338,12 +338,25 @@ 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(); } } } +/// 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 def1fa324c837..c81bfb6fe11a5 100644 --- a/roofit/roofitcore/test/CMakeLists.txt +++ b/roofit/roofitcore/test/CMakeLists.txt @@ -81,6 +81,7 @@ if(clad) ROOT_ADD_GTEST(testRooFuncWrapper testRooFuncWrapper.cxx LIBRARIES RooFitCore RooFit HistFactory) endif() ROOT_ADD_GTEST(testNaNPacker testNaNPacker.cxx LIBRARIES RooFitCore) +ROOT_ADD_GTEST(testRooEvaluator testRooEvaluator.cxx LIBRARIES RooFitCore RooFit) ROOT_ADD_GTEST(testRooExtendedBinding testRooExtendedBinding.cxx LIBRARIES RooFitCore RooFit) ROOT_ADD_GTEST(testRooMinimizer testRooMinimizer.cxx LIBRARIES RooFitCore RooFit) ROOT_ADD_GTEST(testRooMulti testRooMulti.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()); +}