mgorny created this revision.
mgorny added reviewers: labath, emaste, jingham, krytarowski.
Herald added a subscriber: arichardson.
Herald added a project: All.
mgorny requested review of this revision.
Add explicit support for reading (or skipping) notification packets
to GDBRemoteCommunication::ReadPacket(). Previously, the code would not
really distinguish between regular replies and notifications,
and would return the next packet to the caller without even a clear
indication whether it is a regular or notification packet.
Since the majority of existing call sites do not expect to read
notifications, add a new "allow_notification" parameter that defaults to
false. If it is false, then any notifications received from the server
are stashed onto a queue and the function waits for a regular reply
instead. If it is true, then the function either returns the first
notification queued previously or waits for any packet (either regular
or notification).
To make it possible to clearly distinguish between regular
and notification packets, add a new PacketResult::Notify. It is used
in lieu of PacketResult::Success when a notification packet is read.
While admittedly this is not the cleanest possible solution, it requires
minimal changes to the existing code. In the end, we expect only a few
call sites to actually expect and read notifications.
Split from the non-stop protocol support in D126614
<https://reviews.llvm.org/D126614>.
Sponsored by: The FreeBSD Foundation
https://reviews.llvm.org/D131094
Files:
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h
lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationTest.cpp
Index: lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationTest.cpp
===================================================================
--- lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationTest.cpp
+++ lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationTest.cpp
@@ -20,9 +20,11 @@
TestClient()
: GDBRemoteCommunication("test.client", "test.client.listener") {}
- PacketResult ReadPacket(StringExtractorGDBRemote &response) {
+ PacketResult ReadPacket(StringExtractorGDBRemote &response,
+ bool allow_notify = false) {
return GDBRemoteCommunication::ReadPacket(response, std::chrono::seconds(1),
- /*sync_on_timeout*/ false);
+ /*sync_on_timeout*/ false,
+ allow_notify);
}
};
@@ -69,3 +71,23 @@
ASSERT_EQ(PacketResult::Success, server.GetAck());
}
}
+
+TEST_F(GDBRemoteCommunicationTest, ReadPacketNotify) {
+ StringExtractorGDBRemote response;
+ ASSERT_TRUE(Write("%Stdio:O48656c6c6f20776f726c640d0a#f1"));
+ ASSERT_TRUE(Write("%Stop:W29;process:114b63#97"));
+ ASSERT_EQ(PacketResult::Notify, client.ReadPacket(response, true));
+ ASSERT_EQ("Stdio:O48656c6c6f20776f726c640d0a", response.GetStringRef());
+ ASSERT_EQ(PacketResult::Notify, client.ReadPacket(response, true));
+ ASSERT_EQ("Stop:W29;process:114b63", response.GetStringRef());
+}
+
+TEST_F(GDBRemoteCommunicationTest, ReadPacketNotifyQueue) {
+ StringExtractorGDBRemote response;
+ ASSERT_TRUE(Write("%Stop:W29;process:114b63#97"));
+ ASSERT_TRUE(Write("$foobar#79"));
+ ASSERT_EQ(PacketResult::Success, client.ReadPacket(response));
+ ASSERT_EQ("foobar", response.GetStringRef());
+ ASSERT_EQ(PacketResult::Notify, client.ReadPacket(response, true));
+ ASSERT_EQ("Stop:W29;process:114b63", response.GetStringRef());
+}
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);
+ // 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::Notify;
+ }
+
// Check for a packet from our cache first without trying any reading...
- if (CheckForPacket(nullptr, 0, packet) != PacketType::Invalid)
- return PacketResult::Success;
+ 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,14 @@
bytes_read);
if (bytes_read > 0) {
- if (CheckForPacket(buffer, bytes_read, packet) != PacketType::Invalid)
- return PacketResult::Success;
+ packet_type = CheckForPacket(buffer, bytes_read, packet);
+ while (packet_type == PacketType::Notify && !allow_notification) {
+ 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;
} else {
switch (status) {
case eConnectionStatusTimedOut:
@@ -1315,6 +1335,9 @@
case PacketResult::ErrorNoSequenceLock:
Stream << "ErrorNoSequenceLock";
break;
+ case PacketResult::Notify:
+ Stream << "Notify";
+ break;
}
}
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
