aaron.ballman 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.
I'm not opposed to a revert here, but reverting on (plausible) assumptions of
this being a false positive without discussion or a reproducer beyond "compile
chromium" comes across as a somewhat aggressive revert. Especially given that
we knew this had the potential to break code and we were okay with that code
being broken (as stated in the commit message). No community bots went red from
this change and it's possible this could have been fixed forward/explained as a
desired break with less churn to the codebase. Nothing to be done for it now
(and it wasn't wrong per se), just something to keep in mind for future reverts
-- a bit of discussion before a revert like this would be appropriate.
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
