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