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