aaronpuchert added a comment.
In D87629#2278383 <https://reviews.llvm.org/D87629#2278383>, @ajtowns wrote:
> Not sure the `try { AssertHeld } catch (...) { a = 0; }` example reveals very
> much: it seems like thread safety annotations aren't checked within a catch
> block at all?
>
> Mutex m;
> int i GUARDED_BY(m);
>
> static void thrower() { throw std::exception(); }
> void f() { i = 1; }
> void g() {
> try {
> thrower();
> } catch (...) {
> i = 5;
> }
> i = 6;
> }
>
> gives me warnings for `i=1` and `i=6` but not `i=5` ?
It's not we don't check catch-blocks, but rather that the Clang CFG is
incomplete:
[B5 (ENTRY)]
Succs (1): B4
[B1]
1: 6
2: i
3: [B1.2] = [B1.1]
Preds (2): B3 B4
Succs (1): B0
[B2]
T: try ...
Succs (1): B3
[B3]
catch (...):
1: catch (...) {
[B3.4];
}
2: 5
3: i
4: [B3.3] = [B3.2]
Preds (1): B2
Succs (1): B1
[B4]
1: thrower
2: [B4.1] (ImplicitCastExpr, FunctionToPointerDecay, void (*)(void))
3: [B4.2]()
Preds (1): B5
Succs (1): B1
[B0 (EXIT)]
Preds (1): B1
There is no edge from the `thrower` call to `B2` or `B3`. We can turn on
"exception handling edges" from non-noexcept calls to catch blocks, but that's
a bigger change and it would affect other warnings as well. There are also
performance problems. (https://bugs.llvm.org/show_bug.cgi?id=43311)
> Using a dummy RAII class with SCOPED_CAPABILITY that does ACQUIRE/RELEASE
> without modifying the actual mutex seems like it behaves more sensibly to
> me...
Well, you clearly don't acquire or release anything. It's just a hack that
seems to solve the problem, but doesn't really (see below).
> class SCOPED_LOCKABLE assumer
> {
> public:
> assumer(Mutex& m_) EXCLUSIVE_LOCK_FUNCTION(m_) { }
> ~assumer() UNLOCK_FUNCTION() { }
> };
>
> void h(bool b) {
> try {
> if (b) thrower();
> i = 7;
> assumer a(m);
> i = 8;
> } catch (...) { }
> i = 9;
> }
>
> gives me warnings for both `i=7` and `i=9` but allows `i=8`.
You're cherry-picking here. If you move `i=9` into the catch block, there is
still no warning. And if you remove `if (b) thrower();` we wouldn't want the
warning, because the assertion could be calling `std::abort` and then we'd have
a false positive.
Assertions aren't scoped. The easiest way to see that is that destructor
doesn't do anything.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D87629/new/
https://reviews.llvm.org/D87629
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
