Weird behaviour changing captured atomic bool

Question

I am doing something like this with TBB:

#include <tbb/tbb.h>
#include <memory>
#include <atomic>

class base_creator
{
public:
    using node = tbb::flow::input_node<bool>;

    virtual ~base_creator() = default;
    base_creator()
    {
        m_kill = std::make_shared<std::atomic_bool>(false);
    };

    static tbb::flow::graph& g()
    {
        static tbb::flow::graph me_g;
        return me_g;
    };

    virtual std::shared_ptr<node> get_node() const = 0;

    template<typename Op>
    static tbb::flow::input_node<bool> build_node(const Op& op)
    {
        return tbb::flow::input_node<bool>(base_creator::g(), op);
    };

protected:
    mutable std::shared_ptr<std::atomic_bool> m_kill;
};

class creater : public base_creator
{
public:
    creater() = default;

public:
    virtual std::shared_ptr<node> get_node() const override
    {
        const std::shared_ptr<std::atomic_bool> flag = this->m_kill;
        auto op = [flag](flow_control& control) -> bool
        {
            if (flag->load(std::memory_order_relaxed))
                control.stop();
            return true;
        };

        node nd = base_creator::build_node(std::cref(op));
        return std::make_shared<node>(nd);
    };
};

int main(int argc, char* argv[])
{
    creater c;
    std::shared_ptr<base_creator::node> s = c.get_node();
    using my_func_node = std::shared_ptr<tbb::flow::function_node<bool, bool>>;
    my_func_node f = std::make_shared<tbb::flow::function_node<bool, bool>>(base_creator::g(), 1,[](const bool b) { std::cout << b; return b; });
    tbb::flow::make_edge(*s, *f);
};

The flag should be always false in this code. Once I call tbb::flow::make_edge it becomes true when debugging the body of the node s in THAT IS SO WEIRD? I have no clue, could you please help, I am starting to hit in the TBB code now and it's too complex :)

You need to show the code for `creator` ... You know show the code for its parent class. — ChrisMM, May 07 '22 at 12:31
`true (72)`? Does that mean the object-representation was `72`, not `1`? If so, that's further evidence (beyond a value changing when you don't expect it to) that you have a memory over-write bug somewhere. An actual `bool` is required by both mainstream x86-64 ABIs (Windows and SysV) to be `0` or `1`. ([Does the C++ standard allow for an uninitialized bool to crash a program?](https://stackoverflow.com/a/54125820)). **Set a watchpoint on that `atomic` and see what changes it.** — Peter Cordes, May 07 '22 at 13:12
You're not capturing the `atomic_bool`, though, you're capturing a `std::shared_ptr` local variable. It was constructed from the member, but itself is a local variable. (I don't see why you need all this complexity, or a `shared_ptr` member variable instead of just an `atomic_bool` member variable right in the class object, but maybe you have your reasons for wanting an extra level of indirection and the smart-pointer with control-block to manage it. Concurrent access to the same shared_ptr object is not safe, though, only access to the same pointed-to atomic_bool) — Peter Cordes, May 07 '22 at 13:23
The `tbb::flow::make_edge` changes it, but the TBB code is too complex for me to get the exact component (it could be during a destructor so I am not that good). I think I figured what's wrong, the constructor here `node nd = base_creator::build_node(std::cref(op));` does not like `std::cref` nor `std::ref`, **That's the bug**, however I thought `std::ref`/`std::cref` are just wrappers around the object, definitly a `move` constructor has been called somewhere and moved the value!! — Vero, May 07 '22 at 13:24
@Peter Cordes I read in the cpp reference access to shared is safe!! Are you sure? Dereferencing is safe (I have a good reason why it's shared :) ) — Vero, May 07 '22 at 13:25
https://en.cppreference.com/w/cpp/memory/shared_ptr - *All member functions (including copy constructor and copy assignment) can be called by multiple threads on **different instances of shared_ptr** without additional synchronization even if these instances are copies and share ownership of the same object. If multiple threads of execution access **the same instance of shared_ptr** without synchronization and any of those accesses uses a non-const member function of shared_ptr then a data race will occur; the shared_ptr overloads of atomic functions can be used to prevent the data race.* — Peter Cordes, May 07 '22 at 13:27
@Peter Cordes I am wrong then. Could please anyone help explaining the bug, likely you need the TBB code but no idea why TBB didn't raise an error. — Vero, May 07 '22 at 13:31
So an instance of `std::shared_ptr` itself isn't safe to access from multiple threads, except its const members. (Not sure which those include, hopefully deref for read-write access to the underlying object is ok). But the underlying `atomic_bool` itself can be concurrently accessed in any way by multiple threads at once. It's not clear to me whether your usage pattern might have any data-race bugs on the shared_ptr object. — Peter Cordes, May 07 '22 at 13:32
Did you try using a debugger to set a watchpoint? Finding out which code is actually changing the bool itself would be the first thing I'd want to know. — Peter Cordes, May 07 '22 at 13:33
@Peter Cordes Will do that. Following up please: I will never change the `shared_ptr` itself (only dereference it to get the atomic), only the content `atomic_bool`, I shouldn't have a data race then, I keep things as they are? — Vero, May 07 '22 at 13:34
Indeed, `operator*` and `operator->` are `const`, like the `get()` member function. https://en.cppreference.com/w/cpp/memory/shared_ptr/operator*. Your code does contain `const std::shared_ptr flag = this->m_kill;` - IDK if that's also safe. It may depend on whether which version of the copy constructor is called, and there are lots (https://en.cppreference.com/w/cpp/memory/shared_ptr/shared_ptr), some with non-const RHS. — Peter Cordes, May 07 '22 at 13:39
@Peter Cordes to be safe I will remove the `shared_ptr` from the `atomic` and pass `this` as capture and refer to it inside the `op` function :) THANKS VERY MUCH!! — Vero, May 07 '22 at 13:43
Yeah, that seems obviously more efficient than having a shared-ptr control block (and two pointers, i.e. an instance of `shared_ptr`) for every one-byte `atomic_bool`. If different objects don't need to maybe point at and share ownership of the same `atomic_bool`, just have the member in the class directly, not dynamically allocated and pointed-to. — Peter Cordes, May 07 '22 at 13:49
@Peter Cordes cool stuff, cool stuff!! Thanks for the time :) — Vero, May 07 '22 at 13:50

score 1 · Answer 1 · answered May 12 '22 at 06:12

Pay attention to the following code. It creates std::reference_wrapper over local object op that is copied into input_node with build_node. Therefore, when get_node returns the std::reference_wrapper (that is inside the node that is inside shared_ptr), it references the destroyed object. Then, make_edge reuses the stack and replaces flag pointer with some other pointer that contains 72.

        auto op = [flag](flow_control& control) -> bool
        {
            if (flag->load(std::memory_order_relaxed))
                control.stop();
            return true;
        };

        node nd = base_creator::build_node(std::cref(op));

Thanks so much! Clear now, that's the kind of mistakes every c++ programmer is told not to do.. Thanks again :) — Vero, May 12 '22 at 20:36

Weird behaviour changing captured atomic bool

1 Answers1