[Lldb-commits] [PATCH] D158035: [lldb] Protect RNBRemote from a data race

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added inline comments.



Comment at: lldb/tools/debugserver/source/RNBRemote.cpp:780
 
   PThreadMutex::Locker locker(m_mutex);
   if (m_rx_packets.empty()) {

JDevlieghere wrote:
> This is an RAII object, right? Can we just block scope it? Right now it looks 
> like we might not unlock if we return early on line 787. 
I agree with the suggested change.  But yeah I think putting the part of this 
method that touches m_rx_packets in a block, and holding the lock in that 
block, would be best, make it a little clearer.   As for the early return on 
787, won't locker release the lock in its dtor?  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158035

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D158035: [lldb] Protect RNBRemote from a data race

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.



Comment at: lldb/tools/debugserver/source/RNBRemote.cpp:780
 
   PThreadMutex::Locker locker(m_mutex);
   if (m_rx_packets.empty()) {

This is an RAII object, right? Can we just block scope it? Right now it looks 
like we might not unlock if we return early on line 787. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158035

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D158035: [lldb] Protect RNBRemote from a data race

[Augusto Noronha via Phabricator via lldb-commits]

augusto2112 created this revision.
augusto2112 added reviewers: JDevlieghere, bulbazord, jingham.
Herald added a subscriber: kristof.beyls.
Herald added a project: All.
augusto2112 requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Thread sanitizer reports the following data race:

  Write of size 8 at 0x000103303e70 by thread T1 (mutexes: write M0):
#0 RNBRemote::CommDataReceived(std::__1::basic_string, std::__1::allocator> const&) 
RNBRemote.cpp:1075 (debugserver:arm64+0x100038db8) (BuildId: 
f130b34f693c4f3eba96139104af2b71320021000e00)
#1 RNBRemote::ThreadFunctionReadRemoteData(void*) RNBRemote.cpp:1180 
(debugserver:arm64+0x1000391dc) (BuildId: 
f130b34f693c4f3eba96139104af2b71320021000e00)
  
  Previous read of size 8 at 0x000103303e70 by main thread:
#0 RNBRemote::GetPacketPayload(std::__1::basic_string, std::__1::allocator>&) RNBRemote.cpp:797 
(debugserver:arm64+0x100037c5c) (BuildId: 
f130b34f693c4f3eba96139104af2b71320021000e00)
#1 RNBRemote::GetPacket(std::__1::basic_string, std::__1::allocator>&, RNBRemote::Packet&, 
bool) RNBRemote.cpp:907 (debugserver:arm64+0x1000378cc) (BuildId: 
f130b34f693c4f3eba96139104af2b71320021000e00)

RNBRemote already has a mutex, extend its usage to protect the read of
m_rx_packets.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D158035

Files:
  lldb/tools/debugserver/source/RNBRemote.cpp


Index: lldb/tools/debugserver/source/RNBRemote.cpp
===
--- lldb/tools/debugserver/source/RNBRemote.cpp
+++ lldb/tools/debugserver/source/RNBRemote.cpp
@@ -792,12 +792,12 @@
   // m_rx_packets.size());
   return_packet.swap(m_rx_packets.front());
   m_rx_packets.pop_front();
-  locker.Reset(); // Release our lock on the mutex
 
   if (m_rx_packets.empty()) {
 // Reset the remote command available event if we have no more packets
 m_ctx.Events().ResetEvents(RNBContext::event_read_packet_available);
   }
+  locker.Reset(); // Release our lock on the mutex
 
   // DNBLogThreadedIf (LOG_RNB_MEDIUM, "%8u RNBRemote::%s: '%s'",
   // (uint32_t)m_comm.Timer().ElapsedMicroSeconds(true), __FUNCTION__,


Index: lldb/tools/debugserver/source/RNBRemote.cpp
===
--- lldb/tools/debugserver/source/RNBRemote.cpp
+++ lldb/tools/debugserver/source/RNBRemote.cpp
@@ -792,12 +792,12 @@
   // m_rx_packets.size());
   return_packet.swap(m_rx_packets.front());
   m_rx_packets.pop_front();
-  locker.Reset(); // Release our lock on the mutex
 
   if (m_rx_packets.empty()) {
 // Reset the remote command available event if we have no more packets
 m_ctx.Events().ResetEvents(RNBContext::event_read_packet_available);
   }
+  locker.Reset(); // Release our lock on the mutex
 
   // DNBLogThreadedIf (LOG_RNB_MEDIUM, "%8u RNBRemote::%s: '%s'",
   // (uint32_t)m_comm.Timer().ElapsedMicroSeconds(true), __FUNCTION__,
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits