oneapi-src / oneTBB

oneAPI Threading Building Blocks (oneTBB)
https://oneapi-src.github.io/oneTBB/
Apache License 2.0
5.67k stars 1.02k forks source link

Possible lifetime issue with parallel_scan over non trivially destructible range #1390

Open coo1berg opened 4 months ago

coo1berg commented 4 months ago

Hi,

I've faced odd crash using parallel_scan over range object that has some logic inside its destructor. It seems that destructor of range sometimes called on not yet constructed range object.

It happens not 100% times, but there is a minimal reproducible example I managed to get (ensure asserts are on):

#include <cassert>
#include <vector>

#include <tbb/parallel_reduce.h>
#include <tbb/parallel_scan.h>

int main()
{
    std::vector <int> vec;
    const int len = 34479;

    struct non_trivial_range : tbb::blocked_range <decltype(vec)::iterator>
    {
        using non_trivial_range::blocked_range::blocked_range;

        ~non_trivial_range()
        {
            assert( cookie == 0xc001c001 );
            cookie = 0xdeadbeef;
        }
        unsigned cookie = 0xc001c001;
    };

    vec.assign( len, 1 );

    // this works fine
    int s = tbb::parallel_reduce( non_trivial_range{ vec.begin(), vec.end() }, 0, [&]( const auto & rng, int acc )
    {
        for (int p : rng) acc += p; // work is irrelevant

        return acc;
    }, std::plus<>() );
    assert( s == len );

    // this fails eventually
    for (int i = 0; i < 1000; ++i)
    {
        s = tbb::parallel_scan( non_trivial_range{ vec.begin(), vec.end() }, 0, [&]( const auto & rng, int acc, auto is_final )
        {
            for (int p : rng) acc += p; // work is irrelevant

            return acc;
        }, std::plus<>() );
        assert( s == len );
    }
    return s;
}

tbb.cpp.txt

Tested on:

Is there a problem inside my code?

Regards, Slava

dnmokhov commented 4 months ago

I can reproduce the issue. It does appear that sometimes the right zombie child self destructs without being fully constructed.

coo1berg commented 4 months ago

If that helps, here is a little patch that fixes lifetime problem for me

diff --git a/include/oneapi/tbb/parallel_scan.h b/include/oneapi/tbb/parallel_scan.h
index d624f7eb..b9169b97 100644
--- a/include/oneapi/tbb/parallel_scan.h
+++ b/include/oneapi/tbb/parallel_scan.h
@@ -94,6 +94,7 @@ private:

     wait_context& m_wait_context;
     sum_node_type* m_parent = nullptr;
+    bool m_alive = false;
 public:
     small_object_allocator m_allocator;
     final_sum( Body& body, wait_context& w_o, small_object_allocator& alloc ) :
@@ -107,13 +108,15 @@ public:
     }

     ~final_sum() {
-        m_range.begin()->~Range();
+        if (m_alive)
+            m_range.begin()->~Range();
     }
     void finish_construction( sum_node_type* parent, const Range& range, Body* stuff_last ) {
         __TBB_ASSERT( m_parent == nullptr, nullptr );
         m_parent = parent;
         new( m_range.begin() ) Range(range);
         m_stuff_last = stuff_last;
+        m_alive = true;
     }
 private:
     sum_node_type* release_parent() {

Of course, it doesn't fix the cause of why it wants to self destruct at wrong time.

dnmokhov commented 4 months ago

We are still looking into why the object is left uninitialized. Meanwhile, I hope that you are able to use the workaround (even a simple if (m_parent) check would work).