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

[Aaron Ballman via Phabricator via cfe-commits]

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