Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Vectorised RooLandau #1

Closed
wants to merge 26 commits into from

Conversation

manolismih
Copy link

No description provided.

hageboeck and others added 25 commits July 9, 2019 11:54
evaluatePartition() addresses event numbers with integers. It was
changed to use std::size_t. This protects from negative indices, and is
future proof when large numbers of events are available.
o Add a RooSpan that uses a std::span in order to pass around data.
o Change rudimentary batch data interface from std::vectors to spans.
o Clean up iterators in RooVectorDataStore.
o Revert RooNLLVar to single-value computations to have something ready for testing.
o Increment class version of RooVectorDataStore.
Speedup of about 4x achieved, but only ever reads first data column.
- Move functions for finding index of items from RooArgList to RooAbsCollection.
- Fix override/not-yet-implemented warnings in various classes.
- Improve comments.
- Improve readability by using new iterators and correcting indentation.
- Add a RooArgSet to the batch interface that holds variables found in the dataset.
- Using the new BatchHelpers, PDFs can now search for their proxies in the dataset.
- Implement batched/vectorised computations for Gauss and Poisson.
Small cleanups and comments in RooSpan, RooAbsTestStatistic, RooAbsReal.
RooNLLVar now has two modes to switch between scalar and batch+vectorised computation.
The RooNLLVar implementation is not ready, yet. In particular, it's not vectorised.
RooAddPdf allocates its own memory to compute addition of PDFs. The next step
is to take memory in the data store such that computations can be reused.
- Add class BatchData to hold results of batch computations to RooRealVar
- Make batch data accessible through RooRealProxy::getValBatch(begin, end)
- Change batch computation interface in RooAbsPdf, AddPdf, Gauss, Poisson to
getValBatch(begin, end, normSet)
- Make data in RooVectorDataStore accessible to RooRealVar/RooRealProxy
- [Cleanup] Remove traceEvalHook, as nobody uses it
- Update typedefs in std::span backport
- Add conversion constructor from RooSpan<T> to RooSpan<const T>
Replace the existing Kahan summation by a version with multiple accumulators.
This can be auto-vectorised by most compilers when filled from a container with
contiguous memory.
- BatchData: Add possibility to choose whether batches are initialised
- RooAbsReal: Add interface to clear batches between computations
- Adapt already vectorised PDFs to above change
- Implement returning of weight variable in RooVectorDataStore
- Properly enable RooSpan conversion constructor from T to const T
- Add cache value checking for debug builds
- Start refactoring NLL logic into single value and batch branches.
Allows to easily switch between the two.
- [ROOT-9818] Add a fallback implementation for evaluateBatch to
RooAbsReal such that non-batch classes can be used in the same
computation as batched classes.
- Make getValBatch in RooAbsDataStore pure virtual, and place dummy
implementations in derived classes.
- Add code to check batch against scalar computations when compiled in
debug mode.
- Change indexing of batches to a begin+size interface.
- Remove requirement that batches have to be allocated (and their size
given) before the computation. This allows for easier integration with
different data storage backends, which might possibly yield shorter
batches than asked for. This also allows to reduce memory pressure by
not allocating batch storage for every entry in the dataset. Memory can
be recycled for each batch.
- Fix ROOT::span constructor to both take const and non-const vectors.
- Fix override warning in RooDataSet.
- Vectorised the error checking routine in RooAbsPdf, and make sure that errors are communicated
also in batch mode.
- Instead of clearing batches in between NLL runs, only mark them dirty.
- Stabilise error-checking debug routines against inf/nan.
- AbsArgs need a name to be indentified in datasets and PDF trees.
- This commit adds a check such that empty names cannot be used.
@manolismih manolismih marked this pull request as ready for review July 26, 2019 08:05
@hageboeck
Copy link
Owner

Will be re-requested from other branch.

@hageboeck hageboeck closed this Aug 9, 2019
hageboeck pushed a commit that referenced this pull request May 29, 2020
…urrently

in a nutshell, the condition was:
- Thread #1) a task ends and pushes back processing slot
- Thread #2) a task starts and overwrites thread-local TTreeReaderValues
- Thread #1) first task deletes TTreeReader

See root-project@26e8ace
hageboeck pushed a commit that referenced this pull request May 5, 2021
Before, MetaParser might have pointed to a StringRef whose storage
was gone, see asan failure in roottest/cling/other/runfileClose.C below.

This was caused by recursive uses of MetaParser; see stack trace below:
the inner recursion returned, but as the same MetaParser object was used
by both frames, the objects cursor now pointed to freed memory.

Instead, create a MetaParser (and MetaLexer) object per input. That way,
their lifetime corresponds to the lifetime of their input.

=================================================================
==529104==ERROR: AddressSanitizer: stack-use-after-return on address 0x7ffff3afd82a at pc 0x7fffea18df6d bp 0x7fffffff8170 sp 0x7fffffff8168
READ of size 1 at 0x7ffff3afd82a thread T0
[Detaching after fork from child process 529183]
    #0 0x7fffea18df6c in cling::MetaLexer::Lex(cling::Token&) src/interpreter/cling/lib/MetaProcessor/MetaLexer.cpp:58:11
    #1 0x7fffea190d7c in cling::MetaParser::lookAhead(unsigned int) src/interpreter/cling/lib/MetaProcessor/MetaParser.cpp:89:15
    #2 0x7fffea190bd5 in cling::MetaParser::consumeToken() src/interpreter/cling/lib/MetaProcessor/MetaParser.cpp:49:5
    #3 0x7fffea191d4d in cling::MetaParser::isLCommand(cling::MetaSema::ActionResult&) src/interpreter/cling/lib/MetaProcessor/MetaParser.cpp:147:9
    #4 0x7fffea1914dd in cling::MetaParser::isCommand(cling::MetaSema::ActionResult&, cling::Value*) src/interpreter/cling/lib/MetaProcessor/MetaParser.cpp:123:12
    #5 0x7fffea191216 in cling::MetaParser::isMetaCommand(cling::MetaSema::ActionResult&, cling::Value*) src/interpreter/cling/lib/MetaProcessor/MetaParser.cpp:101:33
    #6 0x7fffea14e5aa in cling::MetaProcessor::process(llvm::StringRef, cling::Interpreter::CompilationResult&, cling::Value*, bool) src/interpreter/cling/lib/MetaProcessor/MetaProcessor.cpp:317:24
    #7 0x7fffe99b67b7 in HandleInterpreterException(cling::MetaProcessor*, char const*, cling::Interpreter::CompilationResult&, cling::Value*) src/core/metacling/src/TCling.cxx:2431:29
    #8 0x7fffe99bde30 in TCling::Load(char const*, bool) src/core/metacling/src/TCling.cxx:3454:10
    #9 0x7ffff7865f11 in TSystem::Load(char const*, char const*, bool) src/core/base/src/TSystem.cxx:1941:27
    #10 0x7ffff7b8a0e3 in TUnixSystem::Load(char const*, char const*, bool) src/core/unix/src/TUnixSystem.cxx:2789:20
    #11 0x7fffd78dd08b  (<unknown module>)
    #12 0x7fffe9f8a5d9 in cling::IncrementalExecutor::executeWrapper(llvm::StringRef, cling::Value*) const src/interpreter/cling/lib/Interpreter/IncrementalExecutor.cpp:376:3
    #13 0x7fffe9d73dc2 in cling::Interpreter::RunFunction(clang::FunctionDecl const*, cling::Value*) src/interpreter/cling/lib/Interpreter/Interpreter.cpp:1141:20
    root-project#14 0x7fffe9d6e317 in cling::Interpreter::EvaluateInternal(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, cling::CompilationOptions, cling::Value*, cling::Transaction**, unsigned long) src/interpreter/cling/lib/Interpreter/Interpreter.cpp:1391:29
    root-project#15 0x7fffe9d6c1fe in cling::Interpreter::process(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, cling::Value*, cling::Transaction**, bool) src/interpreter/cling/lib/Interpreter/Interpreter.cpp:819:9
    root-project#16 0x7fffea151826 in cling::MetaProcessor::readInputFromFile(llvm::StringRef, cling::Value*, unsigned long, bool) src/interpreter/cling/lib/MetaProcessor/MetaProcessor.cpp:507:22
    root-project#17 0x7fffe99b585b in TCling::ProcessLine(char const*, TInterpreter::EErrorCode*) src/core/metacling/src/TCling.cxx:2570:39
    root-project#18 0x7fffe99bbfee in TCling::ProcessLineSynch(char const*, TInterpreter::EErrorCode*) src/core/metacling/src/TCling.cxx:3496:17
    root-project#19 0x7ffff77203d3 in TApplication::ExecuteFile(char const*, int*, bool) src/core/base/src/TApplication.cxx:1608:30
    root-project#20 0x7ffff771ebdf in TApplication::ProcessFile(char const*, int*, bool) src/core/base/src/TApplication.cxx:1480:11
    root-project#21 0x7ffff771e385 in TApplication::ProcessLine(char const*, bool, int*) src/core/base/src/TApplication.cxx:1453:14
    root-project#22 0x7ffff7f8157a in TRint::ProcessLineNr(char const*, char const*, int*) src/core/rint/src/TRint.cxx:766:11
    root-project#23 0x7ffff7f802f0 in TRint::Run(bool) src/core/rint/src/TRint.cxx:424:22
    root-project#24 0x4ff96d in main src/main/src/rmain.cxx:30:12
    root-project#25 0x7ffff6e040b2 in __libc_start_main /build/glibc-YbNSs7/glibc-2.31/csu/../csu/libc-start.c:308:16
    root-project#26 0x41f35d in _start (asan/bin/root.exe+0x41f35d)

Address 0x7ffff3afd82a is located in stack of thread T0 at offset 42 in frame
    #0 0x7fffe99b3d8f in TCling::ProcessLine(char const*, TInterpreter::EErrorCode*) src/core/metacling/src/TCling.cxx:2456

  This frame has 21 object(s):
    [32, 56) 'sLine' (line 2462) <== Memory access at offset 42 is inside this variable
    [96, 104) 'R__guard2471' (line 2471)
    [128, 136) 'R__guard2488' (line 2488)
    [160, 176) 'interpreterFlagsRAII' (line 2491)
    [192, 240) 'result' (line 2511)
    [272, 276) 'compRes' (line 2512)
    [288, 312) 'mod_line' (line 2517)
    [352, 376) 'aclicMode' (line 2518)
    [416, 440) 'arguments' (line 2519)
    [480, 504) 'io' (line 2520)
    [544, 568) 'fname' (line 2521)
    [608, 632) 'ref.tmp' (line 2547)
    [672, 696) 'ref.tmp145' (line 2547)
    [736, 768) 'code' (line 2555)
    [800, 832) 'codeline' (line 2556)
    [864, 1384) 'in' (line 2559)
    [1520, 1552) 'ref.tmp176' (line 2562)
    [1584, 1600) 'agg.tmp'
    [1616, 1624) 'ref.tmp198' (line 2568)
    [1648, 1664) 'agg.tmp207'
    [1680, 1696) 'autoParseRaii' (line 2588)
HINT: this may be a false positive if your program uses some custom stack unwind mechanism, swapcontext or vfork
      (longjmp and C++ exceptions *are* supported)
SUMMARY: AddressSanitizer: stack-use-after-return src/interpreter/cling/lib/MetaProcessor/MetaLexer.cpp:58:11 in cling::MetaLexer::Lex(cling::Token&)
Shadow bytes around the buggy address:
  0x10007e757ab0: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
  0x10007e757ac0: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
  0x10007e757ad0: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
  0x10007e757ae0: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
  0x10007e757af0: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
=>0x10007e757b00: f5 f5 f5 f5 f5[f5]f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
  0x10007e757b10: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
  0x10007e757b20: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
  0x10007e757b30: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
  0x10007e757b40: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
  0x10007e757b50: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
  Shadow gap:              cc
==529104==ABORTING

    at src/interpreter/cling/lib/MetaProcessor/MetaLexer.cpp:49
    at src/interpreter/cling/lib/MetaProcessor/MetaParser.cpp:41
    compRes=@0x7ffff3afd910: cling::Interpreter::kSuccess, result=0x7ffff3afd8c0, disableValuePrinting=false)
    at src/interpreter/cling/lib/MetaProcessor/MetaProcessor.cpp:314
    input_line=0x7ffff3afd829 "#define XYZ 21", compRes=@0x7ffff3afd910: cling::Interpreter::kSuccess,
    result=0x7ffff3afd8c0) at src/core/metacling/src/TCling.cxx:2431
    error=0x7fffd78cb0f4 <x>) at src/core/metacling/src/TCling.cxx:2591
    sync=false, err=0x7fffd78cb0f4 <x>) at src/core/base/src/TApplication.cxx:1472
    line=0x7fffd78c9000 "#define XYZ 21", error=0x7fffd78cb0f4 <x>)
    at src/core/base/src/TROOT.cxx:2328
   from asan/roottest/cling/other/fileClose_C.so
    filename=0x6070000f0fd0 "asan/roottest/cling/other/fileClose_C.so", flag=257)
    at /home/axel/build/llvm/llvm-project/compiler-rt/lib/asan/../sanitizer_common/sanitizer_common_interceptors.inc:6270
    at src/interpreter/cling/lib/Utils/PlatformPosix.cpp:118
    permanent=false, resolved=true)
    at src/interpreter/cling/lib/Interpreter/DynamicLibraryManager.cpp:184
    at src/interpreter/cling/lib/Interpreter/Interpreter.cpp:1444
    T=0x0) at src/interpreter/cling/lib/Interpreter/Interpreter.cpp:1560
    at src/interpreter/cling/lib/MetaProcessor/MetaSema.cpp:57
    actionResult=@0x7ffff39532b0: cling::MetaSema::AR_Success)
hageboeck pushed a commit that referenced this pull request Jul 14, 2021
This is to avoid output that doesn't indicate any problem and only makes
the test output harder to interpret.

That's how the output of test 27 was before this commit:
```
[#1] INFO:NumericIntegration -- RooRealIntegral::init(pxyz_Int[z|R]_Norm[x,y,z]_Int[y|R]) using numeric integrator RooIntegrator1D to calculate Int(y)
Test 27 : Integration over non-rectangular regions...............OK
```
hageboeck pushed a commit that referenced this pull request Dec 12, 2022
Fixes root-meta-execUnloading which shows
```==768591== Invalid read of size 8
==768591==    at 0x6EFB10E: llvm::ilist_node_base<false>::getNext() const (ilist_node_base.h:29)
==768591==    by 0x6FBF119: llvm::ilist_node_impl<llvm::ilist_detail::node_options<llvm::Function, false, false, void> >::getNext() (ilist_node.h:66)
==768591==    by 0x6FBED48: llvm::ilist_iterator<llvm::ilist_detail::node_options<llvm::Function, false, false, void>, false, false>::operator++() (ilist_iterator.h:157)
==768591==    by 0x6FBEA20: llvm::simple_ilist<llvm::Function>::begin() (simple_ilist.h:117)
==768591==    by 0x6FBE40F: llvm::Module::begin() (Module.h:622)
==768591==    by 0x6FBE475: llvm::Module::functions() (Module.h:634)
==768591==    by 0x6FBE22F: cling::TransactionUnloader::unloadModule(llvm::Module*) (TransactionUnloader.cpp:175)
==768591==    by 0x6FBDF83: cling::TransactionUnloader::RevertTransaction(cling::Transaction*) (TransactionUnloader.cpp:126)
==768591==    by 0x6F90CD0: cling::Interpreter::unload(cling::Transaction&) (Interpreter.cpp:1553)
==768591==    by 0x6F90E25: cling::Interpreter::unload(unsigned int) (Interpreter.cpp:1574)
==768591==    by 0x721A9F2: cling::MetaSema::actOnUndoCommand(unsigned int) (MetaSema.cpp:186)
```
...
```
==768591==  Address 0x144b0ce0 is 32 bytes inside a block of size 800 free'd
==768591==    at 0x484399B: operator delete(void*, unsigned long) (vg_replace_malloc.c:935)
==768591==    by 0x6FBC7CA: std::default_delete<llvm::Module>::operator()(llvm::Module*) const (unique_ptr.h:95)
==768591==    by 0x6FBC82D: std::__uniq_ptr_impl<llvm::Module, std::default_delete<llvm::Module> >::reset(llvm::Module*) (unique_ptr.h:203)
==768591==    by 0x6FBC87E: std::unique_ptr<llvm::Module, std::default_delete<llvm::Module> >::reset(llvm::Module*) (unique_ptr.h:501)
==768591==    by 0x6FBC04E: std::unique_ptr<llvm::Module, std::default_delete<llvm::Module> >::operator=(decltype(nullptr)) (unique_ptr.h:434)
==768591==    by 0x70E60E7: llvm::orc::ThreadSafeModule::~ThreadSafeModule() (ThreadSafeModule.h:116)
==768591==    by 0x936EC79: llvm::orc::IRMaterializationUnit::~IRMaterializationUnit() (Layer.h:31)
==768591==    by 0x93707F5: llvm::orc::BasicIRLayerMaterializationUnit::~BasicIRLayerMaterializationUnit() (Layer.h:121)
==768591==    by 0x9370811: llvm::orc::BasicIRLayerMaterializationUnit::~BasicIRLayerMaterializationUnit() (Layer.h:121)
==768591==    by 0x6E91B45: std::default_delete<llvm::orc::MaterializationUnit>::operator()(llvm::orc::MaterializationUnit*) const (unique_ptr.h:95)
==768591==    by 0x6E8F9A3: std::unique_ptr<llvm::orc::MaterializationUnit, std::default_delete<llvm::orc::MaterializationUnit> >::~unique_ptr() (unique_ptr.h:396)
==768591==    by 0x935D3C3: llvm::orc::JITDylib::UnmaterializedInfo::~UnmaterializedInfo() (Core.h:1062)
==768591==    by 0x935D3DE: void std::_Destroy<llvm::orc::JITDylib::UnmaterializedInfo>(llvm::orc::JITDylib::UnmaterializedInfo*) (stl_construct.h:151)
==768591==    by 0x935D26B: void std::allocator_traits<std::allocator<void> >::destroy<llvm::orc::JITDylib::UnmaterializedInfo>(std::allocator<void>&, llvm::orc::JITDylib::UnmaterializedInfo*) (alloc_traits.h:648)
==768591==    by 0x935CBA0: std::_Sp_counted_ptr_inplace<llvm::orc::JITDylib::UnmaterializedInfo, std::allocator<void>, (__gnu_cxx::_Lock_policy)2>::_M_dispose() (shared_ptr_base.h:613)
==768591==    by 0x6C39FCE: std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release() (shared_ptr_base.h:346)
==768591==    by 0x6C3CB36: std::__shared_count<(__gnu_cxx::_Lock_policy)2>::~__shared_count() (shared_ptr_base.h:1071)
==768591==    by 0x6E98CED: std::__shared_ptr<llvm::orc::JITDylib::UnmaterializedInfo, (__gnu_cxx::_Lock_policy)2>::~__shared_ptr() (shared_ptr_base.h:1524)
==768591==    by 0x6E98D09: std::shared_ptr<llvm::orc::JITDylib::UnmaterializedInfo>::~shared_ptr() (shared_ptr.h:175)
==768591==    by 0x932D614: llvm::DenseMapBase<llvm::DenseMap<llvm::orc::SymbolStringPtr, std::shared_ptr<llvm::orc::JITDylib::UnmaterializedInfo>, llvm::DenseMapInfo<llvm::orc::SymbolStringPtr>, llvm::detail::DenseMapPair<llvm::orc::SymbolStringPtr, std::shared_ptr<llvm::orc::JITDylib::UnmaterializedInfo> > >, llvm::orc::SymbolStringPtr, std::shared_ptr<llvm::orc::JITDylib::UnmaterializedInfo>, llvm::DenseMapInfo<llvm::orc::SymbolStringPtr>, llvm::detail::DenseMapPair<llvm::orc::SymbolStringPtr, std::shared_ptr<llvm::orc::JITDylib::UnmaterializedInfo> > >::erase(llvm::orc::SymbolStringPtr const&) (DenseMap.h:307)
==768591==    by 0x931028D: llvm::orc::JITDylib::removeTracker(llvm::orc::ResourceTracker&) (Core.cpp:1510)
==768591==    by 0x9315060: llvm::orc::ExecutionSession::removeResourceTracker(llvm::orc::ResourceTracker&)::{lambda()#1}::operator()() const (Core.cpp:2155)
==768591==    by 0x93150D0: decltype(auto) llvm::orc::ExecutionSession::runSessionLocked<llvm::orc::ExecutionSession::removeResourceTracker(llvm::orc::ResourceTracker&)::{lambda()#1}>(llvm::orc::ExecutionSession::removeResourceTracker(llvm::orc::ResourceTracker&)::{lambda()#1}&&) (Core.h:1326)
==768591==    by 0x9315272: llvm::orc::ExecutionSession::removeResourceTracker(llvm::orc::ResourceTracker&) (Core.cpp:2152)
==768591==    by 0x93060FD: llvm::orc::ResourceTracker::remove() (Core.cpp:53)
==768591==    by 0x70F1D58: cling::IncrementalJIT::removeModule(cling::Transaction const&) (IncrementalJIT.cpp:151)
==768591==    by 0x6FBE6CF: cling::IncrementalExecutor::unloadModule(cling::Transaction const&) const (IncrementalExecutor.h:180)
==768591==    by 0x6FBDE5B: cling::TransactionUnloader::RevertTransaction(cling::Transaction*) (TransactionUnloader.cpp:119)
==768591==    by 0x6F90CD0: cling::Interpreter::unload(cling::Transaction&) (Interpreter.cpp:1553)
==768591==    by 0x6F90E25: cling::Interpreter::unload(unsigned int) (Interpreter.cpp:1574)
==768591==    by 0x721A9F2: cling::MetaSema::actOnUndoCommand(unsigned int) (MetaSema.cpp:186)
```
hageboeck pushed a commit that referenced this pull request Dec 15, 2023
[ RUN      ] RooTruthModel.IntegrateSubrange
[#1] INFO:Eval -- RooRealVar::setRange(dt) new range named 'integral' created with bounds [2,2]
/builddir/build/BUILD/root-6.30.02/roofit/roofitcore/test/testRooTruthModel.cxx:38: Failure
Expected equality of these values:
  integ->getVal()
    Which is: -2.8822093e-17
  0.0
    Which is: 0
[  FAILED  ] RooTruthModel.IntegrateSubrange (194 ms)
guitargeek pushed a commit that referenced this pull request May 21, 2024
In the jitted version of the `Vary` transformation a node is allocated on the heap and its address is passed down to the function `JitVariationHelper`, which is also responsible for deleting the allocated memory. In case a mismatch in the return type of the jitted function given to the Vary call is detected, we throw an error to inform the user they should return an RVec for the Vary to properly work. This means that the call to JitVariationHelper does not happen, thus the memory of the node is not deallocated. This commit corrects that behaviour by properly deleting the pointer before throwing the exception.

Thanks to the address sanitizer:
```
Direct leak of 16 byte(s) in 1 object(s) allocated from:
    #0 0x7f28c78d9e28 in operator new(unsigned long) (/lib64/libasan.so.8+0xd9e28) (BuildId: 2b657470ea196ba4342e3bd8a3cc138b1e200599)
    #1 0xb711e0 in std::shared_ptr<ROOT::Detail::RDF::RNodeBase>* ROOT::Internal::RDF::MakeSharedOnHeap<ROOT::Detail::RDF::RNodeBase>(std::shared_ptr<ROOT::Detail::RDF::RNodeBase> const&) /home/vpadulan/Programs/rootproject/rootbuild/master-a73f11dfc5-testing-asan/include/ROOT/RDF/InterfaceUtils.hxx:370
    #2 0xb843a8 in ROOT::RDF::RInterface<ROOT::Detail::RDF::RLoopManager, void>::JittedVaryImpl(std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > const&, std::basic_string_view<char, std::char_traits<char> >, std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > const&, std::basic_string_view<char, std::char_traits<char> >, bool) /home/vpadulan/Programs/rootproject/rootbuild/master-a73f11dfc5-testing-asan/include/ROOT/RDF/RInterface.hxx:3108
```
hageboeck pushed a commit that referenced this pull request Sep 18, 2024
The destructor of TTreeReader was making use of the TFile after its destruction.

```
480: ==2560128==ERROR: AddressSanitizer: heap-use-after-free on address 0x617000011648 at pc 0x7fb9d1ae089e bp 0x7ffd8e35b0a0 sp 0x7ffd8e35b098
480: READ of size 8 at 0x617000011648 thread T0
480:     #0 0x7fb9d1ae089d in TTree::GetNotify() const /home/vpadulan/Programs/rootproject/rootsrc/tree/tree/inc/TTree.h:503
480:     #1 0x7fb9d1ae089d in void TNotifyLinkBase::RemoveLink<TTree>(TTree&) /home/vpadulan/Programs/rootproject/rootsrc/core/base/inc/TNotifyLin
k.h:104
480:     #2 0x7fb9d1ae089d in TTreeReader::~TTreeReader() /home/vpadulan/Programs/rootproject/rootsrc/tree/treeplayer/src/TTreeReader.cxx:252
480:     #3 0x433968 in BulkApiSillyStructTest_stdReadSplitBranch_Test::TestBody() /home/vpadulan/Programs/rootproject/rootsrc/tree/tree/test/Bulk
ApiSillyStruct.cxx:104
480:     root-project#4 0x470bac in void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)()
, char const*) (/home/vpadulan/Programs/rootproject/rootbuild/bulksilly-heap-use-after-free-testing-asan/tree/tree/test/testBulkApiSillyStruct+0x4
70bac) (BuildId: 45d8d15c24ba2aae252398ec00943aba7665b034)
480:     root-project#5 0x45a5f3 in testing::Test::Run() [clone .part.0] (/home/vpadulan/Programs/rootproject/rootbuild/bulksilly-heap-use-after-free-testing-
asan/tree/tree/test/testBulkApiSillyStruct+0x45a5f3) (BuildId: 45d8d15c24ba2aae252398ec00943aba7665b034)
480:     root-project#6 0x45a969 in testing::TestInfo::Run() (/home/vpadulan/Programs/rootproject/rootbuild/bulksilly-heap-use-after-free-testing-asan/tree/tr
ee/test/testBulkApiSillyStruct+0x45a969) (BuildId: 45d8d15c24ba2aae252398ec00943aba7665b034)
480:     root-project#7 0x45ab10 in testing::TestSuite::Run() [clone .part.0] (/home/vpadulan/Programs/rootproject/rootbuild/bulksilly-heap-use-after-free-tes
ting-asan/tree/tree/test/testBulkApiSillyStruct+0x45ab10) (BuildId: 45d8d15c24ba2aae252398ec00943aba7665b034)
480:     root-project#8 0x4675be in testing::internal::UnitTestImpl::RunAllTests() (/home/vpadulan/Programs/rootproject/rootbuild/bulksilly-heap-use-after-fre
e-testing-asan/tree/tree/test/testBulkApiSillyStruct+0x4675be) (BuildId: 45d8d15c24ba2aae252398ec00943aba7665b034)
480:     root-project#9 0x45af6c in testing::UnitTest::Run() (/home/vpadulan/Programs/rootproject/rootbuild/bulksilly-heap-use-after-free-testing-asan/tree/tr
ee/test/testBulkApiSillyStruct+0x45af6c) (BuildId: 45d8d15c24ba2aae252398ec00943aba7665b034)
480:     root-project#10 0x425456 in main (/home/vpadulan/Programs/rootproject/rootbuild/bulksilly-heap-use-after-free-testing-asan/tree/tree/test/testBulkApi
SillyStruct+0x425456) (BuildId: 45d8d15c24ba2aae252398ec00943aba7665b034)
480:     root-project#11 0x7fb9cd446149 in __libc_start_call_main (/lib64/libc.so.6+0x28149) (BuildId: 0d710e9d9dc10c500b8119c85da75004183618e2)
480:     root-project#12 0x7fb9cd44620a in __libc_start_main_impl (/lib64/libc.so.6+0x2820a) (BuildId: 0d710e9d9dc10c500b8119c85da75004183618e2)
480:     root-project#13 0x4257b4 in _start (/home/vpadulan/Programs/rootproject/rootbuild/bulksilly-heap-use-after-free-testing-asan/tree/tree/test/testBulkA
piSillyStruct+0x4257b4) (BuildId: 45d8d15c24ba2aae252398ec00943aba7665b034)

480: 0x617000011648 is located 328 bytes inside of 712-byte region [0x617000011500,0x6170000117c8)
480: freed by thread T0 here:
480:     #0 0x7fb9d1eda878 in operator delete(void*) (/lib64/libasan.so.8+0xda878) (BuildId: 2e1c50524ff1a2e7e73c4565b46f3f51892353ea)
480:     #1 0x7fb9cdfb4f25 in TCollection::GarbageCollect(TObject*) /home/vpadulan/Programs/rootproject/rootsrc/core/cont/src/TCollection.cxx:736
480:     #2 0x7fb9cdfe8a27 in TList::Delete(char const*) /home/vpadulan/Programs/rootproject/rootsrc/core/cont/src/TList.cxx:535
480:     #3 0x7fb9cdfc53d7 in THashList::Delete(char const*) /home/vpadulan/Programs/rootproject/rootsrc/core/cont/src/THashList.cxx:215
480:     root-project#4 0x7fb9ce8d285d in TDirectoryFile::Close(char const*) /home/vpadulan/Programs/rootproject/rootsrc/io/io/src/TDirectoryFile.cxx:585
480:     root-project#5 0x7fb9ce8d285d in TDirectoryFile::Close(char const*) /home/vpadulan/Programs/rootproject/rootsrc/io/io/src/TDirectoryFile.cxx:561
480:     root-project#6 0x7fb9ce9468e4 in TFile::Close(char const*) /home/vpadulan/Programs/rootproject/rootsrc/io/io/src/TFile.cxx:989
480:     root-project#7 0x7fb9ce9481fd in TFile::~TFile() /home/vpadulan/Programs/rootproject/rootsrc/io/io/src/TFile.cxx:566
480:     root-project#8 0x7fb9ce948fd0 in TFile::~TFile() /home/vpadulan/Programs/rootproject/rootsrc/io/io/src/TFile.cxx:603
480:     root-project#9 0x4338cd in BulkApiSillyStructTest_stdReadSplitBranch_Test::TestBody() /home/vpadulan/Programs/rootproject/rootsrc/tree/tree/test/Bulk
ApiSillyStruct.cxx:103
480:     root-project#10 0x470bac in void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(
), char const*) (/home/vpadulan/Programs/rootproject/rootbuild/bulksilly-heap-use-after-free-testing-asan/tree/tree/test/testBulkApiSillyStruct+0x
470bac) (BuildId: 45d8d15c24ba2aae252398ec00943aba7665b034)

480: previously allocated by thread T0 here:
480:     #0 0x7fb9d1ed9e38 in operator new(unsigned long) (/lib64/libasan.so.8+0xd9e38) (BuildId: 2e1c50524ff1a2e7e73c4565b46f3f51892353ea)
480:     #1 0x7fb9cdebbfd0 in TStorage::ObjectAlloc(unsigned long) /home/vpadulan/Programs/rootproject/rootsrc/core/base/src/TStorage.cxx:293
480:     #2 0x7fb9d12cc209 in TObject::operator new(unsigned long) /home/vpadulan/Programs/rootproject/rootbuild/bulksilly-heap-use-after-free-tes
ting-asan/include/TObject.h:181
480:     #3 0x7fb9d12cc209 in new_TTree /home/vpadulan/Programs/rootproject/rootbuild/bulksilly-heap-use-after-free-testing-asan/tree/tree/G__Tree
.cxx:4363
480:     root-project#4 0x7fb9ce10a49c in TClass::NewObject(TClass::ENewType, bool) const /home/vpadulan/Programs/rootproject/rootsrc/core/meta/src/TClass.cxx
:5003
480:     root-project#5 0x7fb9ce113c7d in TClass::New(TClass::ENewType, bool) const /home/vpadulan/Programs/rootproject/rootsrc/core/meta/src/TClass.cxx:4980
480:     root-project#6 0x7fb9ce9cda38 in TKey::ReadObjectAny(TClass const*) /home/vpadulan/Programs/rootproject/rootsrc/io/io/src/TKey.cxx:1086
480:     root-project#7 0x7fb9ce8d10a7 in TDirectoryFile::GetObjectChecked(char const*, TClass const*) /home/vpadulan/Programs/rootproject/rootsrc/io/io/src/T
DirectoryFile.cxx:1111
480:     root-project#8 0x7fb9d1ade1d1 in void TDirectory::GetObject<TTree>(char const*, TTree*&) /home/vpadulan/Programs/rootproject/rootsrc/core/base/inc/TD
irectory.h:214
480:     root-project#9 0x7fb9d1ade1d1 in TTreeReader::TTreeReader(char const*, TDirectory*, TEntryList*) /home/vpadulan/Programs/rootproject/rootsrc/tree/tre
eplayer/src/TTreeReader.cxx:232
480:     root-project#10 0x433332 in BulkApiSillyStructTest_stdReadSplitBranch_Test::TestBody() /home/vpadulan/Programs/rootproject/rootsrc/tree/tree/test/Bu$
kApiSillyStruct.cxx:87
480:     root-project#11 0x470bac in void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(
), char const*) (/home/vpadulan/Programs/rootproject/rootbuild/bulksilly-heap-use-after-free-testing-asan/tree/tree/test/testBulkApiSillyStruct+0x
470bac) (BuildId: 45d8d15c24ba2aae252398ec00943aba7665b034)
480:
480: SUMMARY: AddressSanitizer: heap-use-after-free /home/vpadulan/Programs/rootproject/rootsrc/tree/tree/inc/TTree.h:503 in TTree::GetNotify() co
nst
```
hageboeck pushed a commit that referenced this pull request Sep 18, 2024
Ensure proper TFile memory management and avoid TTreeReader accessing the TFile after its destruction.

```
479: ==2573107==ERROR: AddressSanitizer: heap-use-after-free on address 0x617000051b48 at pc 0x7f0fcf4e089e bp 0x7fff6e7e1fe0 sp 0x7fff6e7e1fd8
479: READ of size 8 at 0x617000051b48 thread T0
479:     #0 0x7f0fcf4e089d in TTree::GetNotify() const /home/vpadulan/Programs/rootproject/rootsrc/tree/tree/inc/TTree.h:503
479:     #1 0x7f0fcf4e089d in void TNotifyLinkBase::RemoveLink<TTree>(TTree&) /home/vpadulan/Programs/rootproject/rootsrc/core/base/inc/TNotifyLin
k.h:104
479:     #2 0x7f0fcf4e089d in TTreeReader::~TTreeReader() /home/vpadulan/Programs/rootproject/rootsrc/tree/treeplayer/src/TTreeReader.cxx:252
479:     #3 0x4321ca in BulkApiVariableTest_stdRead_Test::TestBody() /home/vpadulan/Programs/rootproject/rootsrc/tree/tree/test/BulkApiVarLength.c
xx:135
479:     root-project#4 0x470c8c in void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)()
, char const*) (/home/vpadulan/Programs/rootproject/rootbuild/bulksilly-heap-use-after-free-testing-asan/tree/tree/test/testBulkApiVarLength+0x470
c8c) (BuildId: aac947b72f02e5567382f0dadfefd1e97d058a56)
479:     root-project#5 0x45a6d3 in testing::Test::Run() [clone .part.0] (/home/vpadulan/Programs/rootproject/rootbuild/bulksilly-heap-use-after-free-testing-
asan/tree/tree/test/testBulkApiVarLength+0x45a6d3) (BuildId: aac947b72f02e5567382f0dadfefd1e97d058a56)
479:     root-project#6 0x45aa49 in testing::TestInfo::Run() (/home/vpadulan/Programs/rootproject/rootbuild/bulksilly-heap-use-after-free-testing-asan/tree/tr
ee/test/testBulkApiVarLength+0x45aa49) (BuildId: aac947b72f02e5567382f0dadfefd1e97d058a56)
479:     root-project#7 0x45abf0 in testing::TestSuite::Run() [clone .part.0] (/home/vpadulan/Programs/rootproject/rootbuild/bulksilly-heap-use-after-free-tes
ting-asan/tree/tree/test/testBulkApiVarLength+0x45abf0) (BuildId: aac947b72f02e5567382f0dadfefd1e97d058a56)
479:     root-project#8 0x46769e in testing::internal::UnitTestImpl::RunAllTests() (/home/vpadulan/Programs/rootproject/rootbuild/bulksilly-heap-use-after-fre
e-testing-asan/tree/tree/test/testBulkApiVarLength+0x46769e) (BuildId: aac947b72f02e5567382f0dadfefd1e97d058a56)
479:     root-project#9 0x45b04c in testing::UnitTest::Run() (/home/vpadulan/Programs/rootproject/rootbuild/bulksilly-heap-use-after-free-testing-asan/tree/tr
ee/test/testBulkApiVarLength+0x45b04c) (BuildId: aac947b72f02e5567382f0dadfefd1e97d058a56)
479:     root-project#10 0x424606 in main (/home/vpadulan/Programs/rootproject/rootbuild/bulksilly-heap-use-after-free-testing-asan/tree/tree/test/testBulkApi
VarLength+0x424606) (BuildId: aac947b72f02e5567382f0dadfefd1e97d058a56)
479: 0x617000051b48 is located 328 bytes inside of 712-byte region [0x617000051a00,0x617000051cc8)

479: freed by thread T0 here:
479:     #0 0x7f0fcf8da878 in operator delete(void*) (/lib64/libasan.so.8+0xda878) (BuildId: 2e1c50524ff1a2e7e73c4565b46f3f51892353ea)
479:     #1 0x7f0fcb9b4f25 in TCollection::GarbageCollect(TObject*) /home/vpadulan/Programs/rootproject/rootsrc/core/cont/src/TCollection.cxx:736
479:     #2 0x7f0fcb9e8a27 in TList::Delete(char const*) /home/vpadulan/Programs/rootproject/rootsrc/core/cont/src/TList.cxx:535
479:     #3 0x7f0fcb9c53d7 in THashList::Delete(char const*) /home/vpadulan/Programs/rootproject/rootsrc/core/cont/src/THashList.cxx:215
479:     root-project#4 0x7f0fcc2d285d in TDirectoryFile::Close(char const*) /home/vpadulan/Programs/rootproject/rootsrc/io/io/src/TDirectoryFile.cxx:585
479:     root-project#5 0x7f0fcc2d285d in TDirectoryFile::Close(char const*) /home/vpadulan/Programs/rootproject/rootsrc/io/io/src/TDirectoryFile.cxx:561
479:     root-project#6 0x7f0fcc3468e4 in TFile::Close(char const*) /home/vpadulan/Programs/rootproject/rootsrc/io/io/src/TFile.cxx:989
479:     root-project#7 0x7f0fcc3481fd in TFile::~TFile() /home/vpadulan/Programs/rootproject/rootsrc/io/io/src/TFile.cxx:566
479:     root-project#8 0x7f0fcc348fd0 in TFile::~TFile() /home/vpadulan/Programs/rootproject/rootsrc/io/io/src/TFile.cxx:603
479:     root-project#9 0x432ebf in BulkApiVariableTest_stdRead_Test::TestBody() /home/vpadulan/Programs/rootproject/rootsrc/tree/tree/test/BulkApiVarLength.c
xx:130
479:     root-project#10 0x470c8c in void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(
), char const*) (/home/vpadulan/Programs/rootproject/rootbuild/bulksilly-heap-use-after-free-testing-asan/tree/tree/test/testBulkApiVarLength+0x47
0c8c) (BuildId: aac947b72f02e5567382f0dadfefd1e97d058a56)
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants