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

[Anthony Towns via Phabricator via cfe-commits]

ajtowns added a comment.

In D87629#2272676 <https://reviews.llvm.org/D87629#2272676>, @aaronpuchert wrote:

> (The existing `LockAssertion` class that the change removed looks like it should use `ASSERT_CAPABILITY` instead—please don't use `ACQUIRE` when the capability is assumed to be held previously.)

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

Extending the above with:

  void AssertHeld() ASSERT_CAPABILITY(m) { return; }
  void h(bool b) {
      try {
          if (b) thrower();
          i = 7;
          AssertHeld();
          i = 8;
     } catch (...) { }
     i = 9;
  }

only gives me a warning for `i = 7`, even though when `b` is true `i = 9` is just as bad as the `i = 6` assignment that did generate a warning.

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

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87629