mgorny created this revision.
mgorny added reviewers: krytarowski, emaste, labath, jingham.
Herald added a subscriber: arichardson.
Herald added a project: All.
mgorny requested review of this revision.
Perform all the communication between client and server in a dedicated
thread that's started after establishing the connection. The thread
uses a MainLoop instance to read all incoming packets into a queue,
and handle asynchronous requests from the client via
the AddPendingCallback() API.
Using a dedicated thread will make it possible to continuously read
asynchronous notifications from running processes while permitting
the plugin to simultaneously handle synchronous requests in multiprocess
mode.
Sponsored by: The FreeBSD Foundation
TODO:
- implement complete error handling
- implement timeouts
https://reviews.llvm.org/D135031
Files:
lldb/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp
lldb/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.h
lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
Index: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
===================================================================
--- lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -886,6 +886,11 @@
return error;
}
+ llvm::Error thread_error = m_gdb_comm.StartThread();
+ if (thread_error) {
+ m_gdb_comm.Disconnect();
+ return Status(std::move(thread_error));
+ }
// We always seem to be able to open a connection to a local port so we need
// to make sure we can then send data to it. If we can't then we aren't
// actually connected to anything, so try and do the handshake with the
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.h
===================================================================
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.h
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.h
@@ -11,7 +11,11 @@
#include "GDBRemoteCommunication.h"
+#include "lldb/Host/HostThread.h"
+#include "lldb/Host/MainLoop.h"
+
#include <condition_variable>
+#include <deque>
namespace lldb_private {
namespace process_gdb_remote {
@@ -22,6 +26,9 @@
enum {
eBroadcastBitRunPacketSent = (1u << 0),
+ eBroadcastBitCommDone = (1u << 1),
+ eBroadcastBitCommThreadExited = (1u << 2),
+ eBroadcastBitCommPacketRecv = (1u << 3),
};
struct ContinueDelegate {
@@ -116,9 +123,23 @@
return m_comm;
}
+ llvm::Error StartThread();
+ void StopThread();
+
protected:
GDBRemoteCommunication m_comm;
+ HostThread m_comm_thread;
+ bool m_comm_thread_exited;
+ std::unique_ptr<MainLoop> m_comm_loop_up;
+ std::deque<std::string> m_comm_sync_packet_queue;
+
+ void CommThreadReadHandler(MainLoopBase &loop);
+ lldb::thread_result_t CommThread();
+
+ bool RequestComm(const MainLoop::Callback &send_callback,
+ const MainLoop::Callback &recv_callback);
+
PacketResult
SendPacketAndWaitForResponseNoLock(llvm::StringRef payload,
StringExtractorGDBRemote &response);
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
@@ -10,7 +10,10 @@
#include "llvm/ADT/StringExtras.h"
+#include "lldb/Host/ThreadLauncher.h"
#include "lldb/Target/UnixSignals.h"
+#include "lldb/Utility/Connection.h"
+#include "lldb/Utility/Event.h"
#include "lldb/Utility/LLDBAssert.h"
#include "ProcessGDBRemoteLog.h"
@@ -237,29 +240,50 @@
GDBRemoteCommunication::PacketResult
GDBRemoteClientBase::SendPacketAndWaitForResponseNoLock(
llvm::StringRef payload, StringExtractorGDBRemote &response) {
- PacketResult packet_result = SendPacketNoLock(payload);
- if (packet_result != PacketResult::Success)
- return packet_result;
-
+ PacketResult packet_result;
const size_t max_response_retries = 3;
- for (size_t i = 0; i < max_response_retries; ++i) {
- packet_result = ReadPacket(response, m_comm.GetPacketTimeout(), true);
- // Make sure we received a response
- if (packet_result != PacketResult::Success)
- return packet_result;
- // Make sure our response is valid for the payload that was sent
- if (response.ValidateResponse())
- return packet_result;
- // Response says it wasn't valid
- Log *log = GetLog(GDBRLog::Packets);
- LLDB_LOGF(
- log,
- "error: packet with payload \"%.*s\" got invalid response \"%s\": %s",
- int(payload.size()), payload.data(), response.GetStringRef().data(),
- (i == (max_response_retries - 1))
- ? "using invalid response and giving up"
- : "ignoring response and waiting for another");
- }
+ size_t response_retry = 0;
+
+ // TODO: timeout
+ if (!RequestComm(
+ [this, payload, &packet_result](MainLoopBase &) {
+ packet_result = m_comm.SendPacketNoLock(payload);
+ if (packet_result != PacketResult::Success)
+ BroadcastEvent(eBroadcastBitCommDone);
+ },
+ [this, &packet_result, &payload, &response,
+ &response_retry](MainLoopBase &) {
+ while (!m_comm_sync_packet_queue.empty()) {
+ response.Reset(m_comm_sync_packet_queue.front());
+ m_comm_sync_packet_queue.pop_front();
+
+ // Make sure our response is valid for the payload that was sent
+ if (response.ValidateResponse())
+ break;
+
+ // Response says it wasn't valid
+ Log *log = GetLog(GDBRLog::Packets);
+ LLDB_LOGF(log,
+ "error: packet with payload \"%.*s\" got invalid "
+ "response \"%s\": %s",
+ int(payload.size()), payload.data(),
+ response.GetStringRef().data(),
+ (response_retry == (max_response_retries - 1))
+ ? "using invalid response and giving up"
+ : "ignoring response and waiting for another");
+
+ // If we get (max_response_retries) invalid responses,
+ // return the invalid response.
+ ++response_retry;
+ if (response_retry == max_response_retries)
+ break;
+ }
+
+ packet_result = PacketResult::Success;
+ BroadcastEvent(eBroadcastBitCommDone);
+ }))
+ return PacketResult::ErrorDisconnected; // TODO
+
return packet_result;
}
@@ -400,29 +424,192 @@
m_comm.m_cv.notify_one();
}
+bool GDBRemoteClientBase::RequestComm(const MainLoop::Callback &send_callback,
+ const MainLoop::Callback &recv_callback) {
+ assert(m_comm_thread.IsJoinable());
+ ListenerSP listener_sp(Listener::MakeListener("gdb-remote.packet-sent"));
+ if (listener_sp->StartListeningForEvents(
+ this, eBroadcastBitCommDone | eBroadcastBitCommThreadExited |
+ eBroadcastBitCommPacketRecv)) {
+ bool packet_read = false;
+ m_comm_loop_up->AddPendingCallback(send_callback);
+ // If the server replies very fast, the client may read response as part of
+ // ack packet. Queue a read handler run to verify that we do not have any
+ // packets in the read buffer already.
+ m_comm_loop_up->AddPendingCallback(
+ [this, &packet_read, &recv_callback](MainLoopBase &loop) {
+ CommThreadReadHandler(loop);
+ if (recv_callback) {
+ if (!packet_read && !m_comm_sync_packet_queue.empty()) {
+ recv_callback(loop);
+ packet_read = true;
+ }
+ }
+ });
+
+ EventSP event_sp;
+ while (!m_comm_thread_exited &&
+ listener_sp->GetEvent(event_sp, llvm::None)) {
+ if (event_sp->GetType() & eBroadcastBitCommDone)
+ return true;
+ if (event_sp->GetType() & eBroadcastBitCommThreadExited)
+ return false;
+ if (event_sp->GetType() & eBroadcastBitCommPacketRecv) {
+ m_comm_loop_up->AddPendingCallback(
+ [this, &packet_read, &recv_callback](MainLoopBase &loop) {
+ if (!packet_read && !m_comm_sync_packet_queue.empty()) {
+ recv_callback(loop);
+ packet_read = true;
+ }
+ });
+ }
+ }
+ }
+ return false;
+}
+
GDBRemoteClientBase::PacketResult
GDBRemoteClientBase::ReadPacket(StringExtractorGDBRemote &response,
Timeout<std::micro> timeout,
bool sync_on_timeout) {
- return m_comm.ReadPacket(response, timeout, sync_on_timeout);
+ PacketResult packet_result;
+
+ if (!RequestComm([](MainLoopBase &) {},
+ [this, &packet_result, &response](MainLoopBase &) {
+ assert(!m_comm_sync_packet_queue.empty());
+
+ response.Reset(m_comm_sync_packet_queue.front());
+ m_comm_sync_packet_queue.pop_front();
+
+ packet_result = PacketResult::Success;
+ BroadcastEvent(eBroadcastBitCommDone);
+ }))
+ return PacketResult::ErrorDisconnected; // TODO
+
+ return packet_result;
}
GDBRemoteClientBase::PacketResult
GDBRemoteClientBase::SendPacketNoLock(llvm::StringRef payload) {
- return m_comm.SendPacketNoLock(payload);
+ PacketResult packet_result;
+
+ if (!RequestComm(
+ [this, &packet_result, payload](MainLoopBase &) {
+ packet_result = m_comm.SendPacketNoLock(payload);
+ BroadcastEvent(eBroadcastBitCommDone);
+ },
+ nullptr))
+ return PacketResult::ErrorDisconnected; // TODO
+
+ return packet_result;
}
-size_t GDBRemoteClientBase::SendAck() { return m_comm.SendAck(); }
+size_t GDBRemoteClientBase::SendAck() {
+ size_t bytes_written;
+
+ if (RequestComm(
+ [this, &bytes_written](MainLoopBase &) {
+ bytes_written = m_comm.SendAck();
+ BroadcastEvent(eBroadcastBitCommDone);
+ },
+ nullptr))
+ return bytes_written;
+ return 0;
+}
bool GDBRemoteClientBase::SendCtrlC() {
- const char ctrl_c = '\x03';
ConnectionStatus status = eConnectionStatusSuccess;
- size_t bytes_written = m_comm.Write(&ctrl_c, 1, status, nullptr);
- return bytes_written != 0;
+ size_t bytes_written;
+
+ if (RequestComm(
+ [this, &bytes_written, &status](MainLoopBase &) {
+ const char ctrl_c = '\x03';
+ bytes_written = m_comm.Write(&ctrl_c, 1, status, nullptr);
+ BroadcastEvent(eBroadcastBitCommDone);
+ },
+ nullptr))
+ return bytes_written != 0;
+ return false;
}
bool GDBRemoteClientBase::IsConnected() const { return m_comm.IsConnected(); }
ConnectionStatus GDBRemoteClientBase::Disconnect(Status *error_ptr) {
+ StopThread();
return m_comm.Disconnect(error_ptr);
}
+
+llvm::Error GDBRemoteClientBase::StartThread() {
+ Log *log = GetLog(GDBRLog::Process);
+ LLDB_LOG(log, "Starting comm thread");
+ assert(!m_comm_thread.IsJoinable());
+
+ // Instantiate the loop early to avoid races.
+ m_comm_loop_up.reset(new MainLoop());
+ m_comm_thread_exited = false;
+
+ auto maybe_thread = ThreadLauncher::LaunchThread(
+ "<lldb.gdb-remote.comm>", [this] { return CommThread(); });
+ if (!maybe_thread)
+ return maybe_thread.takeError();
+
+ m_comm_thread = *maybe_thread;
+ assert(m_comm_thread.IsJoinable());
+
+ return llvm::Error::success();
+}
+
+void GDBRemoteClientBase::StopThread() {
+ Log *log = GetLog(GDBRLog::Process);
+
+ if (!m_comm_thread.IsJoinable())
+ return;
+
+ LLDB_LOG(log, "Stopping comm thread");
+ m_comm_loop_up->AddPendingCallback(
+ [](MainLoopBase &loop) { loop.RequestTermination(); });
+ if (m_comm_thread.Join(nullptr).Success())
+ m_comm_loop_up.reset(nullptr);
+ assert(m_comm_thread_exited);
+}
+
+void GDBRemoteClientBase::CommThreadReadHandler(MainLoopBase &loop) {
+ StringExtractorGDBRemote response;
+
+ while (true) {
+ // Read incoming packets until we reach timeout (i.e. all pending packets have been processed).
+ PacketResult packet_result =
+ m_comm.ReadPacket(response, std::chrono::seconds(0), false);
+
+ if (packet_result == PacketResult::ErrorReplyTimeout)
+ break;
+ if (packet_result != PacketResult::Success) {
+ loop.RequestTermination();
+ break;
+ }
+
+ m_comm_sync_packet_queue.push_back(response.GetStringRef().str());
+ BroadcastEvent(eBroadcastBitCommPacketRecv);
+ }
+}
+
+lldb::thread_result_t GDBRemoteClientBase::CommThread() {
+ Log *log = GetLog(GDBRLog::Process);
+ assert(m_comm_loop_up);
+
+ Status error;
+ auto handle = m_comm_loop_up->RegisterReadObject(
+ m_comm.GetConnection()->GetReadObject(),
+ [this](MainLoopBase &loop) { CommThreadReadHandler(loop); }, error);
+ if (error.Success())
+ error = m_comm_loop_up->Run();
+ if (!error.Success()) {
+ // TODO
+ }
+
+ LLDB_LOG(log, "Comm thread exiting");
+ m_comm_thread_exited = true;
+ BroadcastEvent(eBroadcastBitCommThreadExited);
+
+ return {};
+}
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
