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

[Aaron Puchert via Phabricator via cfe-commits]

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