[PATCH] D87629: Thread safety analysis: Improve documentation for ASSERT_CAPABILITY

[Anthony Towns via Phabricator via cfe-commits]

ajtowns added a comment.

In D87629#2278580 <https://reviews.llvm.org/D87629#2278580>, @aaronpuchert 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?
>> `void g() { try { thrower(); } catch (...) { i = 5; } i = 6; }`
>> gives me warnings for ... `i=6` but not `i=5` ?
>
> It's not that we don't check catch-blocks, but rather that the Clang CFG is incomplete:
> 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)

Sure, it makes perfect sense that it's too hard. But as far as I can see the end result is that catch blocks are treated as if the code therein was annotated with NO_THREAD_SAFETY_ANALYSIS?

> You're cherry-picking here. If you move `i=9` into the catch block, there is still no warning.

Right -- that was the `a=0` and `i=5` cases. But there's no way to get a warning for those cases, no matter what I do, as far as I can see?

> 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.

I'd expect: `try { if (b) throw; x(); } catch(...) { } y(); ` and `if (!b) { x(); } y();`

to generate the same errors/warnings. If `x()` is marked `ASSERT_CAPABILITY(m)` and `y()` is marked `REQUIRES(m)` they don't.

I mean, I get why they don't -- exceptions are hard. But if I have to choose between getting warnings on bug free code, and not getting warnings on buggy code, I think I'd rather get warnings on the bug free code, since it's easy to make them go away just by moving the assertion claim into the outer scope, or adding a second assertion later given it compiles to a no-op.

I mean, if I'm using ASSERT_CAPABILITY in the first place, it's because I'm already getting false positives.

> Assertions aren't scoped. The easiest way to see that is that the destructor doesn't do anything.

Right, and in a perfect world that would be fine; but when other parts of the analysis aren't also perfect, it seems like it leads to false negatives that let bugs through?

It looks to me like:

- y() is reachable from the catch(...) block, but we don't know what locks might be held there, so anything goes
- y() is reachable from the try block if nothing is thrown, and in that case the lock is asserted, so it's fine
- because all ways of reaching y() either have the lock or are too hard to analyse, don't warn

With the dummy SCOPED_CAPABILITY that becomes:

- y() is reachable from the catch(...) block, but we don't know what locks might be held there, so anything goes
- y() is reachable from the try block if nothing is thrown, but in that case the capability is released at end of scope
- because there's a way of reaching y() without holding the capability, issue a warning

I mean, I get that that's "wrong" in that it's generating the warning due to the code path that's safe rather than the code path that's unsafe; but it's "right" in that it's ensuring there's an analysable code path that doesn't hold the capabilities that the other un-analysable code paths might not have. If we're sure every un-analysable code path will have the lock, then we can put the assumption in the outer scope.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87629/new/

https://reviews.llvm.org/D87629