Skip to content

Commit 0faf373

Browse files
labathlabath
committedAug 25, 2016
gdb-remote: Make the sequence mutex non-recursive
Summary: This is a preparatory commit for D22914, where I'd like to replace this mutex by an R/W lock (which is also not recursive). This required a couple of changes: - The only caller of Read/WriteRegister, GDBRemoteRegisterContext class, was already acquiring the mutex, so these functions do not need to. All functions which now do not take a lock, take an lock argument instead, to remind the caller of this fact. - GetThreadSuffixSupported() was being called from locked and unlocked contexts (including contexts where the process was running, and the call would fail if it did not have the result cached). I have split this into two functions, one which computes the thread suffix support and caches it (this one always takes the lock), and another, which returns the cached value (and never needs to take the lock). This feels quite natural as ProcessGdbRemote was already pre-caching this value at the start. Reviewers: clayborg Subscribers: lldb-commits Differential Revision: https://reviews.llvm.org/D23802 llvm-svn: 279725
·
llvmorg-21-initllvmorg-4.0.0-rc1
1 parent c156630 commit 0faf373

8 files changed

+164
-101
lines changed
 

‎lldb/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp

+4-2
Original file line numberDiff line numberDiff line change
@@ -208,12 +208,14 @@ GDBRemoteClientBase::SendPacketAndWaitForResponse(llvm::StringRef payload, Strin
208208
return PacketResult::ErrorSendFailed;
209209
}
210210

211-
return SendPacketAndWaitForResponseNoLock(payload, response);
211+
return SendPacketAndWaitForResponse(payload, response, lock);
212212
}
213213

214214
GDBRemoteCommunication::PacketResult
215-
GDBRemoteClientBase::SendPacketAndWaitForResponseNoLock(llvm::StringRef payload, StringExtractorGDBRemote &response)
215+
GDBRemoteClientBase::SendPacketAndWaitForResponse(llvm::StringRef payload, StringExtractorGDBRemote &response,
216+
const Lock &lock)
216217
{
218+
assert(lock);
217219
PacketResult packet_result = SendPacketNoLock(payload.data(), payload.size());
218220
if (packet_result != PacketResult::Success)
219221
return packet_result;

‎lldb/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.h

+4-4
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,7 @@ class GDBRemoteClientBase : public GDBRemoteCommunication
7373
Lock(GDBRemoteClientBase &comm, bool interrupt);
7474
~Lock();
7575

76-
explicit operator bool() { return m_acquired; }
76+
explicit operator bool() const { return m_acquired; }
7777

7878
// Whether we had to interrupt the continue thread to acquire the connection.
7979
bool
@@ -83,7 +83,7 @@ class GDBRemoteClientBase : public GDBRemoteCommunication
8383
}
8484

8585
private:
86-
std::unique_lock<std::recursive_mutex> m_async_lock;
86+
std::unique_lock<std::mutex> m_async_lock;
8787
GDBRemoteClientBase &m_comm;
8888
bool m_acquired;
8989
bool m_did_interrupt;
@@ -94,7 +94,7 @@ class GDBRemoteClientBase : public GDBRemoteCommunication
9494

9595
protected:
9696
PacketResult
97-
SendPacketAndWaitForResponseNoLock(llvm::StringRef payload, StringExtractorGDBRemote &response);
97+
SendPacketAndWaitForResponse(llvm::StringRef payload, StringExtractorGDBRemote &response, const Lock &lock);
9898

9999
virtual void
100100
OnRunPacketSent(bool first);
@@ -126,7 +126,7 @@ class GDBRemoteClientBase : public GDBRemoteCommunication
126126

127127
// This handles the synchronization between individual async threads. For now they just use a
128128
// simple mutex.
129-
std::recursive_mutex m_async_mutex;
129+
std::mutex m_async_mutex;
130130

131131
bool
132132
ShouldStop(const UnixSignals &signals, StringExtractorGDBRemote &response);

‎lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp

+66-41
Original file line numberDiff line numberDiff line change
@@ -516,21 +516,27 @@ GDBRemoteCommunicationClient::GetRemoteQSupported ()
516516
}
517517
}
518518

519-
bool
520-
GDBRemoteCommunicationClient::GetThreadSuffixSupported ()
519+
void
520+
GDBRemoteCommunicationClient::ComputeThreadSuffixSupport()
521521
{
522-
if (m_supports_thread_suffix == eLazyBoolCalculate)
522+
if (m_supports_thread_suffix != eLazyBoolCalculate)
523+
return;
524+
525+
StringExtractorGDBRemote response;
526+
m_supports_thread_suffix = eLazyBoolNo;
527+
if (SendPacketAndWaitForResponse("QThreadSuffixSupported", response, false) == PacketResult::Success)
523528
{
524-
StringExtractorGDBRemote response;
525-
m_supports_thread_suffix = eLazyBoolNo;
526-
if (SendPacketAndWaitForResponse("QThreadSuffixSupported", response, false) == PacketResult::Success)
527-
{
528-
if (response.IsOKResponse())
529-
m_supports_thread_suffix = eLazyBoolYes;
530-
}
529+
if (response.IsOKResponse())
530+
m_supports_thread_suffix = eLazyBoolYes;
531531
}
532-
return m_supports_thread_suffix;
533532
}
533+
534+
bool
535+
GDBRemoteCommunicationClient::GetThreadSuffixSupported()
536+
{
537+
return m_supports_thread_suffix == eLazyBoolYes;
538+
}
539+
534540
bool
535541
GDBRemoteCommunicationClient::GetVContSupported (char flavor)
536542
{
@@ -592,26 +598,17 @@ GDBRemoteCommunicationClient::GetVContSupported (char flavor)
592598
GDBRemoteCommunication::PacketResult
593599
GDBRemoteCommunicationClient::SendThreadSpecificPacketAndWaitForResponse(lldb::tid_t tid, StreamString &&payload,
594600
StringExtractorGDBRemote &response,
595-
bool send_async)
601+
const Lock &lock)
596602
{
597-
Lock lock(*this, send_async);
598-
if (!lock)
599-
{
600-
if (Log *log = ProcessGDBRemoteLog::GetLogIfAnyCategoryIsSet(GDBR_LOG_PROCESS | GDBR_LOG_PACKETS))
601-
log->Printf("GDBRemoteCommunicationClient::%s: Didn't get sequence mutex for %s packet.", __FUNCTION__,
602-
payload.GetString().c_str());
603-
return PacketResult::ErrorNoSequenceLock;
604-
}
605-
606603
if (GetThreadSuffixSupported())
607604
payload.Printf(";thread:%4.4" PRIx64 ";", tid);
608605
else
609606
{
610-
if (!SetCurrentThread(tid))
607+
if (!SetCurrentThread(tid, lock))
611608
return PacketResult::ErrorSendFailed;
612609
}
613610

614-
return SendPacketAndWaitForResponseNoLock(payload.GetString(), response);
611+
return SendPacketAndWaitForResponse(payload.GetString(), response, lock);
615612
}
616613

617614
// Check if the target supports 'p' packet. It sends out a 'p'
@@ -624,11 +621,20 @@ GDBRemoteCommunicationClient::GetpPacketSupported (lldb::tid_t tid)
624621
{
625622
if (m_supports_p == eLazyBoolCalculate)
626623
{
624+
Lock lock(*this, false);
625+
if (!lock)
626+
{
627+
Log *log(ProcessGDBRemoteLog::GetLogIfAnyCategoryIsSet(GDBR_LOG_PROCESS | GDBR_LOG_PACKETS));
628+
if (log)
629+
log->Printf("GDBRemoteCommunicationClient::%s failed to get sequence mutex", __FUNCTION__);
630+
return false;
631+
}
632+
627633
m_supports_p = eLazyBoolNo;
628634
StreamString payload;
629635
payload.PutCString("p0");
630636
StringExtractorGDBRemote response;
631-
if (SendThreadSpecificPacketAndWaitForResponse(tid, std::move(payload), response, false) ==
637+
if (SendThreadSpecificPacketAndWaitForResponse(tid, std::move(payload), response, lock) ==
632638
PacketResult::Success &&
633639
response.IsNormalResponse())
634640
{
@@ -766,7 +772,7 @@ GDBRemoteCommunicationClient::SendPacketsAndConcatenateResponses
766772
// Construct payload
767773
char sizeDescriptor[128];
768774
snprintf(sizeDescriptor, sizeof(sizeDescriptor), "%x,%x", offset, response_size);
769-
PacketResult result = SendPacketAndWaitForResponseNoLock(payload_prefix_str + sizeDescriptor, this_response);
775+
PacketResult result = SendPacketAndWaitForResponse(payload_prefix_str + sizeDescriptor, this_response, lock);
770776
if (result != PacketResult::Success)
771777
return result;
772778

@@ -2789,7 +2795,7 @@ GDBRemoteCommunicationClient::KillSpawnedProcess (lldb::pid_t pid)
27892795
}
27902796

27912797
bool
2792-
GDBRemoteCommunicationClient::SetCurrentThread (uint64_t tid)
2798+
GDBRemoteCommunicationClient::SetCurrentThread(uint64_t tid, const Lock &lock)
27932799
{
27942800
if (m_curr_tid == tid)
27952801
return true;
@@ -2802,7 +2808,7 @@ GDBRemoteCommunicationClient::SetCurrentThread (uint64_t tid)
28022808
packet_len = ::snprintf (packet, sizeof(packet), "Hg%" PRIx64, tid);
28032809
assert (packet_len + 1 < (int)sizeof(packet));
28042810
StringExtractorGDBRemote response;
2805-
if (SendPacketAndWaitForResponse(packet, packet_len, response, false) == PacketResult::Success)
2811+
if (SendPacketAndWaitForResponse(llvm::StringRef(packet, packet_len), response, lock) == PacketResult::Success)
28062812
{
28072813
if (response.IsOKResponse())
28082814
{
@@ -2963,9 +2969,9 @@ GDBRemoteCommunicationClient::GetCurrentThreadIDs (std::vector<lldb::tid_t> &thr
29632969
StringExtractorGDBRemote response;
29642970

29652971
PacketResult packet_result;
2966-
for (packet_result = SendPacketAndWaitForResponseNoLock("qfThreadInfo", response);
2972+
for (packet_result = SendPacketAndWaitForResponse("qfThreadInfo", response, lock);
29672973
packet_result == PacketResult::Success && response.IsNormalResponse();
2968-
packet_result = SendPacketAndWaitForResponseNoLock("qsThreadInfo", response))
2974+
packet_result = SendPacketAndWaitForResponse("qsThreadInfo", response, lock))
29692975
{
29702976
char ch = response.GetChar();
29712977
if (ch == 'l')
@@ -3496,12 +3502,12 @@ GDBRemoteCommunicationClient::AvoidGPackets (ProcessGDBRemote *process)
34963502
}
34973503

34983504
DataBufferSP
3499-
GDBRemoteCommunicationClient::ReadRegister(lldb::tid_t tid, uint32_t reg)
3505+
GDBRemoteCommunicationClient::ReadRegister(lldb::tid_t tid, uint32_t reg, const Lock &lock)
35003506
{
35013507
StreamString payload;
35023508
payload.Printf("p%x", reg);
35033509
StringExtractorGDBRemote response;
3504-
if (SendThreadSpecificPacketAndWaitForResponse(tid, std::move(payload), response, false) != PacketResult::Success ||
3510+
if (SendThreadSpecificPacketAndWaitForResponse(tid, std::move(payload), response, lock) != PacketResult::Success ||
35053511
!response.IsNormalResponse())
35063512
return nullptr;
35073513

@@ -3511,12 +3517,12 @@ GDBRemoteCommunicationClient::ReadRegister(lldb::tid_t tid, uint32_t reg)
35113517
}
35123518

35133519
DataBufferSP
3514-
GDBRemoteCommunicationClient::ReadAllRegisters(lldb::tid_t tid)
3520+
GDBRemoteCommunicationClient::ReadAllRegisters(lldb::tid_t tid, const Lock &lock)
35153521
{
35163522
StreamString payload;
35173523
payload.PutChar('g');
35183524
StringExtractorGDBRemote response;
3519-
if (SendThreadSpecificPacketAndWaitForResponse(tid, std::move(payload), response, false) != PacketResult::Success ||
3525+
if (SendThreadSpecificPacketAndWaitForResponse(tid, std::move(payload), response, lock) != PacketResult::Success ||
35203526
!response.IsNormalResponse())
35213527
return nullptr;
35223528

@@ -3526,25 +3532,26 @@ GDBRemoteCommunicationClient::ReadAllRegisters(lldb::tid_t tid)
35263532
}
35273533

35283534
bool
3529-
GDBRemoteCommunicationClient::WriteRegister(lldb::tid_t tid, uint32_t reg_num, llvm::ArrayRef<uint8_t> data)
3535+
GDBRemoteCommunicationClient::WriteRegister(lldb::tid_t tid, uint32_t reg_num, llvm::ArrayRef<uint8_t> data,
3536+
const Lock &lock)
35303537
{
35313538
StreamString payload;
35323539
payload.Printf("P%x=", reg_num);
35333540
payload.PutBytesAsRawHex8(data.data(), data.size(), endian::InlHostByteOrder(), endian::InlHostByteOrder());
35343541
StringExtractorGDBRemote response;
3535-
return SendThreadSpecificPacketAndWaitForResponse(tid, std::move(payload), response, false) ==
3542+
return SendThreadSpecificPacketAndWaitForResponse(tid, std::move(payload), response, lock) ==
35363543
PacketResult::Success &&
35373544
response.IsOKResponse();
35383545
}
35393546

35403547
bool
3541-
GDBRemoteCommunicationClient::WriteAllRegisters(lldb::tid_t tid, llvm::ArrayRef<uint8_t> data)
3548+
GDBRemoteCommunicationClient::WriteAllRegisters(lldb::tid_t tid, llvm::ArrayRef<uint8_t> data, const Lock &lock)
35423549
{
35433550
StreamString payload;
35443551
payload.PutChar('G');
35453552
payload.PutBytesAsRawHex8(data.data(), data.size(), endian::InlHostByteOrder(), endian::InlHostByteOrder());
35463553
StringExtractorGDBRemote response;
3547-
return SendThreadSpecificPacketAndWaitForResponse(tid, std::move(payload), response, false) ==
3554+
return SendThreadSpecificPacketAndWaitForResponse(tid, std::move(payload), response, lock) ==
35483555
PacketResult::Success &&
35493556
response.IsOKResponse();
35503557
}
@@ -3555,12 +3562,21 @@ GDBRemoteCommunicationClient::SaveRegisterState (lldb::tid_t tid, uint32_t &save
35553562
save_id = 0; // Set to invalid save ID
35563563
if (m_supports_QSaveRegisterState == eLazyBoolNo)
35573564
return false;
3558-
3565+
3566+
Lock lock(*this, false);
3567+
if (!lock)
3568+
{
3569+
Log *log(ProcessGDBRemoteLog::GetLogIfAnyCategoryIsSet(GDBR_LOG_PROCESS | GDBR_LOG_PACKETS));
3570+
if (log)
3571+
log->Printf("GDBRemoteCommunicationClient::%s failed to get sequence mutex", __FUNCTION__);
3572+
return false;
3573+
}
3574+
35593575
m_supports_QSaveRegisterState = eLazyBoolYes;
35603576
StreamString payload;
35613577
payload.PutCString("QSaveRegisterState");
35623578
StringExtractorGDBRemote response;
3563-
if (SendThreadSpecificPacketAndWaitForResponse(tid, std::move(payload), response, false) != PacketResult::Success)
3579+
if (SendThreadSpecificPacketAndWaitForResponse(tid, std::move(payload), response, lock) != PacketResult::Success)
35643580
return false;
35653581

35663582
if (response.IsUnsupportedResponse())
@@ -3583,10 +3599,19 @@ GDBRemoteCommunicationClient::RestoreRegisterState (lldb::tid_t tid, uint32_t sa
35833599
if (m_supports_QSaveRegisterState == eLazyBoolNo)
35843600
return false;
35853601

3602+
Lock lock(*this, false);
3603+
if (!lock)
3604+
{
3605+
Log *log(ProcessGDBRemoteLog::GetLogIfAnyCategoryIsSet(GDBR_LOG_PROCESS | GDBR_LOG_PACKETS));
3606+
if (log)
3607+
log->Printf("GDBRemoteCommunicationClient::%s failed to get sequence mutex", __FUNCTION__);
3608+
return false;
3609+
}
3610+
35863611
StreamString payload;
35873612
payload.Printf("QRestoreRegisterState:%u", save_id);
35883613
StringExtractorGDBRemote response;
3589-
if (SendThreadSpecificPacketAndWaitForResponse(tid, std::move(payload), response, false) != PacketResult::Success)
3614+
if (SendThreadSpecificPacketAndWaitForResponse(tid, std::move(payload), response, lock) != PacketResult::Success)
35903615
return false;
35913616

35923617
if (response.IsOKResponse())
@@ -3815,7 +3840,7 @@ GDBRemoteCommunicationClient::ServeSymbolLookups(lldb_private::Process *process)
38153840
StreamString packet;
38163841
packet.PutCString ("qSymbol::");
38173842
StringExtractorGDBRemote response;
3818-
while (SendPacketAndWaitForResponseNoLock(packet.GetString(), response) == PacketResult::Success)
3843+
while (SendPacketAndWaitForResponse(packet.GetString(), response, lock) == PacketResult::Success)
38193844
{
38203845
if (response.IsOKResponse())
38213846
{

‎lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h

+12-8
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,9 @@ class GDBRemoteCommunicationClient : public GDBRemoteClientBase
6464
SendPacketsAndConcatenateResponses (const char *send_payload_prefix,
6565
std::string &response_string);
6666

67+
void
68+
ComputeThreadSuffixSupport();
69+
6770
bool
6871
GetThreadSuffixSupported();
6972

@@ -394,9 +397,6 @@ class GDBRemoteCommunicationClient : public GDBRemoteClientBase
394397
SendSpeedTestPacket (uint32_t send_size,
395398
uint32_t recv_size);
396399

397-
bool
398-
SetCurrentThread (uint64_t tid);
399-
400400
bool
401401
SetCurrentThreadForRun (uint64_t tid);
402402

@@ -488,17 +488,18 @@ class GDBRemoteCommunicationClient : public GDBRemoteClientBase
488488

489489
lldb::DataBufferSP
490490
ReadRegister(lldb::tid_t tid,
491-
uint32_t reg_num); // Must be the eRegisterKindProcessPlugin register number
491+
uint32_t reg_num, // Must be the eRegisterKindProcessPlugin register number
492+
const Lock &lock);
492493

493494
lldb::DataBufferSP
494-
ReadAllRegisters(lldb::tid_t tid);
495+
ReadAllRegisters(lldb::tid_t tid, const Lock &lock);
495496

496497
bool
497498
WriteRegister(lldb::tid_t tid, uint32_t reg_num, // eRegisterKindProcessPlugin register number
498-
llvm::ArrayRef<uint8_t> data);
499+
llvm::ArrayRef<uint8_t> data, const Lock &lock);
499500

500501
bool
501-
WriteAllRegisters(lldb::tid_t tid, llvm::ArrayRef<uint8_t> data);
502+
WriteAllRegisters(lldb::tid_t tid, llvm::ArrayRef<uint8_t> data, const Lock &lock);
502503

503504
bool
504505
SaveRegisterState(lldb::tid_t tid, uint32_t &save_id);
@@ -686,7 +687,10 @@ class GDBRemoteCommunicationClient : public GDBRemoteClientBase
686687

687688
PacketResult
688689
SendThreadSpecificPacketAndWaitForResponse(lldb::tid_t tid, StreamString &&payload,
689-
StringExtractorGDBRemote &response, bool send_async);
690+
StringExtractorGDBRemote &response, const Lock &lock);
691+
692+
bool
693+
SetCurrentThread(uint64_t tid, const Lock &lock);
690694

691695
private:
692696
DISALLOW_COPY_AND_ASSIGN (GDBRemoteCommunicationClient);

0 commit comments

Comments
 (0)
Please sign in to comment.