[PATCH] D129755: Thread safety analysis: Support copy-elided production of scoped capabilities through arbitrary calls

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

In D129755#3866213 <https://reviews.llvm.org/D129755#3866213>, @aaronpuchert wrote:

> Are you seeing warnings because of the different treatment of copy-elided construction, or because we've started to consider `CXXConstructorCall`s outside of the initializer of a `DeclStmt`?

I'm not familiar with internal Clang APIs so I'm not entirely sure how to answer that, but I can share sketches of the breakages I've addressed so far:

1. (2x) Returning a MutexLock-like structure, e.g.

  MutexLock Lock() {
    return MutexLock(&mu);
  }

this is documented in the test as a known false positive. Is that something you plan to address some day, or is it beyond the limits of thread safety analysis? (Or theoretically possible, but then compile times would be too much if you have to do more analysis?) Anyway, I applied `__attribute__((no_thread_safety_analysis))` to the method.

2. (3x) deferred/offloaded unlocking, e.g.

  void Func() {
    auto *lock = new MutexLock(&mu);
    auto cleanup = [lock]() { delete lock; };
    cleanup();
  } // Error that mu is not unlocked

I assume this is beyond the scope of thread safety analysis -- `cleanup()` in the internal case actually happens in a different TU (the "cleanup" gets passed to some generic scheduler thingy), which is even more complex than this trivial example. In one of the cases the unlocking would happen on some other thread, which I guess is a little suspicious, but should still be guaranteed to execute (I didn't dig _too_ deep there). Anyway, I also suppressed analysis here.

3. Missed lock annotation on the constructor, take 1

  struct Foo {
    explicit Foo(Mutex *mu) EXCLUSIVE_LOCKS_REQUIRED(mu);
  };
  
  Something Func() {
    SomethingLikeMutexLock lock(&mu);
    lock.Release(); // !!!
    return Get(Foo(&mu));
  }

Glaringly obvious bug that wasn't being caught before this patch. Moving the call to `Foo(&mu)` earlier while the lock is still held fixes the bug.

4. Missed lock annotation on the constructor, take 2

  struct Foo {
    explicit Foo(Mutex *mu) EXCLUSIVE_LOCKS_REQUIRED(mu);
  };
  void X() {
    auto lock = MutexLock(&mu);
    auto f = MakeFoo();
  }
  Foo Y() {
    return Foo(&mu);
  }

`X` grabs the lock, but `Y` warns that we don't have `mu` held. In this case, `mu->AssertHeld()` suffices (the mutex pattern is a little more complicated, and the pattern is used elsewhere, but was missed in this method).

5. Alternate `std::make_unique` implementation (this one still has me puzzled)

  template <typename T, typename... Args>
  inline std::unique_ptr<T> MyMakeUnique(Args &&...args) {
    return std::unique_ptr<T>(new T(std::forward<Args>(args)...));
  }
  
  static Mutex y_mu;
  
  class Z {
   public:
    Z() EXCLUSIVE_LOCKS_REQUIRED(y_mu) {}
  };
  
  std::unique_ptr<Z> Y() {
    auto lock = MutexLock(&y_mu);
    return MyMakeUnique<Z>();
  }

Returning  `std::make_unique<Z>()` instead of the custom helper works. Why? No idea. `MyMakeUnique` is a c++11 compatibility helper -- std::unique_ptr is in c++11 but std::make_unique is only in c++14 -- and fortunately this code doesn't need it anymore. The implementation seems to be identical to the one in libc++, so my best guess is threading analysis has some special handling for some std:: methods.

I might have a better answer in a day or two of how widespread this is beyond just the core files that seem to make the world break when we enable this. We can just fix the bugs it if it's only a few of them, but it might be difficult if we have too many.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129755