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

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

In D87629#2280475 <https://reviews.llvm.org/D87629#2280475>, @ajtowns wrote:

> Sure, it makes perfect sense that it's too hard.

It's not really too hard, there is an existing parameter somewhere in the CFG generation that controls these exception handling edges, and we'd probably have to change some other diagnostics plus improve the performance/memory usage.

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

Not quite, if you have an explicit `throw` statement that connects to the `catch` block it will be analyzed just like anything else. Otherwise we currently see the catch block as unreachable (wrongly of course). Some other diagnostics work around that, but I don't think it's worth doing it here as well. We better fix the CFG.

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

With an explicit throw you should be able to reach the catch block. Otherwise no.

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

Right, if you have a `throw`, but that's not what I was saying. If there is no potentially throwing call before `x()` then `y()` can only be called after `x()`.

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

Completely understand your sentiment here, but for a compiler warning that's treated as error on large code bases we have to be a bit careful. See e.g. D52395#1248608 <https://reviews.llvm.org/D52395#1248608> by the original author of this feature. I've almost gotten those angry rollbacks, see e.g. D49355#1188448 <https://reviews.llvm.org/D49355#1188448> or D52398#1255149 <https://reviews.llvm.org/D52398#1255149>.

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

That's the situation, there are false negatives. And we're slowly trying to close the gaps. But hacky workarounds using misleading annotations don't improve it.

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

That's the key: It accidentally does the right thing in your example, but for the wrong reasons. (It thinks we get the capability, but then release it, whereas in reality we never had it.)

Regarding your analysis: scopes and code paths are orthogonal. Scopes are only interesting for lookup and automatic destructor calls of automatic storage duration variables. So they have nothing to with the validity of an assertion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87629