<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, May 27, 2015 at 7:12 AM, Ewan Crawford <span dir="ltr"><<a href="mailto:ewan@codeplay.com" target="_blank">ewan@codeplay.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Author: ewancrawford<br>
Date: Wed May 27 09:12:34 2015<br>
New Revision: 238323<br>
<br>
URL: <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject-3Frev-3D238323-26view-3Drev&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=MEqT8U_n7oNfuDW5NRbY3ZV384ZquXIYFPWmprwUdKM&m=QZJeUvWx27Yk7qhTHI5ACd6tBPndGynKJctU26Gw_Vs&s=7KuDy69_nXgn685NlklgD4E7jHecPsEPhs_SkQdddNo&e=" target="_blank">http://llvm.org/viewvc/llvm-project?rev=238323&view=rev</a><br>
Log:<br>
Change ProcessGDBRemote last stop packet to a container.<br>
<br>
In ProcessGDBRemote we currently have a single packet, m_last_stop_packet, used to set the thread stop info.<br>
However in non-stop mode we can receive several stop reply packets in a sequence for different threads. As a result we need to use a container to hold them before they are processed.<br>
<br>
This patch also changes the return type of CheckPacket() so we can detect async notification packets.<br>
<br>
Reviewers: clayborg<br>
<br>
Subscribers: labath, ted, deepak2427, lldb-commits<br>
<br>
Differential Revision: <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_D9853&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=MEqT8U_n7oNfuDW5NRbY3ZV384ZquXIYFPWmprwUdKM&m=QZJeUvWx27Yk7qhTHI5ACd6tBPndGynKJctU26Gw_Vs&s=hxkw3kGyahSERvxkbSvwtNHSqV00dEK8P95teIx8SKo&e=" target="_blank">http://reviews.llvm.org/D9853</a><br>
<br>
Modified:<br>
lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp<br>
lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h<br>
lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp<br>
lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h<br>
<br>
Modified: lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp<br>
URL: <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_lldb_trunk_source_Plugins_Process_gdb-2Dremote_GDBRemoteCommunication.cpp-3Frev-3D238323-26r1-3D238322-26r2-3D238323-26view-3Ddiff&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=MEqT8U_n7oNfuDW5NRbY3ZV384ZquXIYFPWmprwUdKM&m=QZJeUvWx27Yk7qhTHI5ACd6tBPndGynKJctU26Gw_Vs&s=_u19OJqAI8Y2imRSbPdoe5EccWryd7MPivd7Lz28ajM&e=" target="_blank">http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp?rev=238323&r1=238322&r2=238323&view=diff</a><br>
==============================================================================<br>
--- lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp (original)<br>
+++ lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp Wed May 27 09:12:34 2015<br>
@@ -329,7 +329,7 @@ GDBRemoteCommunication::WaitForPacketWit<br>
Log *log (ProcessGDBRemoteLog::GetLogIfAllCategoriesSet (GDBR_LOG_PACKETS | GDBR_LOG_VERBOSE));<br>
<br>
// Check for a packet from our cache first without trying any reading...<br>
- if (CheckForPacket (NULL, 0, packet))<br>
+ if (CheckForPacket(NULL, 0, packet) != PacketType::Invalid)<br>
return PacketResult::Success;<br>
<br>
bool timed_out = false;<br>
@@ -349,7 +349,7 @@ GDBRemoteCommunication::WaitForPacketWit<br>
<br>
if (bytes_read > 0)<br>
{<br>
- if (CheckForPacket (buffer, bytes_read, packet))<br>
+ if (CheckForPacket(buffer, bytes_read, packet) != PacketType::Invalid)<br>
return PacketResult::Success;<br>
}<br>
else<br>
@@ -383,7 +383,7 @@ GDBRemoteCommunication::WaitForPacketWit<br>
return PacketResult::ErrorReplyFailed;<br>
}<br>
<br>
-bool<br>
+GDBRemoteCommunication::PacketType<br>
GDBRemoteCommunication::CheckForPacket (const uint8_t *src, size_t src_len, StringExtractorGDBRemote &packet)<br>
{<br>
// Put the packet data into the buffer in a thread safe fashion<br>
@@ -405,6 +405,8 @@ GDBRemoteCommunication::CheckForPacket (<br>
m_bytes.append ((const char *)src, src_len);<br>
}<br>
<br>
+ bool isNotifyPacket = false;<br>
+<br>
// Parse up the packets into gdb remote packets<br>
if (!m_bytes.empty())<br>
{<br>
@@ -425,6 +427,9 @@ GDBRemoteCommunication::CheckForPacket (<br>
break;<br>
<br>
case '%': // Async notify packet<br>
+ isNotifyPacket = true;<br>
+ // Intentional fall through<br>
+<br>
case '$':<br>
// Look for a standard gdb packet?<br>
{<br>
@@ -487,7 +492,7 @@ GDBRemoteCommunication::CheckForPacket (<br>
if (content_length == std::string::npos)<br>
{<br>
packet.Clear();<br>
- return false;<br>
+ return GDBRemoteCommunication::PacketType::Invalid;<br>
}<br>
else if (total_length > 0)<br>
{<br>
@@ -626,11 +631,15 @@ GDBRemoteCommunication::CheckForPacket (<br>
<br>
m_bytes.erase(0, total_length);<br>
packet.SetFilePos(0);<br>
- return success;<br>
+<br>
+ if (isNotifyPacket)<br>
+ return GDBRemoteCommunication::PacketType::Notify;<br>
+ else<br>
+ return GDBRemoteCommunication::PacketType::Standard;<br>
}<br>
}<br>
packet.Clear();<br>
- return false;<br>
+ return GDBRemoteCommunication::PacketType::Invalid;<br>
}<br>
<br>
Error<br>
<br>
Modified: lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h<br>
URL: <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_lldb_trunk_source_Plugins_Process_gdb-2Dremote_GDBRemoteCommunication.h-3Frev-3D238323-26r1-3D238322-26r2-3D238323-26view-3Ddiff&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=MEqT8U_n7oNfuDW5NRbY3ZV384ZquXIYFPWmprwUdKM&m=QZJeUvWx27Yk7qhTHI5ACd6tBPndGynKJctU26Gw_Vs&s=d1rNyPO8TudmQ-dEQW4CzxSfSHbug_6LgTSIUhuJ2i8&e=" target="_blank">http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h?rev=238323&r1=238322&r2=238323&view=diff</a><br>
==============================================================================<br>
--- lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h (original)<br>
+++ lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h Wed May 27 09:12:34 2015<br>
@@ -49,7 +49,14 @@ public:<br>
{<br>
eBroadcastBitRunPacketSent = kLoUserBroadcastBit<br>
};<br>
-<br>
+<br>
+ enum class PacketType<br>
+ {<br>
+ Invalid = 0,<br>
+ Standard,<br>
+ Notify<br>
+ };<br>
+<br>
enum class PacketResult<br>
{<br>
Success = 0, // Success<br>
@@ -101,7 +108,7 @@ public:<br>
bool<br>
GetSequenceMutex (Mutex::Locker& locker, const char *failure_message = NULL);<br>
<br>
- bool<br>
+ PacketType<br>
CheckForPacket (const uint8_t *src,<br>
size_t src_len,<br>
StringExtractorGDBRemote &packet);<br>
<br>
Modified: lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp<br>
URL: <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_lldb_trunk_source_Plugins_Process_gdb-2Dremote_ProcessGDBRemote.cpp-3Frev-3D238323-26r1-3D238322-26r2-3D238323-26view-3Ddiff&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=MEqT8U_n7oNfuDW5NRbY3ZV384ZquXIYFPWmprwUdKM&m=QZJeUvWx27Yk7qhTHI5ACd6tBPndGynKJctU26Gw_Vs&s=mPVKKOZ0B6k-ADwP_ye3UsKYbAVQCOdo7t8kCICqguw&e=" target="_blank">http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp?rev=238323&r1=238322&r2=238323&view=diff</a><br>
==============================================================================<br>
--- lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp (original)<br>
+++ lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp Wed May 27 09:12:34 2015<br>
@@ -374,7 +374,6 @@ ProcessGDBRemote::ProcessGDBRemote(Targe<br>
m_flags (0),<br>
m_gdb_comm (),<br>
m_debugserver_pid (LLDB_INVALID_PROCESS_ID),<br>
- m_last_stop_packet (),<br>
m_last_stop_packet_mutex (Mutex::eMutexTypeNormal),<br>
m_register_info (),<br>
m_async_broadcaster (NULL, "lldb.process.gdb-remote.async-broadcaster"),<br>
@@ -769,8 +768,10 @@ ProcessGDBRemote::DoConnectRemote (Strea<br>
// We have a valid process<br>
SetID (pid);<br>
GetThreadList();<br>
- if (m_gdb_comm.GetStopReply(m_last_stop_packet))<br>
+ StringExtractorGDBRemote response;<br>
+ if (m_gdb_comm.GetStopReply(response))<br>
{<br>
+ SetLastStopPacket(response);<br>
<br>
// '?' Packets must be handled differently in non-stop mode<br>
if (GetTarget().GetNonStopModeEnabled())<br>
@@ -788,7 +789,7 @@ ProcessGDBRemote::DoConnectRemote (Strea<br>
}<br>
}<br>
<br>
- const StateType state = SetThreadStopInfo (m_last_stop_packet);<br>
+ const StateType state = SetThreadStopInfo (response);<br>
if (state == eStateStopped)<br>
{<br>
SetPrivateState (state);<br>
@@ -1057,9 +1058,10 @@ ProcessGDBRemote::DoLaunch (Module *exe_<br>
return error;<br>
}<br>
<br>
- if (m_gdb_comm.GetStopReply(m_last_stop_packet))<br>
+ StringExtractorGDBRemote response;<br>
+ if (m_gdb_comm.GetStopReply(response))<br>
{<br>
-<br>
+ SetLastStopPacket(response);<br></blockquote><div><br></div><div>Why call SetLastStopPacket if you're calling SetThreadStopInfo()? </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
// '?' Packets must be handled differently in non-stop mode<br>
if (GetTarget().GetNonStopModeEnabled())<br>
HandleStopReplySequence();<br>
@@ -1077,7 +1079,7 @@ ProcessGDBRemote::DoLaunch (Module *exe_<br>
m_target.MergeArchitecture(host_arch);<br>
}<br>
<br>
- SetPrivateState (SetThreadStopInfo (m_last_stop_packet));<br>
+ SetPrivateState (SetThreadStopInfo (response));<br>
<br>
if (!disable_stdio)<br>
{<br>
@@ -2123,7 +2125,25 @@ ProcessGDBRemote::RefreshStateAfterStop<br>
// Set the thread stop info. It might have a "threads" key whose value is<br>
// a list of all thread IDs in the current process, so m_thread_ids might<br>
// get set.<br>
- SetThreadStopInfo (m_last_stop_packet);<br>
+<br>
+ // Scope for the lock<br>
+ {<br>
+ // Lock the thread stack while we access it<br>
+ Mutex::Locker stop_stack_lock(m_last_stop_packet_mutex);<br>
+ // Get the number of stop packets on the stack<br>
+ int nItems = m_stop_packet_stack.size();<br>
+ // Iterate over them<br>
+ for (int i = 0; i < nItems; i++)<br>
+ {<br>
+ // Get the thread stop info<br>
+ StringExtractorGDBRemote stop_info = m_stop_packet_stack[i];<br>
+ // Process thread stop info<br>
+ SetThreadStopInfo(stop_info);<br></blockquote><div><br></div><div>This doesn't seem right.</div><div>SetThreadStopInfo needs an up to date threadlist for <span class="">m_thread_list</span><span class="">.</span>FindThreadByProtocolID(). If these packets contain threads, only the first will be processed with the correct threads because UpdateThreadListIfNeeded only checks the stop id.</div><div>So it seems we should either be bumping the stop id for each packet, or calling UpdateThreadList directly before using m_thread_list.</div><div>Also if these packets don't have the thread list you're in trouble because UpdateThreadListIfNeeded will get the current list, not the one from the time of the stop.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
+ }<br>
+ // Clear the thread stop stack<br>
+ m_stop_packet_stack.clear();<br>
+ }<br>
+<br>
// Check to see if SetThreadStopInfo() filled in m_thread_ids?<br>
if (m_thread_ids.empty())<br>
{<br>
@@ -2395,7 +2415,6 @@ ProcessGDBRemote::DoDestroy ()<br>
void<br>
ProcessGDBRemote::SetLastStopPacket (const StringExtractorGDBRemote &response)<br>
{<br>
- Mutex::Locker locker (m_last_stop_packet_mutex);<br>
const bool did_exec = response.GetStringRef().find(";reason:exec;") != std::string::npos;<br>
if (did_exec)<br>
{<br>
@@ -2408,7 +2427,16 @@ ProcessGDBRemote::SetLastStopPacket (con<br>
BuildDynamicRegisterInfo (true);<br>
m_gdb_comm.ResetDiscoverableSettings();<br>
}<br>
- m_last_stop_packet = response;<br>
+<br>
+ // Scope the lock<br>
+ {<br>
+ // Lock the thread stack while we access it<br>
+ Mutex::Locker stop_stack_lock(m_last_stop_packet_mutex);<br>
+ // Add this stop packet to the stop packet stack<br>
+ // This stack will get popped and examined when we switch to the<br>
+ // Stopped state<br>
+ m_stop_packet_stack.push_back(response);<br>
+ }<br>
}<br>
<br>
<br>
<br>
Modified: lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h<br>
URL: <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_lldb_trunk_source_Plugins_Process_gdb-2Dremote_ProcessGDBRemote.h-3Frev-3D238323-26r1-3D238322-26r2-3D238323-26view-3Ddiff&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=MEqT8U_n7oNfuDW5NRbY3ZV384ZquXIYFPWmprwUdKM&m=QZJeUvWx27Yk7qhTHI5ACd6tBPndGynKJctU26Gw_Vs&s=yw4948esw3XcqlX80ymPaY0mzeDYCMwDbv_AaNI5J6Y&e=" target="_blank">http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h?rev=238323&r1=238322&r2=238323&view=diff</a><br>
==============================================================================<br>
--- lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h (original)<br>
+++ lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h Wed May 27 09:12:34 2015<br>
@@ -333,7 +333,7 @@ protected:<br>
Flags m_flags; // Process specific flags (see eFlags enums)<br>
GDBRemoteCommunicationClient m_gdb_comm;<br>
std::atomic<lldb::pid_t> m_debugserver_pid;<br>
- StringExtractorGDBRemote m_last_stop_packet;<br>
+ std::vector<StringExtractorGDBRemote> m_stop_packet_stack; // The stop packet stack replaces the last stop packet variable<br>
Mutex m_last_stop_packet_mutex;<br>
GDBRemoteDynamicRegisterInfo m_register_info;<br>
Broadcaster m_async_broadcaster;<br>
<br>
<br>
_______________________________________________<br>
lldb-commits mailing list<br>
<a href="mailto:lldb-commits@cs.uiuc.edu">lldb-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits</a><br>
</blockquote></div><br></div></div>