[clang] Thread Safety Analysis: Differentiate between lock sets at real join points and expected/actual sets at function end (PR #105526)
Aaron Puchert via cfe-commits
cfe-commits at lists.llvm.org
Thu Sep 12 10:51:39 PDT 2024
aaronpuchert wrote:
The warning is arguably wrong, and this might even be considered a regression. However, operations on asserted capabilities have been an issue before this change, and this worked for scoped capabilities only accidentally. If you do this without a scoped capability, you get a warning even before this change:
```c++
bool InitPreferredSampleRateDirect() {
sMutex.AssertCurrentThreadOwns();
sMutex.Unlock();
sMutex.Lock();
return true;
} // warning: mutex 'sMutex' is still held at the end of function [-Wthread-safety-analysis]
```
This should behave exactly like your example, because it does the same operations in the same order. They are semantically equivalent. And if you remove `sMutex.Lock()`, there is no warning even though there should be one. Doing this via scoped capability hid the warning because we didn't look at the underlying mutexes of a scoped lock at the end of a function prior to this change, but that is arguably wrong. We should look at the set of underlying mutexes to find the bug here:
```c++
class __attribute__((scoped_lockable)) StaticMutexAdoptLock {
public:
explicit StaticMutexAdoptLock(StaticMutex& aLock) __attribute__((exclusive_locks_required(aLock)));
~StaticMutexAdoptLock(void) __attribute__((unlock_function()));
};
bool InitPreferredSampleRateAdopt() {
sMutex.AssertCurrentThreadOwns();
{
StaticMutexAdoptLock unlock(sMutex);
}
return true;
} // should warn here, but we don't
```
Even after this change, we don't warn about the unscoped version of this, but this is because the modelling of asserted capabilities is wrong. Without this change and a fixed modelling of asserted capabilities, we still wouldn't warn, because `sMutex` is managed. That's why I think this change is correct even though it introduced a false positive.
I agree that the modelling of asserted capabilities should be fixed, but it's orthogonal to this change. For now, the only thing that's reliably supported with asserted capabilities is to leave them as they are. We need to figure out what the semantics are for locking asserted capabilities, perhaps through a scoped capability, and how we implement that. Currently, unlocking the asserted capability just removes it from the set of held capabilities, which is true but doesn't account for the fact that we should reacquire it. Similarly, reacquiring the asserted capability introduces an acquired capability, but the function wasn't declared to have the capability acquired at the end of the function. That's why you get the warning. Maybe we should treat this as asserted capability, because you're just reacquiring a previously released asserted capability. Asserted capabilities are fine to be held at the end of a function.
https://github.com/llvm/llvm-project/pull/105526
More information about the cfe-commits
mailing list