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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
