[cfe-dev] Who is working on/has worked on Capability analysis (-Wthread-safety)?

[Randell Jesup via cfe-dev]

On 11/5/2021 12:32 AM, Randell Jesup wrote:
> On 11/4/2021 6:55 PM, Aaron Puchert wrote:
>> Am 04.11.21 um 04:40 schrieb Randell Jesup:
>>>
>>>
>>> Well, the example I gave doesn't match that, I believe.  This is a 
>>> value only written on Thread 1, and read on Thread 1 and on other 
>>> threads. The requirements for locking would be the lock around all 
>>> writes (on thread 1), and lock around all reads from threads *other* 
>>> than thread 1.  Reads from Thread 1 don't need to lock, since 
>>> they're on the only thread that ever writes to the value.   This is 
>>> conceptually GUARDED_BY(mMutex || OnThread1).
>>>
>>> The case you're referencing sounds more like "value is freely 
>>> written and read on thread 1, then at some point later access to it 
>>> is provided to other threads, and after that happens access is 
>>> controlled by a mutex" - where the protection regime changes over 
>>> time (and may also correlate with specific methods, like an Init() 
>>> method).
>>>
>> You're absolutely right, I was misreading your case.
>>
>> What you're describing sounds in fact like thread 1 always has a 
>> shared lock, which it sometimes promotes to exclusive lock, then 
>> demotes to a shared lock again. The other threads can only acquire 
>> shared locks, because thread 1 doesn't ever release its shared lock. 
>> Now there are multiple ways to write that down:
>>
>
> You're correct, that's a reasonable way to model this pattern using 
> locks for everything.   The pattern I mentioned is data-race safe (it 
> shouldn't trigger tsan alerts), but doesn't allow for validation of 
> safety.  It depends on the developers following the rules for that 
> variable.
>

The downside of using a rwlock is performance and fairness; c++ doesn't 
guarantee any specific fairness to rwlocks, and they have lower 
performance/higher complexity than normal locks/mutexes. In particular, 
an implementation could let readers starve writers... though if the same 
thread holds a read lock, can it obtain a write lock?  Only I think in 
rwlocks that allow upgrades.  Otherwise it must release the read lock, 
grab the write lock, then release it and get the read lock back.

The other alternatives are to lock exclusively around each access 
(including on the writing thread), or ignore static analysis warnings if 
we're on the writing thread, or annotate with an assertion that quiets 
the warnings, such as AssertOnWritingThread(), which has 
ASSERT_EXCLUSIVE_LOCK/ASSERT_CAPABILITY (perhaps only in DEBUG mode).

>
>>> We have another pattern that's interesting, where a method may use 
>>> Maybe<MutexAutoLock> lock(std::in_place, mMutex);  (Maybe<> is 
>>> effectively std::optional). This allows us to lock.reset() instead 
>>> of using MutexAutoUnlock lock(mLock), which will lead to an extra 
>>> lock/unlock pair compared to Maybe<> and lock.reset().
>>>
>> Supporting that would be hard, because we'd need to look into the 
>> Maybe type, and then the analysis isn't local anymore. However, the 
>> analysis supports “premature unlocking” of scopes, i.e. you can write 
>> lock.Unlock(). I've even added support for “relockable scopes”, where 
>> you can lock.Lock() again. (Have a look at the MutexLocker in 
>> https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#mutex-h to see 
>> what is possible.)
>>
>
> I've checked, and the reason for this pattern is basically as I 
> mentioned; to allow for unlocking of an RAII lock without an added 
> lock/unlock pair on exit of the scope.   If we can do "MutexAutoLock 
> lock(mMutex); ...; if (foo) { lock.Unlock(); 
> MethodThatWeCantHoldLockIn(); return; } ..." then we don't need the 
> Maybe<> stuff.
>

Writing a MaybeMutexAutoLock class actually works fairly well for this 
case, and avoids Maybe<>/std::optional, and lets us avoid the extra bool 
for all the MutexAutoLocks (which appear ~50-100x+ the frequency of 
MaybeMutexAutoLock).

Randell Jesup, Mozilla
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20211105/43e39835/attachment.html>