root icon indicating copy to clipboard operation
root copied to clipboard

[df] Delete allocated node before throwing error

Open vepadulano opened this issue 1 year ago • 1 comments

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

This Pull request:

Changes or fixes:

Checklist:

  • [ ] tested changes locally
  • [ ] updated the docs (if necessary)

This PR fixes #

vepadulano avatar May 10 '24 13:05 vepadulano

Test Results

    10 files      10 suites   1d 22h 14m 41s :stopwatch:  2 635 tests  2 635 :white_check_mark: 0 :zzz: 0 :x: 24 868 runs  24 868 :white_check_mark: 0 :zzz: 0 :x:

Results for commit 80ce1948.

:recycle: This comment has been updated with latest results.

github-actions[bot] avatar May 10 '24 14:05 github-actions[bot]

I'm not 100% sure, but may be related to the general issue with jitted code and errors at https://github.com/root-project/root/issues/15076 ?

eguiraud avatar May 15 '24 13:05 eguiraud

I'm not 100% sure, but may be related to the general issue with jitted code and errors at https://github.com/root-project/root/issues/15076 ?

Ah thanks for pointing this out! It skipped my attention and I recently created exactly the same bug report which is already closed with a fix by https://github.com/root-project/root/pull/15400 so I guess we can close that one and open a new issue to remind ourselves to investigate a better approach for the JIT code.

vepadulano avatar May 15 '24 13:05 vepadulano