mgorny created this revision.
mgorny added reviewers: labath, krytarowski, emaste.
Herald added a subscriber: arichardson.
Herald added a project: All.
mgorny requested review of this revision.
The current GDBRemoteCommunication::ReadPacket() support
for notifications is very limited. Notably, it can return notifications
when the caller does not expect them and it does not provide an explicit
API to distinguish them from standard packets. This is not a major
problem right now, as we force serialization when running in non-stop
mode and can predict when we expect notifications to arrive.
Firstly, add an explicit 'allow_notification' parameter to ReadPacket()
and WaitForPacketNoLock(). When it is false (the default),
notifications are pushed onto a queue rather than returned
to the caller. Effectively, the existing callers can now safely expect
the reading functions to return only standard responses,
and the non-stop logic can explicitly permit notifications.
Secondly, add an explicit PacketResult value for notifications. This
value can only be returned when allow_notification is true, so it does
not require changes for most of the callers. For the few that support
notifications, it makes it possible to distinguish standard packets
from notification packets.
Update the non-stop client code to take advantage of the new API,
and therefore be more strict when dealing with responses in non-stop
protocol mode.
Sponsored by: The FreeBSD Foundation
https://reviews.llvm.org/D126655
Files:
lldb/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h
===================================================================
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h
@@ -12,6 +12,7 @@
#include "GDBRemoteCommunicationHistory.h"
#include <condition_variable>
+#include <deque>
#include <future>
#include <mutex>
#include <queue>
@@ -89,17 +90,18 @@
enum class PacketType { Invalid = 0, Standard, Notify };
enum class PacketResult {
- Success = 0, // Success
- ErrorSendFailed, // Status sending the packet
- ErrorSendAck, // Didn't get an ack back after sending a packet
- ErrorReplyFailed, // Status getting the reply
- ErrorReplyTimeout, // Timed out waiting for reply
- ErrorReplyInvalid, // Got a reply but it wasn't valid for the packet that
- // was sent
- ErrorReplyAck, // Sending reply ack failed
- ErrorDisconnected, // We were disconnected
- ErrorNoSequenceLock // We couldn't get the sequence lock for a multi-packet
- // request
+ Success = 0, // Success
+ ErrorSendFailed, // Status sending the packet
+ ErrorSendAck, // Didn't get an ack back after sending a packet
+ ErrorReplyFailed, // Status getting the reply
+ ErrorReplyTimeout, // Timed out waiting for reply
+ ErrorReplyInvalid, // Got a reply but it wasn't valid for the packet that
+ // was sent
+ ErrorReplyAck, // Sending reply ack failed
+ ErrorDisconnected, // We were disconnected
+ ErrorNoSequenceLock, // We couldn't get the sequence lock for a multi-packet
+ // request
+ Notify, // Successfully gotten a notification packet
};
// Class to change the timeout for a given scope and restore it to the
@@ -179,6 +181,7 @@
bool m_is_platform; // Set to true if this class represents a platform,
// false if this class represents a debug session for
// a single process
+ std::deque<std::string> m_notification_packet_queue;
CompressionType m_compression_type;
@@ -190,7 +193,8 @@
bool skip_ack = false);
PacketResult ReadPacket(StringExtractorGDBRemote &response,
- Timeout<std::micro> timeout, bool sync_on_timeout);
+ Timeout<std::micro> timeout, bool sync_on_timeout,
+ bool allow_notification = false);
PacketResult ReadPacketWithOutputSupport(
StringExtractorGDBRemote &response, Timeout<std::micro> timeout,
@@ -199,7 +203,8 @@
PacketResult WaitForPacketNoLock(StringExtractorGDBRemote &response,
Timeout<std::micro> timeout,
- bool sync_on_timeout);
+ bool sync_on_timeout,
+ bool allow_notification = false);
bool CompressionIsEnabled() {
return m_compression_type != CompressionType::None;
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
===================================================================
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
@@ -239,16 +239,15 @@
return result;
}
-GDBRemoteCommunication::PacketResult
-GDBRemoteCommunication::ReadPacket(StringExtractorGDBRemote &response,
- Timeout<std::micro> timeout,
- bool sync_on_timeout) {
+GDBRemoteCommunication::PacketResult GDBRemoteCommunication::ReadPacket(
+ StringExtractorGDBRemote &response, Timeout<std::micro> timeout,
+ bool sync_on_timeout, bool allow_notification) {
using ResponseType = StringExtractorGDBRemote::ResponseType;
Log *log = GetLog(GDBRLog::Packets);
for (;;) {
- PacketResult result =
- WaitForPacketNoLock(response, timeout, sync_on_timeout);
+ PacketResult result = WaitForPacketNoLock(
+ response, timeout, sync_on_timeout, allow_notification);
if (result != PacketResult::Success ||
(response.GetResponseType() != ResponseType::eAck &&
response.GetResponseType() != ResponseType::eNack))
@@ -260,15 +259,30 @@
GDBRemoteCommunication::PacketResult
GDBRemoteCommunication::WaitForPacketNoLock(StringExtractorGDBRemote &packet,
Timeout<std::micro> timeout,
- bool sync_on_timeout) {
+ bool sync_on_timeout,
+ bool allow_notification) {
uint8_t buffer[8192];
Status error;
Log *log = GetLog(GDBRLog::Packets);
- // Check for a packet from our cache first without trying any reading...
- if (CheckForPacket(nullptr, 0, packet) != PacketType::Invalid)
+ // If the caller expects notifications, check the notification queue first.
+ if (allow_notification && !m_notification_packet_queue.empty()) {
+ packet.Reset(m_notification_packet_queue.front());
+ m_notification_packet_queue.pop_front();
return PacketResult::Success;
+ }
+
+ // Check for a packet from our cache first without trying any reading...
+ PacketType packet_type = CheckForPacket(nullptr, 0, packet);
+ if (packet_type == PacketType::Notify && !allow_notification) {
+ // If the caller does not support notifications, queue it for later.
+ m_notification_packet_queue.push_back(packet.GetStringRef().str());
+ packet_type = CheckForPacket(nullptr, 0, packet);
+ }
+ if (packet_type != PacketType::Invalid)
+ return packet_type == PacketType::Standard ? PacketResult::Success
+ : PacketResult::Notify;
bool timed_out = false;
bool disconnected = false;
@@ -283,8 +297,12 @@
bytes_read);
if (bytes_read > 0) {
- if (CheckForPacket(buffer, bytes_read, packet) != PacketType::Invalid)
- return PacketResult::Success;
+ packet_type = CheckForPacket(buffer, bytes_read, packet);
+ if (packet_type == PacketType::Notify && !allow_notification)
+ m_notification_packet_queue.push_back(packet.GetStringRef().str());
+ else if (packet_type != PacketType::Invalid)
+ return packet_type == PacketType::Standard ? PacketResult::Success
+ : PacketResult::Notify;
} else {
switch (status) {
case eConnectionStatusTimedOut:
@@ -1315,6 +1333,9 @@
case PacketResult::ErrorNoSequenceLock:
Stream << "ErrorNoSequenceLock";
break;
+ case PacketResult::Notify:
+ Stream << "Notify";
+ break;
}
}
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp
===================================================================
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp
@@ -60,7 +60,8 @@
std::chrono::seconds computed_timeout =
std::min(interrupt_timeout, kWakeupInterval);
for (;;) {
- PacketResult read_result = ReadPacket(response, computed_timeout, false);
+ PacketResult read_result =
+ ReadPacket(response, computed_timeout, false, m_non_stop_protocol);
// Reset the computed_timeout to the default value in case we are going
// round again.
computed_timeout = std::min(interrupt_timeout, kWakeupInterval);
@@ -86,6 +87,7 @@
}
break;
}
+ case PacketResult::Notify:
case PacketResult::Success:
break;
default:
@@ -107,10 +109,11 @@
// 4) issue a "vCtrlC" command to ensure that all threads stop
// 5) repeat 1-3 for the response to this packet
- if (response.IsOKResponse())
+ if (read_result == PacketResult::Success && m_non_stop_protocol &&
+ response.IsOKResponse())
continue;
- // TODO: get a proper mechanism for async notifications
- if (response.GetStringRef().startswith("Stop:")) {
+ if (read_result == PacketResult::Notify &&
+ response.GetStringRef().startswith("Stop:")) {
response.Reset(response.GetStringRef().substr(5));
if (!DrainNotificationQueue("vStopped"))
@@ -144,8 +147,8 @@
}
// vCtrlC may not do anything, so timeout if we don't get notification
- if (ReadPacket(stop_response, milliseconds(500), false) ==
- PacketResult::Success) {
+ if (ReadPacket(stop_response, milliseconds(500), false, true) ==
+ PacketResult::Notify) {
if (!stop_response.GetStringRef().startswith("Stop:")) {
LLDB_LOGF(log,
"GDBRemoteClientBase::%s () unexpected response "
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
