aaronpuchert added a comment.
In D129755#3842729 <https://reviews.llvm.org/D129755#3842729>, @hans wrote:
> We're hitting a false positive in grpc after this:
>
> > ../../third_party/grpc/src/src/core/lib/gprpp/ref_counted_ptr.h:335:31:
> error: calling function 'TlsSessionKeyLoggerCache' requires holding mutex
> 'g_tls_session_key_log_cache_mu' exclusively
> [-Werror,-Wthread-safety-analysis]
> > return RefCountedPtr<T>(new T(std::forward<Args>(args)...));
> > ^
> >
> ../../third_party/grpc/src/src/core/tsi/ssl/key_logging/ssl_key_logging.cc:121:26:
> note: in instantiation of function template specialization
> 'grpc_core::MakeRefCounted<tsi::TlsSessionKeyLoggerCache>' requested here
> > cache = grpc_core::MakeRefCounted<TlsSessionKeyLoggerCache>();
> > ^
>
>
> The code looks like this:
>
> grpc_core::RefCountedPtr<TlsSessionKeyLogger> TlsSessionKeyLoggerCache::Get(
> std::string tls_session_key_log_file_path) {
> gpr_once_init(&g_cache_mutex_init, do_cache_mutex_init);
> GPR_DEBUG_ASSERT(g_tls_session_key_log_cache_mu != nullptr);
> if (tls_session_key_log_file_path.empty()) {
> return nullptr;
> }
> {
> grpc_core::MutexLock lock(g_tls_session_key_log_cache_mu);
> <---------- holding the mutex
> grpc_core::RefCountedPtr<TlsSessionKeyLoggerCache> cache;
> if (g_cache_instance == nullptr) {
> // This will automatically set g_cache_instance.
> cache = grpc_core::MakeRefCounted<TlsSessionKeyLoggerCache>();
> <------ line 121
>
>
>
> lock is holding a MutexLock (I assume that's an exclusive thing) on
> g_tls_session_key_log_cache_mu.
>
> See https://bugs.chromium.org/p/chromium/issues/detail?id=1372394#c4 for how
> to reproduce.
>
> I've reverted this in
> https://github.com/llvm/llvm-project/commit/a4afa2bde6f4db215ddd3267a8d11c04367812e5
> in the meantime.
This doesn't look like a false positive to me. The documentation states that no
inlining <https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#no-inlining> is
being done, so unless `grpc_core::MakeRefCounted` (or a specialization) has a
`require_capability` attribute, the lock doesn't count as locked in the
function body. That has always been the case.
The only thing that changed is that we now also consider `CXXConstructExpr`
outside of `DeclStmt`s, which is arguably an improvement.
In D129755#3842744 <https://reviews.llvm.org/D129755#3842744>, @hans wrote:
> We also hit a different case:
> https://bugs.chromium.org/p/chromium/issues/detail?id=1372394#c6
From the logs:
../../extensions/browser/api/content_settings/content_settings_store.cc(75,49):
note: mutex acquired here
std::unique_ptr<base::AutoLock> auto_lock(new base::AutoLock(lock_));
^
That seems to be precisely the example that the documentation says doesn't
work. The constructor of `std::unique_ptr` has no thread safety annotations and
so we have a constructor call without a matching destructor call. (The
destructor is called indirectly from the destructor of `unique_ptr`. We see
this now because of the constructor call that doesn't initialize a local
variable.
The documentation also lists ways to make this work (such as having more
powerful scope types).
I'll give you some time to reply before I reland, but I can't see a bug here.
For the next time, as already pointed out, give the involved people some time
to reply before you revert (unless it's a crash). We have to be able to
continue working on the compiler without having Google revert changes all the
time because we produce new warnings.
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
