Add SAL annotations to shared_mutex.

[NN---]

Annotate shared_mutex.
Next part can be mutex annotations #3163.
This allows compiler to catch the following mistakes:

void g3()
{
    std::shared_mutex sm;

    std::shared_lock x(sm);
    std::shared_lock x2(sm);
}

// warning C26111: Caller failing to release lock 'sm' before calling function 'std::shared_lock<std::shared_mutex>::shared_lock<std::shared_mutex>
// warning C26110: Caller failing to hold lock '&x' before calling function 'std::shared_lock<std::shared_mutex>::~shared_lock<std::shared_mutex>'.

void g4()
{
    std::shared_mutex sm;

    sm.lock();
    std::shared_lock x(sm);

    sm.lock_shared();
}

// warning C26111: Caller failing to release lock '&sm' before calling function 'std::shared_lock<std::shared_mutex>::shared_lock<std::shared_mutex>'
// warning C26111: Caller failing to release lock '&sm' before calling function 'std::shared_mutex::lock_shared'.
// warning C26110: Caller failing to hold lock '&x' before calling function 'std::shared_lock<std::shared_mutex>::~shared_lock<std::shared_mutex>'.

void g3()
{
    std::shared_mutex sm;
    if (sm.try_lock())
    {
        sm.unlock();

        if (sm.try_lock_shared())
        {
            sm.lock();
        }
    }
}

// warning C26111: Caller failing to release lock '&sm' before calling function 'std::shared_mutex::lock'.

[StephanTLavavej]

Ok, I've pushed changes to address the issues I previously found, and I believe (based on command-line testing) that they improve things.

However, there are some warnings being emitted in the implementation, and I haven't figured out how to avoid them yet (without blunt suppression, instead of targeted assume directives):

D:\GitHub\STL\out\x64>type meow.cpp

#include <chrono>
#include <shared_mutex>
#include <utility>
using namespace std;
using namespace std::chrono;

void test_shared_mutex() {
    shared_mutex sm;

    sm.lock();
    sm.unlock();

    if (sm.try_lock()) {
        sm.unlock();
    }

    sm.lock_shared();
    sm.unlock_shared();

    if (sm.try_lock_shared()) {
        sm.unlock_shared();
    }
}

void test_shared_timed_mutex() {
    shared_timed_mutex stm;

    const duration dur  = 100ms;
    const time_point tp = steady_clock::now() + dur;

    stm.lock();
    stm.unlock();

    if (stm.try_lock()) {
        stm.unlock();
    }

    if (stm.try_lock_for(dur)) {
        stm.unlock();
    }

    if (stm.try_lock_until(tp)) {
        stm.unlock();
    }

    stm.lock_shared();
    stm.unlock_shared();

    if (stm.try_lock_shared()) {
        stm.unlock_shared();
    }

    if (stm.try_lock_shared_for(dur)) {
        stm.unlock_shared();
    }

    if (stm.try_lock_shared_until(tp)) {
        stm.unlock_shared();
    }
}

void test_shared_lock() {
    using STM = shared_timed_mutex;
    using SL  = shared_lock<STM>;

    const duration dur  = 100ms;
    const time_point tp = steady_clock::now() + dur;

    SL default_constructed;

    {
        STM stm;
        SL sl_mutex{stm};
    }

    {
        STM stm;
        SL sl_deferred{stm, defer_lock};

        sl_deferred.lock();
        sl_deferred.unlock();

        if (sl_deferred.try_lock()) {
            sl_deferred.unlock();
        }

        if (sl_deferred.try_lock_for(dur)) {
            sl_deferred.unlock();
        }

        if (sl_deferred.try_lock_until(tp)) {
            sl_deferred.unlock();
        }

        [[maybe_unused]] bool b1 = sl_deferred.owns_lock();
        [[maybe_unused]] bool b2 = !!sl_deferred;
    }

    {
        STM stm;
        SL sl_try{stm, try_to_lock};
    }

    {
        STM stm;
        stm.lock_shared();
        SL sl_adopt{stm, adopt_lock};
    }

    {
        STM stm;
        SL sl_try_for{stm, dur};
    }

    {
        STM stm;
        SL sl_try_until{stm, tp};
    }

    {
        STM stm;
        SL sl1{stm};
        SL sl2{static_cast<SL&&>(sl1)};
    }

    {
        STM cat;
        STM dog;
        SL sl1{cat};
        SL sl2{dog};
        sl1 = static_cast<SL&&>(sl2);
    }

    {
        STM cat;
        STM dog;
        SL sl1{cat};
        SL sl2{dog};
        sl1.swap(sl2);
    }

    {
        STM stm;
        SL sl{stm};

        STM* raw_ptr = sl.release();
        raw_ptr->unlock_shared();
    }
}

D:\GitHub\STL\out\x64>cl /EHsc /nologo /W4 /MT /std:c++latest /analyze /analyze:autolog- /analyze:plugin EspXEngine.dll /analyze:ruleset "%VSINSTALLDIR%\Team Tools\Static Analysis Tools\Rule Sets\ConcurrencyCheckRules.ruleset" /c /I ..\..\stl\inc meow.cpp
meow.cpp
D:\GitHub\STL\stl\inc\shared_mutex(386) : warning C26135: Missing annotation _Acquires_shared_lock_(this->_Pmtx) at function 'std::shared_lock<std::shared_timed_mutex>::operator='.: Lines: 376, 377, 378, 379, 382, 383, 384, 385, 386
D:\GitHub\STL\stl\inc\shared_mutex(354) : warning C26167: Possibly releasing unheld lock '*this->_Pmtx' in function 'std::shared_lock<std::shared_timed_mutex>::~shared_lock<std::shared_timed_mutex>'.: Lines: 352, 353, 354
D:\GitHub\STL\stl\inc\shared_mutex(313) : warning C26166: Possibly failing to acquire or to hold lock '*this->_Pmtx' in function 'std::shared_lock<std::shared_timed_mutex>::shared_lock<std::shared_timed_mutex>'.: Lines: 312, 313
D:\GitHub\STL\stl\inc\shared_mutex(297) : warning C26166: Possibly failing to acquire or to hold lock '*this->_Pmtx' in function 'std::shared_lock<std::shared_timed_mutex>::shared_lock<std::shared_timed_mutex>'.: Lines: 295, 296, 297
D:\GitHub\STL\stl\inc\shared_mutex(136) : warning C26165: Possibly failing to release lock '*this' in function 'std::shared_timed_mutex::try_lock_for<__int64,std::ratio<1,1000> >'.: Lines: 132, 136
D:\GitHub\STL\stl\inc\shared_mutex(217) : warning C26165: Possibly failing to release lock '*this' in function 'std::shared_timed_mutex::try_lock_shared_for<__int64,std::ratio<1,1000> >'.: Lines: 212, 217
D:\GitHub\STL\stl\inc\shared_mutex(331) : warning C26166: Possibly failing to acquire or to hold lock '*this->_Pmtx' in function 'std::shared_lock<std::shared_timed_mutex>::shared_lock<std::shared_timed_mutex><__int64,std::ratio<1,1000> >'.: Lines: 325, 331
D:\GitHub\STL\stl\inc\shared_mutex(343) : warning C26166: Possibly failing to acquire or to hold lock '*this->_Pmtx' in function 'std::shared_lock<std::shared_timed_mutex>::shared_lock<std::shared_timed_mutex><std::chrono::steady_clock,std::chrono::duration<__int64,std::ratio<1,1000000000> > >'.: Lines: 337, 343

Any ideas what I'm missing here?

[StephanTLavavej]

I believe I found how to suppress the spurious warnings - my test case now compiles cleanly, while your original examples all appear to emit intended warnings. (Probably need more manual testing.)

Unfortunately, I wasn't able to add the test case to the test harness for the most frustrating reason - the path to the ruleset contains spaces, which our env.lst and Python machinery is not prepared to escape.

Edit: I realized that we should be able to add a "custombuild" script (at least for the GitHub side, not MSVC-internal) to form an arbitrary command line, as we do in the Header Units and Modules tests. I can look into that later.

[StephanTLavavej]

Thanks again (so much) for putting extensive effort into developing this PR and responding to our feedback. We talked about this at the weekly maintainer meeting, after having contacted the compiler's static analysis team with questions about the annotations. Unfortunately, we've learned that the concurrency SAL annotations were never designed with C++ lock types in mind, and we have significant concerns about the compiler's ability to handle them robustly. (Internally, there's a concerningly small amount of test coverage for them, and none with actual C++ types with member functions like try_lock(), only a few plain C-style functions.) The static analysis team wants to look into enhancing the compiler with better knowledge of Standard Library mutexes and smart lock types, which may be entirely within the compiler, or may require some annotation support in the library sources. However, at this time, we don't believe that we can merge any concurrency SAL annotations (not specific to the great work you've done in this PR!) due to the risk associated with flaky compiler behavior in a feature that hasn't been thoroughly tested.

If and when the static analyzer is enhanced, we can look into resuming this work. We regret having to close your first submitted PR to the microsoft/STL repo (so sorry! 😿) but it's not your fault at all, it's ours.

[NN---]

Thanks for the effort, the engagement with this PR was not expected at all.
Yeah, compiler issues are not fun at all.

The simple cases of lock annotations in class work pretty well, I use them in other classes.
Given the value lock annotations can, I hope compiler team can invest some time for improving them.
Meanwhile I try opening as more issues as possible with lock annotations.

[StephanTLavavej]

You're welcome! I hope the compiler team can make some significant improvements here too. Thanks for reporting issues, having them tracked in DevCom helps the compiler team a lot.