[Lldb-commits] [PATCH] D135031: [lldb] [llgs] Move client-server communication into a separate thread (WIP)
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
+#include
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 m_comm_loop_up;
+ std::deque m_comm_sync_packet_queue;
+
+ void CommThreadReadHandler(MainLoopBase );
+ lldb::thread_result_t CommThread();
+
+ bool RequestComm(const MainLoop::Callback _callback,
+ const MainLoop::Callback _callback);
+
PacketResult
SendPacketAndWaitForResponseNoLock(llvm::StringRef payload,
StringExtractorGDBRemote );
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 ) {
- 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, _result](MainLoopBase &) {
+
[Lldb-commits] [PATCH] D135030: [lldb] [gdb-remote] Abstract sending ^c packet into SendCtrlC() method
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.
Abstract sending ^c packet into a dedicated
GDBRemoteClientBase::SendCtrlC() method. This makes it possible to avoid
exposing the complete Write() method in the API.
Sponsored by: The FreeBSD Foundation
https://reviews.llvm.org/D135030
Files:
lldb/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp
lldb/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.h
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
@@ -106,8 +106,7 @@
size_t SendAck();
- size_t Write(const void *src, size_t src_len, lldb::ConnectionStatus ,
- Status *error_ptr);
+ bool SendCtrlC();
bool IsConnected() const;
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
@@ -374,10 +374,7 @@
if (m_comm.m_async_count == 1) {
// The sender has sent the continue packet and we are the first async
// packet. Let's interrupt it.
- const char ctrl_c = '\x03';
- ConnectionStatus status = eConnectionStatusSuccess;
- size_t bytes_written = m_comm.Write(_c, 1, status, nullptr);
- if (bytes_written == 0) {
+ if (!m_comm.SendCtrlC()) {
--m_comm.m_async_count;
LLDB_LOGF(log, "GDBRemoteClientBase::Lock::Lock failed to send "
"interrupt packet");
@@ -417,9 +414,11 @@
size_t GDBRemoteClientBase::SendAck() { return m_comm.SendAck(); }
-size_t GDBRemoteClientBase::Write(const void *src, size_t src_len,
- ConnectionStatus , Status *error_ptr)
{
- return m_comm.Write(src, src_len, status, error_ptr);
+bool GDBRemoteClientBase::SendCtrlC() {
+ const char ctrl_c = '\x03';
+ ConnectionStatus status = eConnectionStatusSuccess;
+ size_t bytes_written = m_comm.Write(_c, 1, status, nullptr);
+ return bytes_written != 0;
}
bool GDBRemoteClientBase::IsConnected() const { return m_comm.IsConnected(); }
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
@@ -106,8 +106,7 @@
size_t SendAck();
- size_t Write(const void *src, size_t src_len, lldb::ConnectionStatus ,
- Status *error_ptr);
+ bool SendCtrlC();
bool IsConnected() const;
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
@@ -374,10 +374,7 @@
if (m_comm.m_async_count == 1) {
// The sender has sent the continue packet and we are the first async
// packet. Let's interrupt it.
- const char ctrl_c = '\x03';
- ConnectionStatus status = eConnectionStatusSuccess;
- size_t bytes_written = m_comm.Write(_c, 1, status, nullptr);
- if (bytes_written == 0) {
+ if (!m_comm.SendCtrlC()) {
--m_comm.m_async_count;
LLDB_LOGF(log, "GDBRemoteClientBase::Lock::Lock failed to send "
"interrupt packet");
@@ -417,9 +414,11 @@
size_t GDBRemoteClientBase::SendAck() { return m_comm.SendAck(); }
-size_t GDBRemoteClientBase::Write(const void *src, size_t src_len,
- ConnectionStatus , Status *error_ptr) {
- return m_comm.Write(src, src_len, status, error_ptr);
+bool GDBRemoteClientBase::SendCtrlC() {
+ const char ctrl_c = '\x03';
+ ConnectionStatus status = eConnectionStatusSuccess;
+ size_t bytes_written = m_comm.Write(_c, 1, status, nullptr);
+ return bytes_written != 0;
}
bool GDBRemoteClientBase::IsConnected() const { return m_comm.IsConnected(); }
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D135029: [lldb] [gdb-remote] Isolate ClientBase from Communication
mgorny created this revision.
mgorny added reviewers: labath, jingham, krytarowski, emaste.
Herald added a subscriber: arichardson.
Herald added a project: All.
mgorny requested review of this revision.
Introduce an isolation layer between GDBRemoteClientBase
and GDBRemoteCommunication. This makes it possible to replace
the public packet reading/writing API with wrappers that will make
threaded access possible in the future.
Sponsored by: The FreeBSD Foundation
https://reviews.llvm.org/D135029
Files:
lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
lldb/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp
lldb/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.h
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
lldb/source/Plugins/Process/gdb-remote/GDBRemoteRegisterContext.cpp
lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
lldb/unittests/Process/gdb-remote/GDBRemoteClientBaseTest.cpp
lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp
lldb/unittests/tools/lldb-server/tests/TestClient.cpp
Index: lldb/unittests/tools/lldb-server/tests/TestClient.cpp
===
--- lldb/unittests/tools/lldb-server/tests/TestClient.cpp
+++ lldb/unittests/tools/lldb-server/tests/TestClient.cpp
@@ -31,8 +31,8 @@
#endif
TestClient::TestClient(std::unique_ptr Conn) {
- SetConnection(std::move(Conn));
- SetPacketTimeout(std::chrono::seconds(10));
+ GetCommunication().SetConnection(std::move(Conn));
+ GetCommunication().SetPacketTimeout(std::chrono::seconds(10));
}
TestClient::~TestClient() {
@@ -50,7 +50,7 @@
if (Error E = SendMessage("QStartNoAckMode"))
return E;
- m_send_acks = false;
+ GetCommunication().DisableSendingAcks();
return Error::success();
}
@@ -268,7 +268,8 @@
m_stop_reply = std::move(*StopReplyOr);
if (!isa(m_stop_reply)) {
StringExtractorGDBRemote R;
-PacketResult result = ReadPacket(R, GetPacketTimeout(), false);
+PacketResult result =
+ReadPacket(R, GetCommunication().GetPacketTimeout(), false);
if (result != PacketResult::ErrorDisconnected) {
return make_error(
formatv("Expected connection close after sending {0}. Got {1}/{2} "
Index: lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp
===
--- lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp
+++ lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp
@@ -29,7 +29,9 @@
typedef GDBRemoteCommunication::PacketResult PacketResult;
struct TestClient : public GDBRemoteCommunicationClient {
- TestClient() { m_send_acks = false; }
+ TestClient() {
+GetCommunication().DisableSendingAcks();
+ }
};
void Handle_QThreadSuffixSupported(MockServer , bool supported) {
@@ -62,7 +64,8 @@
class GDBRemoteCommunicationClientTest : public GDBRemoteTest {
public:
void SetUp() override {
-ASSERT_THAT_ERROR(GDBRemoteCommunication::ConnectLocally(client, server),
+ASSERT_THAT_ERROR(GDBRemoteCommunication::ConnectLocally(
+ client.GetCommunication(), server),
llvm::Succeeded());
}
Index: lldb/unittests/Process/gdb-remote/GDBRemoteClientBaseTest.cpp
===
--- lldb/unittests/Process/gdb-remote/GDBRemoteClientBaseTest.cpp
+++ lldb/unittests/Process/gdb-remote/GDBRemoteClientBaseTest.cpp
@@ -40,14 +40,15 @@
struct TestClient : public GDBRemoteClientBase {
TestClient() : GDBRemoteClientBase("test.client") {
-m_send_acks = false;
+GetCommunication().DisableSendingAcks();
}
};
class GDBRemoteClientBaseTest : public GDBRemoteTest {
public:
void SetUp() override {
-ASSERT_THAT_ERROR(GDBRemoteCommunication::ConnectLocally(client, server),
+ASSERT_THAT_ERROR(GDBRemoteCommunication::ConnectLocally(
+ client.GetCommunication(), server),
llvm::Succeeded());
ASSERT_EQ(TestClient::eBroadcastBitRunPacketSent,
listener_sp->StartListeningForEvents(
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
@@ -109,7 +109,10 @@
return;
}
StreamFile stream(std::move(file.get()));
- ((ProcessGDBRemote *)p)->GetGDBRemote().DumpHistory(stream);
+ ((ProcessGDBRemote *)p)
+ ->GetGDBRemote()
+ .GetCommunication()
+ .DumpHistory(stream);
}
} // namespace lldb
@@ -281,7 +284,8 @@
[Lldb-commits] [PATCH] D135028: [lldb] [gdb-remote] Move ReadPacketWithOutputSupport() to client
mgorny created this revision.
mgorny added reviewers: labath, emaste, krytarowski, jingham.
Herald added a project: All.
mgorny requested review of this revision.
Move ReadPacketWithOutputSupport() from GDBRemoteCommunication
to GDBRemoteClientBase. This function is client-specific and moving
it there simplifies followup patches that split communication into
separate thread.
https://reviews.llvm.org/D135028
Files:
lldb/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp
lldb/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.h
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
@@ -190,11 +190,6 @@
PacketResult ReadPacket(StringExtractorGDBRemote ,
Timeout timeout, bool sync_on_timeout);
- PacketResult ReadPacketWithOutputSupport(
- StringExtractorGDBRemote , Timeout timeout,
- bool sync_on_timeout,
- llvm::function_ref output_callback);
-
PacketResult WaitForPacketNoLock(StringExtractorGDBRemote ,
Timeout timeout,
bool sync_on_timeout);
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
@@ -220,23 +220,6 @@
return result;
}
-GDBRemoteCommunication::PacketResult
-GDBRemoteCommunication::ReadPacketWithOutputSupport(
-StringExtractorGDBRemote , Timeout timeout,
-bool sync_on_timeout,
-llvm::function_ref output_callback) {
- auto result = ReadPacket(response, timeout, sync_on_timeout);
- while (result == PacketResult::Success && response.IsNormalResponse() &&
- response.PeekChar() == 'O') {
-response.GetChar();
-std::string output;
-if (response.GetHexByteString(output))
- output_callback(output);
-result = ReadPacket(response, timeout, sync_on_timeout);
- }
- return result;
-}
-
GDBRemoteCommunication::PacketResult
GDBRemoteCommunication::ReadPacket(StringExtractorGDBRemote ,
Timeout timeout,
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
@@ -58,6 +58,11 @@
llvm::StringRef payload, StringExtractorGDBRemote ,
std::chrono::seconds interrupt_timeout = std::chrono::seconds(0));
+ PacketResult ReadPacketWithOutputSupport(
+ StringExtractorGDBRemote , Timeout timeout,
+ bool sync_on_timeout,
+ llvm::function_ref output_callback);
+
PacketResult SendPacketAndReceiveResponseWithOutputSupport(
llvm::StringRef payload, StringExtractorGDBRemote ,
std::chrono::seconds interrupt_timeout,
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
@@ -194,6 +194,23 @@
return SendPacketAndWaitForResponseNoLock(payload, response);
}
+GDBRemoteCommunication::PacketResult
+GDBRemoteClientBase::ReadPacketWithOutputSupport(
+StringExtractorGDBRemote , Timeout timeout,
+bool sync_on_timeout,
+llvm::function_ref output_callback) {
+ auto result = ReadPacket(response, timeout, sync_on_timeout);
+ while (result == PacketResult::Success && response.IsNormalResponse() &&
+ response.PeekChar() == 'O') {
+response.GetChar();
+std::string output;
+if (response.GetHexByteString(output))
+ output_callback(output);
+result = ReadPacket(response, timeout, sync_on_timeout);
+ }
+ return result;
+}
+
GDBRemoteCommunication::PacketResult
GDBRemoteClientBase::SendPacketAndReceiveResponseWithOutputSupport(
llvm::StringRef payload, StringExtractorGDBRemote ,
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
@@ -190,11 +190,6 @@
PacketResult ReadPacket(StringExtractorGDBRemote ,
Timeout timeout, bool sync_on_timeout);
- PacketResult ReadPacketWithOutputSupport(
- StringExtractorGDBRemote , Timeout timeout,
- bool sync_on_timeout,
-
[Lldb-commits] [PATCH] D134133: WIP: [lldb][COFF] Enhance symtab loading of symbol and export tables
alvinhochun abandoned this revision. alvinhochun added a comment. Handled in separate changes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D134133/new/ https://reviews.llvm.org/D134133 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
