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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
