Skip to content

[debugserver] Fix mutex scope in RNBRemote::CommDataReceived #131077

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Mar 13, 2025

Conversation

JDevlieghere
Copy link
Member

The mutex in RNBRemote::CommDataReceived protects m_rx_packets and should extend to the end of the function to cover the read where we check if the list is empty.

The mutex in RNBRemote::CommDataReceived protects m_rx_packets and
should extend to the end of the function to cover the read where we
check if the list is empty.
@llvmbot
Copy link
Member

llvmbot commented Mar 13, 2025

@llvm/pr-subscribers-lldb

Author: Jonas Devlieghere (JDevlieghere)

Changes

The mutex in RNBRemote::CommDataReceived protects m_rx_packets and should extend to the end of the function to cover the read where we check if the list is empty.


Full diff: https://github.com/llvm/llvm-project/pull/131077.diff

1 Files Affected:

  • (modified) lldb/tools/debugserver/source/RNBRemote.cpp (+1-2)
diff --git a/lldb/tools/debugserver/source/RNBRemote.cpp b/lldb/tools/debugserver/source/RNBRemote.cpp
index 8a53094429aba..dd6da3db11814 100644
--- a/lldb/tools/debugserver/source/RNBRemote.cpp
+++ b/lldb/tools/debugserver/source/RNBRemote.cpp
@@ -1054,7 +1054,7 @@ rnb_err_t RNBRemote::HandleReceivedPacket(PacketEnum *type) {
 void RNBRemote::CommDataReceived(const std::string &new_data) {
   //  DNBLogThreadedIf (LOG_RNB_REMOTE, "%8d RNBRemote::%s called",
   //  (uint32_t)m_comm.Timer().ElapsedMicroSeconds(true), __FUNCTION__);
-  {
+
     // Put the packet data into the buffer in a thread safe fashion
     PThreadMutex::Locker locker(m_mutex);
 
@@ -1132,7 +1132,6 @@ void RNBRemote::CommDataReceived(const std::string &new_data) {
                          __FUNCTION__, data[idx]);
         idx = idx + 1;
       }
-    }
   }
 
   if (!m_rx_packets.empty()) {

@@ -1054,7 +1054,7 @@ rnb_err_t RNBRemote::HandleReceivedPacket(PacketEnum *type) {
void RNBRemote::CommDataReceived(const std::string &new_data) {
// DNBLogThreadedIf (LOG_RNB_REMOTE, "%8d RNBRemote::%s called",
// (uint32_t)m_comm.Timer().ElapsedMicroSeconds(true), __FUNCTION__);
{

// Put the packet data into the buffer in a thread safe fashion
PThreadMutex::Locker locker(m_mutex);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should you also fix the indentation?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I don't know why clang-format isn't kicking in...

Copy link

github-actions bot commented Mar 13, 2025

✅ With the latest revision this PR passed the C/C++ code formatter.

Copy link
Collaborator

@jasonmolenda jasonmolenda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes looks correct to me.

@JDevlieghere JDevlieghere merged commit 998511c into llvm:main Mar 13, 2025
8 of 9 checks passed
@JDevlieghere JDevlieghere deleted the debugserver-race-2 branch March 13, 2025 22:12
JDevlieghere added a commit to swiftlang/llvm-project that referenced this pull request Mar 14, 2025
…1077)

The mutex in RNBRemote::CommDataReceived protects m_rx_packets and
should extend to the end of the function to cover the read where we
check if the list is empty.

(cherry picked from commit 998511c)
frederik-h pushed a commit to frederik-h/llvm-project that referenced this pull request Mar 18, 2025
…1077)

The mutex in RNBRemote::CommDataReceived protects m_rx_packets and
should extend to the end of the function to cover the read where we
check if the list is empty.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants