aaronpuchert wrote:
> My software currently contains a nasty global recursive lock that we hope to
> (over a long period of time, likely years) convert to non-recursive.
>
> In the meantime, and during the refactor, it's helpful to be able to mark
> some functions as `EXCLUSIVE_LOCKS_REQUIRED(!recursive_mut)` even though
> technically it wouldn't be a problem.
>
> Ideally, that would even "convert" the lock, as far as the annotations go, to
> non-recursive. That way, from that point on, functions further down the
> call-stack could no longer double-lock. Though, I suppose that's arguably a
> different feature altogether.
That's a very legitimate use case, and a good idea. However, before you address
this interprocedurally, wouldn't you want to address it intraprocedurally? That
is, before you start adding `REQUIRES(!...)` annotations to fix double
acquisition across function boundaries, wouldn't it seem reasonable to remove
the `reentrant_capability` attribute and fix double acquisition within the same
function?
Reentrant mutexes don't have to be annotated as `reentrant_capability`. Our
code base has reentrant mutexes as well, and we never needed the attribute. The
reason is that the analysis only looks at a single function at a time. So it
will not complain if you acquire a mutex, and then call a function which
acquires it again. It will only complain if the same function acquires the
mutex twice:
```c++
void f() {
MutexLocker scope(&mu);
{
// ...
MutexLocker scope(&mu); // warning if you don't use `reentrant_capability`
}
}
```
The inner acquisition is legal with a reentrant mutex, but not necessary. In
general, where the analysis warns about double acquisition, the mutex is
already known to be held, so that acquisition is always superfluous and can be
removed. So even though this could be seen as a false positive, it should be
trivial to fix and the fix should be an improvement.
The problem in the Linux kernel, and the reason why @melver added support for
reentrant capabilities, is that apparently some acquisitions there happen as
part of a macro that also does other things, and so there are cases of double
acquisition that are hard to fix. My recommendation (and that of the
[documentation](https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#reentrant-capability))
is to not use the attribute just because you have a reentrant mutex. Only use
it if you have the similar patterns that are hard to fix.
https://github.com/llvm/llvm-project/pull/141599
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
