Skip to content

Commit dd526d8

Browse files
authored
Merge pull request #10261 from swiftlang/cherrypick-stable/20240723-e823449f66ac-998511c8ef57
Cherrypick debugserver race condition fixes
2 parents 37f3918 + 4ee2fa4 commit dd526d8

File tree

3 files changed

+84
-82
lines changed

3 files changed

+84
-82
lines changed

lldb/tools/debugserver/source/MacOSX/MachProcess.h

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -427,16 +427,16 @@ class MachProcess {
427427
m_profile_data_mutex; // Multithreaded protection for profile info data
428428
std::vector<std::string>
429429
m_profile_data; // Profile data, must be protected by m_profile_data_mutex
430-
PThreadEvent m_profile_events; // Used for the profile thread cancellable wait
430+
PThreadEvent m_profile_events; // Used for the profile thread cancellable wait
431431
DNBThreadResumeActions m_thread_actions; // The thread actions for the current
432432
// MachProcess::Resume() call
433433
MachException::Message::collection m_exception_messages; // A collection of
434434
// exception messages
435435
// caught when
436436
// listening to the
437437
// exception port
438-
PThreadMutex m_exception_messages_mutex; // Multithreaded protection for
439-
// m_exception_messages
438+
PThreadMutex m_exception_and_signal_mutex; // Multithreaded protection for
439+
// exceptions and signals.
440440

441441
MachThreadList m_thread_list; // A list of threads that is maintained/updated
442442
// after each stop

lldb/tools/debugserver/source/MacOSX/MachProcess.mm

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -528,7 +528,7 @@ static bool FBSAddEventDataToOptions(NSMutableDictionary *options,
528528
m_profile_data_mutex(PTHREAD_MUTEX_RECURSIVE), m_profile_data(),
529529
m_profile_events(0, eMachProcessProfileCancel), m_thread_actions(),
530530
m_exception_messages(),
531-
m_exception_messages_mutex(PTHREAD_MUTEX_RECURSIVE), m_thread_list(),
531+
m_exception_and_signal_mutex(PTHREAD_MUTEX_RECURSIVE), m_thread_list(),
532532
m_activities(), m_state(eStateUnloaded),
533533
m_state_mutex(PTHREAD_MUTEX_RECURSIVE), m_events(0, kAllEventsMask),
534534
m_private_events(0, kAllEventsMask), m_breakpoints(), m_watchpoints(),
@@ -1338,8 +1338,11 @@ static bool mach_header_validity_test(uint32_t magic, uint32_t cputype) {
13381338
m_stop_count = 0;
13391339
m_thread_list.Clear();
13401340
{
1341-
PTHREAD_MUTEX_LOCKER(locker, m_exception_messages_mutex);
1341+
PTHREAD_MUTEX_LOCKER(locker, m_exception_and_signal_mutex);
13421342
m_exception_messages.clear();
1343+
m_sent_interrupt_signo = 0;
1344+
m_auto_resume_signo = 0;
1345+
13431346
}
13441347
m_activities.Clear();
13451348
StopProfileThread();
@@ -1575,6 +1578,7 @@ static uint64_t bits(uint64_t value, uint32_t msbit, uint32_t lsbit) {
15751578
bool MachProcess::Interrupt() {
15761579
nub_state_t state = GetState();
15771580
if (IsRunning(state)) {
1581+
PTHREAD_MUTEX_LOCKER(locker, m_exception_and_signal_mutex);
15781582
if (m_sent_interrupt_signo == 0) {
15791583
m_sent_interrupt_signo = SIGSTOP;
15801584
if (Signal(m_sent_interrupt_signo)) {
@@ -1728,7 +1732,7 @@ static uint64_t bits(uint64_t value, uint32_t msbit, uint32_t lsbit) {
17281732
m_thread_actions.Append(thread_action);
17291733
m_thread_actions.SetDefaultThreadActionIfNeeded(eStateRunning, 0);
17301734

1731-
PTHREAD_MUTEX_LOCKER(locker, m_exception_messages_mutex);
1735+
PTHREAD_MUTEX_LOCKER(locker, m_exception_and_signal_mutex);
17321736

17331737
ReplyToAllExceptions();
17341738
}
@@ -1854,7 +1858,7 @@ static uint64_t bits(uint64_t value, uint32_t msbit, uint32_t lsbit) {
18541858
}
18551859

18561860
void MachProcess::ReplyToAllExceptions() {
1857-
PTHREAD_MUTEX_LOCKER(locker, m_exception_messages_mutex);
1861+
PTHREAD_MUTEX_LOCKER(locker, m_exception_and_signal_mutex);
18581862
if (!m_exception_messages.empty()) {
18591863
MachException::Message::iterator pos;
18601864
MachException::Message::iterator begin = m_exception_messages.begin();
@@ -1888,7 +1892,7 @@ static uint64_t bits(uint64_t value, uint32_t msbit, uint32_t lsbit) {
18881892
}
18891893
}
18901894
void MachProcess::PrivateResume() {
1891-
PTHREAD_MUTEX_LOCKER(locker, m_exception_messages_mutex);
1895+
PTHREAD_MUTEX_LOCKER(locker, m_exception_and_signal_mutex);
18921896

18931897
m_auto_resume_signo = m_sent_interrupt_signo;
18941898
if (m_auto_resume_signo)
@@ -2290,7 +2294,7 @@ static uint64_t bits(uint64_t value, uint32_t msbit, uint32_t lsbit) {
22902294
// data has already been copied.
22912295
void MachProcess::ExceptionMessageReceived(
22922296
const MachException::Message &exceptionMessage) {
2293-
PTHREAD_MUTEX_LOCKER(locker, m_exception_messages_mutex);
2297+
PTHREAD_MUTEX_LOCKER(locker, m_exception_and_signal_mutex);
22942298

22952299
if (m_exception_messages.empty())
22962300
m_task.Suspend();
@@ -2304,7 +2308,7 @@ static uint64_t bits(uint64_t value, uint32_t msbit, uint32_t lsbit) {
23042308

23052309
task_t MachProcess::ExceptionMessageBundleComplete() {
23062310
// We have a complete bundle of exceptions for our child process.
2307-
PTHREAD_MUTEX_LOCKER(locker, m_exception_messages_mutex);
2311+
PTHREAD_MUTEX_LOCKER(locker, m_exception_and_signal_mutex);
23082312
DNBLogThreadedIf(LOG_EXCEPTIONS, "%s: %llu exception messages.",
23092313
__PRETTY_FUNCTION__, (uint64_t)m_exception_messages.size());
23102314
bool auto_resume = false;

lldb/tools/debugserver/source/RNBRemote.cpp

Lines changed: 70 additions & 72 deletions
Original file line numberDiff line numberDiff line change
@@ -1054,84 +1054,82 @@ rnb_err_t RNBRemote::HandleReceivedPacket(PacketEnum *type) {
10541054
void RNBRemote::CommDataReceived(const std::string &new_data) {
10551055
// DNBLogThreadedIf (LOG_RNB_REMOTE, "%8d RNBRemote::%s called",
10561056
// (uint32_t)m_comm.Timer().ElapsedMicroSeconds(true), __FUNCTION__);
1057-
{
1058-
// Put the packet data into the buffer in a thread safe fashion
1059-
PThreadMutex::Locker locker(m_mutex);
1060-
1061-
std::string data;
1062-
// See if we have any left over data from a previous call to this
1063-
// function?
1064-
if (!m_rx_partial_data.empty()) {
1065-
// We do, so lets start with that data
1066-
data.swap(m_rx_partial_data);
1067-
}
1068-
// Append the new incoming data
1069-
data += new_data;
1070-
1071-
// Parse up the packets into gdb remote packets
1072-
size_t idx = 0;
1073-
const size_t data_size = data.size();
1074-
1075-
while (idx < data_size) {
1076-
// end_idx must be one past the last valid packet byte. Start
1077-
// it off with an invalid value that is the same as the current
1078-
// index.
1079-
size_t end_idx = idx;
1080-
1081-
switch (data[idx]) {
1082-
case '+': // Look for ack
1083-
case '-': // Look for cancel
1084-
case '\x03': // ^C to halt target
1085-
end_idx = idx + 1; // The command is one byte long...
1086-
break;
10871057

1088-
case '$':
1089-
// Look for a standard gdb packet?
1090-
end_idx = data.find('#', idx + 1);
1091-
if (end_idx == std::string::npos || end_idx + 3 > data_size) {
1092-
end_idx = std::string::npos;
1093-
} else {
1094-
// Add two for the checksum bytes and 1 to point to the
1095-
// byte just past the end of this packet
1096-
end_idx += 3;
1097-
}
1098-
break;
1058+
// Put the packet data into the buffer in a thread safe fashion
1059+
PThreadMutex::Locker locker(m_mutex);
1060+
1061+
std::string data;
1062+
// See if we have any left over data from a previous call to this
1063+
// function?
1064+
if (!m_rx_partial_data.empty()) {
1065+
// We do, so lets start with that data
1066+
data.swap(m_rx_partial_data);
1067+
}
1068+
// Append the new incoming data
1069+
data += new_data;
1070+
1071+
// Parse up the packets into gdb remote packets
1072+
size_t idx = 0;
1073+
const size_t data_size = data.size();
1074+
1075+
while (idx < data_size) {
1076+
// end_idx must be one past the last valid packet byte. Start
1077+
// it off with an invalid value that is the same as the current
1078+
// index.
1079+
size_t end_idx = idx;
1080+
1081+
switch (data[idx]) {
1082+
case '+': // Look for ack
1083+
case '-': // Look for cancel
1084+
case '\x03': // ^C to halt target
1085+
end_idx = idx + 1; // The command is one byte long...
1086+
break;
10991087

1100-
default:
1101-
break;
1088+
case '$':
1089+
// Look for a standard gdb packet?
1090+
end_idx = data.find('#', idx + 1);
1091+
if (end_idx == std::string::npos || end_idx + 3 > data_size) {
1092+
end_idx = std::string::npos;
1093+
} else {
1094+
// Add two for the checksum bytes and 1 to point to the
1095+
// byte just past the end of this packet
1096+
end_idx += 3;
11021097
}
1098+
break;
11031099

1104-
if (end_idx == std::string::npos) {
1105-
// Not all data may be here for the packet yet, save it for
1106-
// next time through this function.
1107-
m_rx_partial_data += data.substr(idx);
1108-
// DNBLogThreadedIf (LOG_RNB_MAX, "%8d RNBRemote::%s saving data for
1109-
// later[%u, npos):
1110-
// '%s'",(uint32_t)m_comm.Timer().ElapsedMicroSeconds(true),
1111-
// __FUNCTION__, idx, m_rx_partial_data.c_str());
1112-
idx = end_idx;
1113-
} else if (idx < end_idx) {
1114-
m_packets_recvd++;
1115-
// Hack to get rid of initial '+' ACK???
1116-
if (m_packets_recvd == 1 && (end_idx == idx + 1) && data[idx] == '+') {
1117-
// DNBLogThreadedIf (LOG_RNB_REMOTE, "%8d RNBRemote::%s throwing first
1118-
// ACK away....[%u, npos):
1119-
// '+'",(uint32_t)m_comm.Timer().ElapsedMicroSeconds(true),
1120-
// __FUNCTION__, idx);
1121-
} else {
1122-
// We have a valid packet...
1123-
m_rx_packets.push_back(data.substr(idx, end_idx - idx));
1124-
DNBLogThreadedIf(LOG_RNB_PACKETS, "getpkt: %s",
1125-
m_rx_packets.back().c_str());
1126-
}
1127-
idx = end_idx;
1100+
default:
1101+
break;
1102+
}
1103+
1104+
if (end_idx == std::string::npos) {
1105+
// Not all data may be here for the packet yet, save it for
1106+
// next time through this function.
1107+
m_rx_partial_data += data.substr(idx);
1108+
// DNBLogThreadedIf (LOG_RNB_MAX, "%8d RNBRemote::%s saving data for
1109+
// later[%u, npos):
1110+
// '%s'",(uint32_t)m_comm.Timer().ElapsedMicroSeconds(true),
1111+
// __FUNCTION__, idx, m_rx_partial_data.c_str());
1112+
idx = end_idx;
1113+
} else if (idx < end_idx) {
1114+
m_packets_recvd++;
1115+
// Hack to get rid of initial '+' ACK???
1116+
if (m_packets_recvd == 1 && (end_idx == idx + 1) && data[idx] == '+') {
1117+
// DNBLogThreadedIf (LOG_RNB_REMOTE, "%8d RNBRemote::%s throwing first
1118+
// ACK away....[%u, npos):
1119+
// '+'",(uint32_t)m_comm.Timer().ElapsedMicroSeconds(true),
1120+
// __FUNCTION__, idx);
11281121
} else {
1129-
DNBLogThreadedIf(LOG_RNB_MAX,
1130-
"%8d RNBRemote::%s tossing junk byte at %c",
1131-
(uint32_t)m_comm.Timer().ElapsedMicroSeconds(true),
1132-
__FUNCTION__, data[idx]);
1133-
idx = idx + 1;
1122+
// We have a valid packet...
1123+
m_rx_packets.push_back(data.substr(idx, end_idx - idx));
1124+
DNBLogThreadedIf(LOG_RNB_PACKETS, "getpkt: %s",
1125+
m_rx_packets.back().c_str());
11341126
}
1127+
idx = end_idx;
1128+
} else {
1129+
DNBLogThreadedIf(LOG_RNB_MAX, "%8d RNBRemote::%s tossing junk byte at %c",
1130+
(uint32_t)m_comm.Timer().ElapsedMicroSeconds(true),
1131+
__FUNCTION__, data[idx]);
1132+
idx = idx + 1;
11351133
}
11361134
}
11371135

0 commit comments

Comments
 (0)