[Lldb-commits] [PATCH] D157760: [lldb] Properly protect the Communication class with reader/writer lock

[Augusto Noronha via Phabricator via lldb-commits]

augusto2112 added inline comments.


================
Comment at: lldb/include/lldb/Core/Communication.h:86-89
+  lldb_private::Connection *GetConnection() {
+    std::shared_lock guard(m_connection_mutex);
+    return m_connection_sp.get();
+  }
----------------
bulbazord wrote:
> This isn't necessarily introduced by your patch, but I don't think this lock does much other than make sure nobody can change `m_connection_sp` in between you calling `GetConnection` and you actually receiving the `Connection` pointer. It still comes with all the pitfalls of handing out a raw pointer (instead of a `shared_ptr`).
> 
> As a follow-up or maybe even precursor, we should probably stop handing out the raw connection pointer...
I agree. I think we should do it after this patch though.


================
Comment at: lldb/include/lldb/Core/Communication.h:172-174
   std::mutex
       m_write_mutex; ///< Don't let multiple threads write at the same time...
+  mutable std::shared_mutex m_connection_mutex;
----------------
JDevlieghere wrote:
> Why do we need both? Can't we use `m_connection_mutex` in write mode instead of `m_write_mutex`?
If we use `m_connection_mutex` in write mode instead of m_write_mutex then we can't have concurrent reads and writes.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157760/new/

https://reviews.llvm.org/D157760