[PATCH] D51187: [RFC] Thread safety analysis: Track status of scoped capability

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

> Any changes should always be done by adding or removing entries from the FactSet, not by mutating the underlying FactEntries.

To make that clearer in the code, I made `FactEntry`s immutable that are managed by `FactManager` in https://reviews.llvm.org/rC342787.

In https://reviews.llvm.org/D51187#1242620, @delesley wrote:

> It should definitely go in -Wthread-safety-beta so it won't break the build.  Unfortunately, the before/after checks are still in thread-safety-beta, and they should really be moved out of beta before something else is moved in.  The good news is that Matthew O'Niel just volunteered to do that.  That is, unfortunately, not a trivial operation, so it may be some weeks before he's done.


That's Ok for me. It's not something that I terribly need, but it seemed to make things more consistent.

Does the migration of the before/after checks include changes to how it is handled? Because I wondered why it doesn't work on S-expressions like the rest of the analysis, but just `ValueDecl`s. That leads to some false positives with more complex expressions:

  class A {
  public:
    Mutex mu1;
    Mutex mu2 ACQUIRED_AFTER(mu1);
  };
  
  class B {
    A a1, a2;
    Mutex lm ACQUIRED_AFTER(a1.mu2);
  
  public:
    void f() {
      a2.mu2.Lock();
      a1.mu1.Lock();    // warns "mutex 'mu1' must be acquired before 'mu2'", but they are on different objects
      a1.mu1.Unlock();
      a2.mu2.Unlock();
    }
  
    void g() {
      lm.Lock();
      a1.mu1.Lock();
      a1.mu1.Unlock();
      a2.mu1.Lock();    // Warns "mutex 'mu1' must be acquired before 'lm'", but lm talks only about a1.mu2.
      a2.mu1.Unlock();
      lm.Unlock();
    }
  };

But maybe that's not the right place to discuss this.


Repository:
  rC Clang

https://reviews.llvm.org/D51187