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

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

Thanks for the detailed write-up, very much appreciated.

In D129755#3866887 <https://reviews.llvm.org/D129755#3866887>, @rupprecht wrote:

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

I'm planning to address this, but don't have a good idea how to solve it yet. Conceptually it should definitely be possible though.

> Anyway, I applied `__attribute__((no_thread_safety_analysis))` to the method.

This seems about right for now. I filed bug 58480 <https://github.com/llvm/llvm-project/issues/58480>, which you could link to in a comment or subscribe to for being notified of a fix.

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

Yes, this is pretty much out of scope, at least for the foreseeable future. This is also documented: https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#no-inlining.

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

Glad to hear that!

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

Possibly you could also "propagate" the lock requirement onto `MakeFoo` like this:

  Foo MakeFoo() EXCLUSIVE_LOCKS_REQUIRED(mu) {
    return Foo(&mu);
  }

But sometimes this isn't possible for encapsulation reasons, in which case `AssertHeld` is indeed a good workaround.

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

We had a similar case earlier in the thread. Basically the problem is that `MyMakeUnique` calls the constructor of `Z` without holding the lock (statically). I don't think that we have special handling for `std::`, but it's possible that the warning is suppressed in system headers.

Apart from the solutions mentioned above, manually inlining the `make_unique`/`MyMakeUnique` should do the job, i.e.

  std::unique_ptr<Z> Y() {
    auto lock = MutexLock(&y_mu);
    return std::unique_ptr<Z>(new Z());
  }

Of course this has downsides of its own, but we can't really do "inlining" in a compiler warning and so it's either the warning or (slightly) more verbose code.

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

The good news is that for now we've only seen the second category of issues, for which a flag to restore the old behavior would be feasible. I can't say for certain whether that would make all the issues here disappear, but it definitely seems so.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129755