[libc++] missing thread safety annotations for std::unique_lock and std::shared_lock

[tamird]

#include <mutex>
#include <shared_mutex>

class Foo {
    int guard() {
        std::lock_guard l(lock_);
        return state_; // no warning
    }
    int unique() {
        std::unique_lock l(lock_);
        return state_; // warning: reading variable 'state_' requires holding shared_mutex 'lock_' [-Wthread-safety-analysis]
    }
    int shared() {
        std::shared_lock l(lock_);
        return state_; // warning: reading variable 'state_' requires holding shared_mutex 'lock_' [-Wthread-safety-analysis]
    }
    private:
    int state_ __attribute__((guarded_by(lock_)));
    std::shared_mutex lock_;
};

https://godbolt.org/z/hq7f9xT9K

Is there a reason these can't be annotated?

[tamird]

cc @EricWF - the most recent discussion of this I could find was https://discourse.llvm.org/t/std-unique-lock-with-thread-annotations/53345/4. It'd be helpful if you could describe the missing functionality needed to implement this.

[CaseyCarter]

unique_lock has constructors (unique_lock(mutex_type&, try_to_lock_t), unique_lock(mutex_type&, const chrono::duration<Rep, Period>&), unique_lock(mutex_type&, const chrono::time_point<Clock, Duration>&)) which conditionally acquire the associated lockable. The thread-safety annotations do not support conditionally-held locks.

unique_lock has an assignment operator, release, and swap which disassociate or reassociate the associated lockable objects. There seems to be no way to annotate these changing relationships between locks and lockables.

I'm very familiar with unique_lock, but just barely acquainted with Clang's thread-safety annotations, so I may be mistaken about the ability of the annotations to express some or all of the above.

[tamird]

@CaseyCarter does it have to be all or nothing? Could we annotate the subset that is supported?

[CaseyCarter]

In general, I think that static analysis with frequent false positives and false negatives is more hurtful than helpful. I think that forcing such warnings on everyone would be counter to the Clang philosophy of minimizing warning false positives. That said, I assume that -Wthread-safety is off-by-default (not part of -W, -Wextra, or -pedantic) so maybe it would be acceptable for libc++ to support annotations so well-informed users could effectively opt-in.

I suppose there is still a concern about people who are currently using libc++ as well as some other annotated library. If libc++ adds these annotations such users may start seeing confusing false indications for their libc++ use. I think this scares me just enough that I would not want to implement annotations in MSVCSTL, certainly not without a way to opt-out. I don't know if the libc++ maintainers are as conservative as MSVCSTL, so I'll stop talking and get out of their thread =)