Re: [Lldb-commits] [PATCH] Add Read Thread to GDBRemoteCommunication.
REPOSITORY
rL LLVM
http://reviews.llvm.org/D10085
Files:
lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h
lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
EMAIL PREFERENCES
http://reviews.llvm.org/settings/panel/emailpreferences/
Index: lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
===
--- lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
+++ lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
@@ -145,7 +145,7 @@
PacketResult packet_result = PacketResult::Success;
const uint32_t timeout_usec = 10 * 1000; // Wait for 10 ms for a response
while (packet_result == PacketResult::Success)
-packet_result = WaitForPacketWithTimeoutMicroSecondsNoLock (response, timeout_usec, false);
+packet_result = ReadPacket (response, timeout_usec, false);
// The return value from QueryNoAckModeSupported() is true if the packet
// was sent and _any_ response (including UNIMPLEMENTED) was received),
@@ -654,7 +654,7 @@
{
PacketResult packet_result = SendPacketNoLock (payload, payload_length);
if (packet_result == PacketResult::Success)
-packet_result = WaitForPacketWithTimeoutMicroSecondsNoLock (response, GetPacketTimeoutInMicroSeconds (), true);
+packet_result = ReadPacket (response, GetPacketTimeoutInMicroSeconds (), true);
return packet_result;
}
@@ -670,6 +670,12 @@
PacketResult packet_result = PacketResult::ErrorSendFailed;
Mutex::Locker locker;
Log *log (ProcessGDBRemoteLog::GetLogIfAllCategoriesSet (GDBR_LOG_PROCESS));
+
+// In order to stop async notifications from being processed in the middle of the
+// send/recieve sequence Hijack the broadcast. Then rebroadcast any events when we are done.
+static Listener hijack_listener("lldb.NotifyHijacker");
+HijackBroadcaster(&hijack_listener, eBroadcastBitGdbReadThreadGotNotify);
+
if (GetSequenceMutex (locker))
{
packet_result = SendPacketAndWaitForResponseNoLock (payload, payload_length, response);
@@ -761,6 +767,15 @@
log->Printf("error: failed to get packet sequence mutex, not sending packet '%*s'", (int) payload_length, payload);
}
}
+
+// Remove our Hijacking listner from the broadcast.
+RestoreBroadcaster();
+
+// If a notification event occured, rebroadcast since it can now be processed safely.
+EventSP event_sp;
+if (hijack_listener.GetNextEvent(event_sp))
+BroadcastEvent(event_sp);
+
return packet_result;
}
@@ -902,9 +917,9 @@
got_async_packet = false;
if (log)
-log->Printf ("GDBRemoteCommunicationClient::%s () WaitForPacket(%s)", __FUNCTION__, continue_packet.c_str());
+log->Printf ("GDBRemoteCommunicationClient::%s () ReadPacket(%s)", __FUNCTION__, continue_packet.c_str());
-if (WaitForPacketWithTimeoutMicroSecondsNoLock(response, UINT32_MAX, false) == PacketResult::Success)
+if (ReadPacket(response, UINT32_MAX, false) == PacketResult::Success)
{
if (response.Empty())
state = eStateInvalid;
@@ -961,7 +976,7 @@
// packet to make sure it doesn't get in the way
StringExtractorGDBRemote extra_stop_reply_packet;
uint32_t timeout_usec = 1000;
-if (WaitForPacketWithTimeoutMicroSecondsNoLock (extra_stop_reply_packet, timeout_usec, false) == PacketResult::Success)
+if (ReadPacket (extra_stop_reply_packet, timeout_usec, false) == PacketResult::Success)
{
switch (extra_stop_reply_packet.GetChar())
{
@@ -1139,7 +1154,7 @@
else
{
if (log)
-log->Printf ("GDBRemoteCommunicationClient::%s () WaitForPacket(...) => false", __FUNCTION__);
+log->Printf ("GDBRemoteCommunicationClient::%s () ReadPacket(...) => false", __FUNCTION__);
state = eStateInvalid;
}
}
Index: lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
===
--- lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
+++ lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
@@ -171,6 +171,12 @@
{
Disconnect();
}
+
+// Stop the communications read thread which is used to parse all
+// incoming packets. This function will block until the read
+// thread retu
Re: [Lldb-commits] [PATCH] Add Read Thread to GDBRemoteCommunication.
Looks fine as long as all tests still pass on MacOSX and there is no regression is packet speed test. REPOSITORY rL LLVM http://reviews.llvm.org/D10085 EMAIL PREFERENCES http://reviews.llvm.org/settings/panel/emailpreferences/ ___ lldb-commits mailing list lldb-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] Add Read Thread to GDBRemoteCommunication.
I looked into how GDB processes async packets with only one thread. Essentially
GDB has a high level event loop which listens for events from multiple sources.
Events are queued and only processed later by calling the appropriate handler,
allowing the event loop to continue listening for more events. There is an
event for async notification packets which is generated when the notify packet
is received, but only responded later so that it doesn't interfere with regular
synchronous communication. The event itself is generated from either polling
the serial connection or looking for a synchronous response after a send.
For our purposes polling isn't the best solution since we can spawn a thread
from Communication easily. However delaying the notify processing does address
a race condition I didn't previously cover.
(a) ---> m
(b) <--- %notify
(c) [broadcast notify packet received]
(d) ---> vStopped [broadcast is handled by listener in AysncThread before
reply to (a) is received]
(e) <--- memory reply to (a), unexpected response to vStopped.
This patch fixes this by getting SendPacketAndWaitForResponse() to Hijack the
notify broadcast, then rebroadcasting after the synchronous response has been
seen. Avoiding interference with regular synchronous communication.
REPOSITORY
rL LLVM
http://reviews.llvm.org/D10085
Files:
source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
EMAIL PREFERENCES
http://reviews.llvm.org/settings/panel/emailpreferences/
Index: source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
===
--- source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
+++ source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
@@ -171,6 +171,12 @@
{
Disconnect();
}
+
+// Stop the communications read thread which is used to parse all
+// incoming packets. This function will block until the read
+// thread returns.
+if (m_read_thread_enabled)
+StopReadThread();
}
char
@@ -295,7 +301,7 @@
GDBRemoteCommunication::GetAck ()
{
StringExtractorGDBRemote packet;
-PacketResult result = WaitForPacketWithTimeoutMicroSecondsNoLock (packet, GetPacketTimeoutInMicroSeconds (), false);
+PacketResult result = ReadPacket (packet, GetPacketTimeoutInMicroSeconds (), false);
if (result == PacketResult::Success)
{
if (packet.GetResponseType() == StringExtractorGDBRemote::ResponseType::eAck)
@@ -324,6 +330,62 @@
}
GDBRemoteCommunication::PacketResult
+GDBRemoteCommunication::ReadPacket (StringExtractorGDBRemote &response, uint32_t timeout_usec, bool sync_on_timeout)
+{
+ if (m_read_thread_enabled)
+ return PopPacketFromQueue (response, timeout_usec);
+ else
+ return WaitForPacketWithTimeoutMicroSecondsNoLock (response, timeout_usec, sync_on_timeout);
+}
+
+
+// This function is called when a packet is requested.
+// A whole packet is popped from the packet queue and returned to the caller.
+// Packets are placed into this queue from the communication read thread.
+// See GDBRemoteCommunication::AppendBytesToCache.
+GDBRemoteCommunication::PacketResult
+GDBRemoteCommunication::PopPacketFromQueue (StringExtractorGDBRemote &response, uint32_t timeout_usec)
+{
+// Calculate absolute timeout value
+TimeValue timeout = TimeValue::Now();
+timeout.OffsetWithMicroSeconds(timeout_usec);
+
+do
+{
+// scope for the mutex
+{
+// lock down the packet queue
+Mutex::Locker locker(m_packet_queue_mutex);
+
+// Wait on condition variable.
+if (m_packet_queue.size() == 0)
+m_condition_queue_not_empty.Wait(m_packet_queue_mutex, &timeout);
+
+if (m_packet_queue.size() > 0)
+{
+// get the front element of the queue
+response = m_packet_queue.front();
+
+// remove the front element
+m_packet_queue.pop();
+
+// we got a packet
+return PacketResult::Success;
+}
+ }
+
+ // Disconnected
+ if (!IsConnected())
+ return PacketResult::ErrorDisconnected;
+
+ // Loop while not timed out
+} while (TimeValue::Now() < timeout);
+
+return PacketResult::ErrorReplyTimeout;
+}
+
+
+GDBRemoteCommunication::PacketResult
GDBRemoteCommunication::WaitForPacketWithTimeoutMicroSecondsNoLock (StringExtractorGDBRemote &packet, uint32_t timeout_usec, bool sync_on_timeout)
{
uint8_t buffer[8192];
@@ -1094,3 +1156,53 @@
{
m_gdb_comm.SetPacketTimeout (m_saved_timeout);
}
+
+// This function is called via the Communications class read thread when bytes become available
+// for this connection. Th
Re: [Lldb-commits] [PATCH] Add Read Thread to GDBRemoteCommunication.
Ewan, has your non-stop mode implementation been tested against gdbserver? On Jun 1, 2015 11:01 AM, "Greg Clayton" wrote: > My main concern with any threading is pairing the correct response to a > the person that sent the packet. Are async packets marked so they > absolutely can't be confused with any other response packets? I really > don't like the fact that there are no sequence numbers in the sent/received > packets because this makes it hard to know when you get the right response > if a previous packet times out. I recently added "qEcho" to support making > sure we stay on track and would like to ensure that any of the code added > above won't let any packets receive the wrong reply packet. > > > REPOSITORY > rL LLVM > > http://reviews.llvm.org/D10085 > > EMAIL PREFERENCES > http://reviews.llvm.org/settings/panel/emailpreferences/ > > > ___ lldb-commits mailing list lldb-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
