Improved the packet throughput when debugging with GDB remote by over 3x on
darwin (not sure about other platforms).
Modified the communication and connection classes to not require the
BytesAvailable function. Now the "Read(...)" function has a timeout in
microseconds.
Fixed a lot of assertions that were firing off in certain cases and replaced
them with error output and code that can deal with the assertion case.
git-svn-id: https://llvm.org/svn/llvm-project/llvdb/trunk@133224 91177308-0d34-0410-b5e6-96231b3b80d8
diff --git a/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp b/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
index c5e11ef..c2c9ddf 100644
--- a/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
+++ b/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
@@ -39,16 +39,12 @@
bool is_platform) :
Communication(comm_name),
m_packet_timeout (60),
- m_rx_packet_listener (listener_name),
m_sequence_mutex (Mutex::eMutexTypeRecursive),
m_public_is_running (false),
m_private_is_running (false),
m_send_acks (true),
m_is_platform (is_platform)
{
- m_rx_packet_listener.StartListeningForEvents(this,
- Communication::eBroadcastBitPacketAvailable |
- Communication::eBroadcastBitReadThreadDidExit);
}
//----------------------------------------------------------------------
@@ -56,12 +52,8 @@
//----------------------------------------------------------------------
GDBRemoteCommunication::~GDBRemoteCommunication()
{
- m_rx_packet_listener.StopListeningForEvents(this,
- Communication::eBroadcastBitPacketAvailable |
- Communication::eBroadcastBitReadThreadDidExit);
if (IsConnected())
{
- StopReadThread();
Disconnect();
}
}
@@ -167,7 +159,7 @@
GDBRemoteCommunication::GetAck ()
{
StringExtractorGDBRemote packet;
- if (WaitForPacket (packet, m_packet_timeout) == 1)
+ if (WaitForPacketWithTimeoutMicroSeconds (packet, GetPacketTimeoutInMicroSeconds ()) == 1)
return packet.GetChar();
return 0;
}
@@ -186,99 +178,63 @@
}
size_t
-GDBRemoteCommunication::WaitForPacket (StringExtractorGDBRemote &packet, uint32_t timeout_seconds)
+GDBRemoteCommunication::WaitForPacketWithTimeoutMicroSeconds (StringExtractorGDBRemote &packet, uint32_t timeout_usec)
{
Mutex::Locker locker(m_sequence_mutex);
- TimeValue timeout_time;
- timeout_time = TimeValue::Now();
- timeout_time.OffsetWithSeconds (timeout_seconds);
- return WaitForPacketNoLock (packet, &timeout_time);
+ return WaitForPacketWithTimeoutMicroSecondsNoLock (packet, timeout_usec);
}
size_t
-GDBRemoteCommunication::WaitForPacket (StringExtractorGDBRemote &packet, const TimeValue* timeout_time_ptr)
+GDBRemoteCommunication::WaitForPacketWithTimeoutMicroSecondsNoLock (StringExtractorGDBRemote &packet, uint32_t timeout_usec)
{
- Mutex::Locker locker(m_sequence_mutex);
- return WaitForPacketNoLock (packet, timeout_time_ptr);
-}
+ uint8_t buffer[8192];
+ Error error;
-size_t
-GDBRemoteCommunication::WaitForPacketNoLock (StringExtractorGDBRemote &packet, const TimeValue* timeout_time_ptr)
-{
- bool checksum_error = false;
- packet.Clear ();
+ // Check for a packet from our cache first without trying any reading...
+ if (CheckForPacket (NULL, 0, packet))
+ return packet.GetStringRef().size();
- EventSP event_sp;
-
- if (m_rx_packet_listener.WaitForEvent (timeout_time_ptr, event_sp))
+ bool timed_out = false;
+ while (IsConnected() && !timed_out)
{
- const uint32_t event_type = event_sp->GetType();
- if (event_type | Communication::eBroadcastBitPacketAvailable)
+ lldb::ConnectionStatus status;
+ size_t bytes_read = Read (buffer, sizeof(buffer), timeout_usec, status, &error);
+ if (bytes_read > 0)
{
- const EventDataBytes *event_bytes = EventDataBytes::GetEventDataFromEvent(event_sp.get());
- if (event_bytes)
+ if (CheckForPacket (buffer, bytes_read, packet))
+ return packet.GetStringRef().size();
+ }
+ else
+ {
+ switch (status)
{
- const char *packet_data = (const char *)event_bytes->GetBytes();
- const uint32_t packet_size = event_bytes->GetByteSize();
- LogSP log (ProcessGDBRemoteLog::GetLogIfAllCategoriesSet (GDBR_LOG_PACKETS));
- if (log)
- log->Printf ("read packet: %.*s", packet_size, packet_data);
- if (packet_data && packet_size > 0)
- {
- std::string &packet_str = packet.GetStringRef();
- if (packet_data[0] == '$')
- {
- bool success = false;
- if (packet_size < 4)
- ::fprintf (stderr, "Packet that starts with $ is too short: '%s'\n", packet_data);
- else if (packet_data[packet_size-3] != '#' ||
- !::isxdigit (packet_data[packet_size-2]) ||
- !::isxdigit (packet_data[packet_size-1]))
- ::fprintf (stderr, "Invalid checksum footer for packet: '%s'\n", packet_data);
- else
- success = true;
-
- if (success)
- packet_str.assign (packet_data + 1, packet_size - 4);
- if (GetSendAcks ())
- {
- if (success)
- {
- char packet_checksum = strtol (&packet_data[packet_size-2], NULL, 16);
- char actual_checksum = CalculcateChecksum (&packet_str[0], packet_str.size());
- checksum_error = packet_checksum != actual_checksum;
- }
- // Send the ack or nack if needed
- if (checksum_error || !success)
- SendNack();
- else
- SendAck();
- }
- }
- else
- {
- packet_str.assign (packet_data, packet_size);
- }
- return packet_str.size();
- }
+ case eConnectionStatusSuccess:
+ break;
+
+ case eConnectionStatusEndOfFile:
+ case eConnectionStatusNoConnection:
+ case eConnectionStatusLostConnection:
+ case eConnectionStatusError:
+ Disconnect();
+ break;
+
+ case eConnectionStatusTimedOut:
+ timed_out = true;
+ break;
}
}
- else if (event_type | Communication::eBroadcastBitReadThreadDidExit)
- {
- // Our read thread exited on us so just fall through and return zero...
- Disconnect();
- }
}
+ packet.Clear ();
return 0;
}
-void
-GDBRemoteCommunication::AppendBytesToCache (const uint8_t *src, size_t src_len, bool broadcast,
- ConnectionStatus status)
+bool
+GDBRemoteCommunication::CheckForPacket (const uint8_t *src, size_t src_len, StringExtractorGDBRemote &packet)
{
// Put the packet data into the buffer in a thread safe fashion
Mutex::Locker locker(m_bytes_mutex);
- m_bytes.append ((const char *)src, src_len);
+ if (src && src_len > 0)
+ m_bytes.append ((const char *)src, src_len);
// Parse up the packets into gdb remote packets
while (!m_bytes.empty())
@@ -286,29 +242,40 @@
// end_idx must be one past the last valid packet byte. Start
// it off with an invalid value that is the same as the current
// index.
- size_t end_idx = 0;
+ size_t content_start = 0;
+ size_t content_length = 0;
+ size_t total_length = 0;
+ size_t checksum_idx = std::string::npos;
switch (m_bytes[0])
{
case '+': // Look for ack
case '-': // Look for cancel
case '\x03': // ^C to halt target
- end_idx = 1; // The command is one byte long...
+ content_length = total_length = 1; // The command is one byte long...
break;
case '$':
// Look for a standard gdb packet?
- end_idx = m_bytes.find('#');
- if (end_idx != std::string::npos)
{
- if (end_idx + 2 < m_bytes.size())
+ size_t hash_pos = m_bytes.find('#');
+ if (hash_pos != std::string::npos)
{
- end_idx += 3;
- }
- else
- {
- // Checksum bytes aren't all here yet
- end_idx = std::string::npos;
+ if (hash_pos + 2 < m_bytes.size())
+ {
+ checksum_idx = hash_pos + 1;
+ // Skip the dollar sign
+ content_start = 1;
+ // Don't include the # in the content or the $ in the content length
+ content_length = hash_pos - 1;
+
+ total_length = hash_pos + 3; // Skip the # and the two hex checksum bytes
+ }
+ else
+ {
+ // Checksum bytes aren't all here yet
+ content_length = std::string::npos;
+ }
}
}
break;
@@ -317,27 +284,76 @@
break;
}
- if (end_idx == std::string::npos)
+ LogSP log (ProcessGDBRemoteLog::GetLogIfAllCategoriesSet (GDBR_LOG_PACKETS));
+ if (content_length == std::string::npos)
{
- //ProcessGDBRemoteLog::LogIf (GDBR_LOG_PACKETS | GDBR_LOG_VERBOSE, "GDBRemoteCommunication::%s packet not yet complete: '%s'",__FUNCTION__, m_bytes.c_str());
- return;
+ packet.Clear();
+ return false;
}
- else if (end_idx > 0)
+ else if (content_length > 0)
{
+
// We have a valid packet...
- assert (end_idx <= m_bytes.size());
- std::auto_ptr<EventDataBytes> event_bytes_ap (new EventDataBytes (&m_bytes[0], end_idx));
- ProcessGDBRemoteLog::LogIf (GDBR_LOG_COMM, "got full packet: %s", event_bytes_ap->GetBytes());
- BroadcastEvent (eBroadcastBitPacketAvailable, event_bytes_ap.release());
- m_bytes.erase(0, end_idx);
+ assert (content_length <= m_bytes.size());
+ assert (total_length <= m_bytes.size());
+ assert (content_length <= total_length);
+
+ bool success = true;
+ std::string &packet_str = packet.GetStringRef();
+ packet_str.assign (m_bytes, content_start, content_length);
+ if (m_bytes[0] == '$')
+ {
+ assert (checksum_idx < m_bytes.size());
+ if (::isxdigit (m_bytes[checksum_idx+0]) ||
+ ::isxdigit (m_bytes[checksum_idx+1]))
+ {
+ if (GetSendAcks ())
+ {
+ const char *packet_checksum_cstr = &m_bytes[checksum_idx];
+ char packet_checksum = strtol (packet_checksum_cstr, NULL, 16);
+ char actual_checksum = CalculcateChecksum (packet_str.c_str(), packet_str.size());
+ success = packet_checksum == actual_checksum;
+ if (!success)
+ {
+ if (log)
+ log->Printf ("error: checksum mismatch: %.*s expected 0x%2.2x, got 0x%2.2x",
+ (int)(total_length),
+ m_bytes.c_str(),
+ (uint8_t)packet_checksum,
+ (uint8_t)actual_checksum);
+ }
+ // Send the ack or nack if needed
+ if (!success)
+ SendNack();
+ else
+ SendAck();
+ }
+ if (success)
+ {
+ if (log)
+ log->Printf ("read packet: %.*s", (int)(total_length), m_bytes.c_str());
+ }
+ }
+ else
+ {
+ success = false;
+ if (log)
+ log->Printf ("error: invalid checksum in packet: '%s'\n", (int)(total_length), m_bytes.c_str());
+ }
+ }
+ m_bytes.erase(0, total_length);
+ packet.SetFilePos(0);
+ return success;
}
else
{
- assert (1 <= m_bytes.size());
- ProcessGDBRemoteLog::LogIf (GDBR_LOG_COMM, "GDBRemoteCommunication::%s tossing junk byte at %c",__FUNCTION__, m_bytes[0]);
+ if (log)
+ log->Printf ("GDBRemoteCommunication::%s tossing junk byte at %c",__FUNCTION__, m_bytes[0]);
m_bytes.erase(0, 1);
}
}
+ packet.Clear();
+ return false;
}
Error
diff --git a/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h b/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h
index 77e3e1a..addec5b 100644
--- a/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h
+++ b/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h
@@ -56,14 +56,8 @@
// Wait for a packet within 'nsec' seconds
size_t
- WaitForPacket (StringExtractorGDBRemote &response,
- uint32_t sec);
-
- // Wait for a packet with an absolute timeout time. If 'timeout' is NULL
- // wait indefinitely.
- size_t
- WaitForPacket (StringExtractorGDBRemote &response,
- const lldb_private::TimeValue* timeout);
+ WaitForPacketWithTimeoutMicroSeconds (StringExtractorGDBRemote &response,
+ uint32_t usec);
char
GetAck ();
@@ -81,12 +75,10 @@
bool
GetSequenceMutex(lldb_private::Mutex::Locker& locker);
- //------------------------------------------------------------------
- // Communication overrides
- //------------------------------------------------------------------
- virtual void
- AppendBytesToCache (const uint8_t *src, size_t src_len, bool broadcast, lldb::ConnectionStatus status);
-
+ bool
+ CheckForPacket (const uint8_t *src,
+ size_t src_len,
+ StringExtractorGDBRemote &packet);
bool
IsRunning() const
{
@@ -121,6 +113,11 @@
return old_packet_timeout;
}
+ uint32_t
+ GetPacketTimeoutInMicroSeconds () const
+ {
+ return m_packet_timeout * USEC_PER_SEC;
+ }
//------------------------------------------------------------------
// Start a debugserver instance on the current host using the
// supplied connection URL.
@@ -130,6 +127,7 @@
const char *unix_socket_name,
lldb_private::ProcessLaunchInfo &launch_info);
+
protected:
typedef std::list<std::string> packet_collection;
@@ -138,8 +136,8 @@
size_t payload_length);
size_t
- WaitForPacketNoLock (StringExtractorGDBRemote &response,
- const lldb_private::TimeValue* timeout_ptr);
+ WaitForPacketWithTimeoutMicroSecondsNoLock (StringExtractorGDBRemote &response,
+ uint32_t timeout_usec);
bool
WaitForNotRunningPrivate (const lldb_private::TimeValue *timeout_ptr);
@@ -148,7 +146,6 @@
// Classes that inherit from GDBRemoteCommunication can see and modify these
//------------------------------------------------------------------
uint32_t m_packet_timeout;
- lldb_private::Listener m_rx_packet_listener;
lldb_private::Mutex m_sequence_mutex; // Restrict access to sending/receiving packets to a single thread at a time
lldb_private::Predicate<bool> m_public_is_running;
lldb_private::Predicate<bool> m_private_is_running;
diff --git a/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp b/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
index d1c51ec..b648ac1 100644
--- a/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
+++ b/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
@@ -68,9 +68,6 @@
m_os_version_minor (UINT32_MAX),
m_os_version_update (UINT32_MAX)
{
- m_rx_packet_listener.StartListeningForEvents(this,
- Communication::eBroadcastBitPacketAvailable |
- Communication::eBroadcastBitReadThreadDidExit);
}
//----------------------------------------------------------------------
@@ -78,14 +75,8 @@
//----------------------------------------------------------------------
GDBRemoteCommunicationClient::~GDBRemoteCommunicationClient()
{
- m_rx_packet_listener.StopListeningForEvents(this,
- Communication::eBroadcastBitPacketAvailable |
- Communication::eBroadcastBitReadThreadDidExit);
if (IsConnected())
- {
- StopReadThread();
Disconnect();
- }
}
bool
@@ -94,7 +85,7 @@
// Start the read thread after we send the handshake ack since if we
// fail to send the handshake ack, there is no reason to continue...
if (SendAck())
- return StartReadThread (error_ptr);
+ return true;
if (error_ptr)
error_ptr->SetErrorString("failed to send the handshake ack");
@@ -245,15 +236,12 @@
)
{
Mutex::Locker locker;
- TimeValue timeout_time;
- timeout_time = TimeValue::Now();
- timeout_time.OffsetWithSeconds (m_packet_timeout);
LogSP log (ProcessGDBRemoteLog::GetLogIfAllCategoriesSet (GDBR_LOG_PROCESS));
if (GetSequenceMutex (locker))
{
if (SendPacketNoLock (payload, payload_length))
- return WaitForPacketNoLock (response, &timeout_time);
+ return WaitForPacketWithTimeoutMicroSecondsNoLock (response, GetPacketTimeoutInMicroSeconds ());
}
else
{
@@ -272,6 +260,10 @@
{
if (sent_interrupt)
{
+ TimeValue timeout_time;
+ timeout_time = TimeValue::Now();
+ timeout_time.OffsetWithSeconds (m_packet_timeout);
+
if (log)
log->Printf ("async: sent interrupt");
if (m_async_packet_predicate.WaitForValueEqualTo (false, &timeout_time, &timed_out))
@@ -380,7 +372,7 @@
if (log)
log->Printf ("GDBRemoteCommunicationClient::%s () WaitForPacket(%.*s)", __FUNCTION__);
- if (WaitForPacket (response, (TimeValue*)NULL))
+ if (WaitForPacketWithTimeoutMicroSeconds (response, UINT32_MAX))
{
if (response.Empty())
state = eStateInvalid;
@@ -1662,13 +1654,9 @@
sequence_mutex_unavailable = false;
StringExtractorGDBRemote response;
- TimeValue timeout_time;
- timeout_time = TimeValue::Now();
- timeout_time.OffsetWithSeconds (m_packet_timeout*2); // We will always send at least two packets here...
-
- for (SendPacketNoLock ("qfThreadInfo", strlen("qfThreadInfo")) && WaitForPacketNoLock (response, &timeout_time);
+ for (SendPacketNoLock ("qfThreadInfo", strlen("qfThreadInfo")) && WaitForPacketWithTimeoutMicroSecondsNoLock (response, GetPacketTimeoutInMicroSeconds ());
response.IsNormalResponse();
- SendPacketNoLock ("qsThreadInfo", strlen("qsThreadInfo")) && WaitForPacketNoLock (response, &timeout_time))
+ SendPacketNoLock ("qsThreadInfo", strlen("qsThreadInfo")) && WaitForPacketWithTimeoutMicroSecondsNoLock (response, GetPacketTimeoutInMicroSeconds ()))
{
char ch = response.GetChar();
if (ch == 'l')
diff --git a/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp b/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp
index 9379077..0619c10 100644
--- a/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp
+++ b/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp
@@ -78,13 +78,13 @@
//}
//
bool
-GDBRemoteCommunicationServer::GetPacketAndSendResponse (const TimeValue* timeout_ptr,
+GDBRemoteCommunicationServer::GetPacketAndSendResponse (uint32_t timeout_usec,
Error &error,
bool &interrupt,
bool &quit)
{
StringExtractorGDBRemote packet;
- if (WaitForPacketNoLock (packet, timeout_ptr))
+ if (WaitForPacketWithTimeoutMicroSeconds(packet, timeout_usec))
{
const StringExtractorGDBRemote::ServerPacketType packet_type = packet.GetServerPacketType ();
switch (packet_type)
@@ -199,9 +199,7 @@
bool
GDBRemoteCommunicationServer::HandshakeWithClient(Error *error_ptr)
{
- if (StartReadThread(error_ptr))
- return GetAck();
- return false;
+ return GetAck();
}
bool
@@ -517,7 +515,7 @@
char pid_str[256];
::memset (pid_str, 0, sizeof(pid_str));
ConnectionStatus status;
- const size_t pid_str_len = file_conn.Read (pid_str, sizeof(pid_str), status, NULL);
+ const size_t pid_str_len = file_conn.Read (pid_str, sizeof(pid_str), NULL, status, NULL);
if (pid_str_len > 0)
{
int pid = atoi (pid_str);
diff --git a/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h b/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h
index 1fddaeb..cce0e4e 100644
--- a/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h
+++ b/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h
@@ -37,7 +37,7 @@
~GDBRemoteCommunicationServer();
bool
- GetPacketAndSendResponse (const lldb_private::TimeValue* timeout_ptr,
+ GetPacketAndSendResponse (uint32_t timeout_usec,
lldb_private::Error &error,
bool &interrupt,
bool &quit);