Thread safety analysis: scoped lockable members and temporaries

[leonid-s-usov]

Hi!

I am trying to create a movable scoped lockable. It kind of works, but there are some cases that generate false positives. It seems that there are two interconnected problems:

1. The lack of support for scoped_lockable members
   • This has a side effect with lambda captures which are lambda class members under the hood
2. Some issue with temporaries, presumably around copy elision via temporary binding
   • Might also affect lambda capture initialization

Here is a compiler explorer example with all of the observed issues: https://godbolt.org/z/vfcb78jqc

In case the above compiler explorer snippet is not available, please see the full source code under the cut:

#include <memory>
#include <iostream>
#include <cassert>

struct __attribute__((lockable)) Mutex {
    void lock() __attribute__((exclusive_lock_function())) {
        assert(!locked);
        locked = true;
    }
    void unlock()  __attribute__((unlock_function())) {
        assert(locked);
        locked = false;
    }
    bool locked = false;
};

struct  __attribute__((scoped_lockable)) MutexLocker {
    MutexLocker(Mutex* pmutex) __attribute__((exclusive_lock_function(*pmutex)))
        : pmutex(pmutex)
    {
        assert(pmutex);
        pmutex->lock();
        std::cout << "constructor lock" << std::endl;
    }

    ~MutexLocker() __attribute__((unlock_function(*pmutex))) {
        if (pmutex) {
            std::cout << "destructor unlock" << std::endl;
            pmutex->unlock();
        }
    }

    // we mark this as exclusive lock because
    // although we're taking the mutex locked already
    // the static analysis doesn't know that other.pmutex
    // and this->pmutex are referencing the same object
    // With the current accounting we should pretend that
    // we took our new lock, and the other will unlock
    // in the destructor - not in the runtime, but static analysis-wise.
    MutexLocker(MutexLocker &&other) __attribute__((exclusive_lock_function(*pmutex))) {
        pmutex = other.pmutex;
        other.pmutex = nullptr;

        std::cout << "constructor move" << std::endl;
    }

    MutexLocker(const MutexLocker& other) = delete;
    MutexLocker& operator=(const MutexLocker& other) = delete;
    MutexLocker& operator=(MutexLocker&& other) = delete;

    Mutex *pmutex;
};


int main() {
    Mutex m;

    std::cout << "=== no warnings in most cases" << std::endl;
    {
        MutexLocker l(&m);

        MutexLocker l2 = std::move(l);

        auto val = [&]() {
            auto l3 = std::move(l2);
            std::cout << "in lambda 1" << std::endl;
            return 10;
        }();
    }

    std::cout << std::endl << "=== warnings 1.1: struct members" << std::endl;
    {
        MutexLocker l(&m);

        struct Wrapper {
            MutexLocker wrapped_locker;
        };

        auto wl = Wrapper { std::move(l) };

        assert(m.locked);
    }

    std::cout << std::endl << "=== warnings 1.2: lambda capture initialization" << std::endl;
    {
        MutexLocker l(&m);

        auto val = [l2 = MutexLocker(std::move(l))]() mutable {
            auto l3 = std::move(l2);
            std::cout << "in lambda 2" << std::endl;
            return 20;
        }();

        assert(!m.locked);
    }

    std::cout << std::endl << "=== warnings 2: binding a temporary" << std::endl;
    {
        MutexLocker locker(&m);

        auto && locker2 = [&]() -> MutexLocker {
            return std::move(locker);
        }();
                
        assert(m.locked);
    }

    assert(!m.locked);
    return 0;
}

[usx95]

CC @aaronpuchert @delesley

[leonid-s-usov]

I suspect now that the case with the lambda capture initialization is directly related to the struct member issue, plus the mishandling of the CXXBindTemporaryExpr which registers a call to constructor but loses track of the object afterwards

[usx95]

The problem is specific to how we handle member destructors.

https://godbolt.org/z/34xzeozGn

int main() {
    ProtectedValue value = {10, Mutex{}};
    struct Wrapper {
        ValueReader r;
    };
    Wrapper wr = Wrapper{ValueReader(&value)};
}

CFG

 [B1]
   1: 10
   2: /*implicit*/(_Bool)0
   3: {}
   4: Mutex[B1.3] (CXXFunctionalCastExpr, NoOp, Mutex)
   5: {[B1.1], [B1.4]}
   6: ProtectedValue value = {10, Mutex{}};
   7: value
   8: &[B1.7]
   9: [B1.8] (CXXConstructExpr, ValueReader)
  10: [B1.9] (BindTemporary)
  11: ValueReader([B1.10]) (CXXFunctionalCastExpr, ConstructorConversion, ValueReader)
  12: {[B1.11]}
  13: [B1.12] (BindTemporary)
  14: Wrapper[B1.13] (CXXFunctionalCastExpr, NoOp, Wrapper)
  15: Wrapper wr = Wrapper{ValueReader(&value)};
  16: [B1.15].~Wrapper() (Implicit destructor)
   Preds (1): B2
   Succs (1): B0

I think what is happening here is that the dtor of the temporary ~ValueReader is called by Wrapper::~Wrapper() and not inside main(). This is expected.

But this makes the analysis see one lock (@ValueReader::ValueReader(ProtectedValue*) but only no unlock.

entry:
  %value = alloca %struct.ProtectedValue, align 4
  %wr = alloca %struct.Wrapper, align 8
  call void @llvm.memcpy.p0.p0.i64(ptr align 4 %value, ptr align 4 @__const.main.value, i64 8, i1 false)
  %r = getelementptr inbounds %struct.Wrapper, ptr %wr, i32 0, i32 0
  call void @ValueReader::ValueReader(ProtectedValue*)(ptr noundef nonnull align 8 dereferenceable(8) %r, ptr noundef %value)
  call void @main::Wrapper::~Wrapper()(ptr noundef nonnull align 8 dereferenceable(8) %wr) #6
  ret i32 0

We seem to ignore member destructors while handling destructor calls.

[leonid-s-usov]

We seem to ignore member destructors while handling destructor calls.

Correct. Though the second piece of the puzzle is that bind temporary step. It's going to be tricky, since the member constructor isn't called explicitly, but "inherited" from that temporary construction via binding. That will have to be taken into account to be able to match the member destructor and the constructor calls lock name wise.

Though maybe it's not going to be that of an issue

[leonid-s-usov]

The problem is specific to how we handle member destructors.

I agree that the lambda capture initialization is related to struct members. However, the third case has no members involved, though it has CXXBindTemporaryExpr which constructs the return value directly into the locker2 var.

int main() {
    Mutex m;

    {
        MutexLocker locker(&m);

        auto && locker2 = [&]() -> MutexLocker {
            return std::move(locker);
        }();
    }

    assert(!m.locked);
    return 0;
}

UPD: I have further simplified the example

[aaronpuchert]

I am trying to create a movable scoped lockable.

The problem with moving is not so much support for moving itself, but the accompanying rewrite of the managed capabilities. On construction of a scoped capability, we store expressions referencing the managed capabilities (the mutexes managed by the scoped object), obtained by rewriting the attribute expression in the context of construction. At this point, by definition, we can still reference the mutex, but after a move, this might no longer be the case.

struct Foo {
  Mutex mu;
};
struct __attribute__((scoped_lockable)) Locker {
  Locker(Foo& foo) __attribute__((exclusive_lock_function(foo.mu)));
};
void bar(Foo* ptr) {
  Locker lock(*ptr);
}

In this case, we substitute *ptr for the formal parameter foo and get (*ptr).mu as the capability managed by lock.

See below for an example where it doesn't work, because we have a scoped lockable that's not a local variable.

It kind of works, but there are some cases that generate false positives. It seems that there are two interconnected problems:

1. The lack of support for scoped_lockable members

This is probably the most interesting case for moving, but it breaks the connection between scoped capability and managed capability. If we move the scoped capability into a different context, the managed capability might no longer be "expressible":

struct Foo {
  Mutex mu;
  Wrapper lock();  // Wrapper from original example
};

void bar() {
  Foo foo;
  Wrapper wrapper = foo.lock();
  // How does wrapper.wrapped_locker reference foo.mu?
}

This is a problem of scopes and lookup: there is no way to reference foo.mu from inside the wrapper. The wrapper cannot refer to the context it is contained in, because it doesn't know anything about that context. This is not an issue if you return the MutexLocker itself, but as a member of another struct it loses the connection.

2. Some issue with temporaries, presumably around copy elision via temporary binding

Maybe you can help me spot the copy elision in your examples, but fundamentally you're right: in case of mandatory copy elision, we don't have a constructor call at all, so attributes on a constructor cannot be taken into account. But I'm not sure what kind of problems this generates. Note that I changed the behavior in case of copy elision in https://reviews.llvm.org/D129755 (54bfd04). We take the attributes on the function that returns the scoped lockable object in that case.

Here is a compiler explorer example with all of the observed issues: https://godbolt.org/z/vfcb78jqc
In case the above compiler explorer snippet is not available, please see the full source code under the cut:

    // we mark this as exclusive lock because
    // although we're taking the mutex locked already
    // the static analysis doesn't know that other.pmutex
    // and this->pmutex are referencing the same object
    // With the current accounting we should pretend that
    // we took our new lock, and the other will unlock
    // in the destructor - not in the runtime, but static analysis-wise.
    MutexLocker(MutexLocker &&other) __attribute__((exclusive_lock_function(*pmutex))) { /*...*/ }

Conceptually this doesn't look right. The move constructor doesn't acquire anything, and it should probably not need an annotation at all.

    std::cout << "=== no warnings in most cases" << std::endl;
    {
        MutexLocker l(&m);

        MutexLocker l2 = std::move(l);

        auto val = [&]() {
            auto l3 = std::move(l2);
            std::cout << "in lambda 1" << std::endl;
            return 10;
        }();
    }

The first part should be easy to support, but uninteresting. The second part is more problematic. The lambda captures the scoped lockable, but probably not the mutex itself. So how do we express the relationship of the lockable to the mutex, if it is unknown to the lambda? If it is known, it would be known via alias.

Also, if we don't see the mutex anymore, what about data that has guarded_by attribute? That likely lives in the same scope.

The problem is specific to how we handle member destructors.

So far we have restrained from looking inside called functions, but since destructors are known to destruct all members we could probably take that into account. However, destruction is not the biggest issue here, it's the rewriting of expressions.