[lldb] Remove GDBRemoteCommunication::ConnectLocally #145293
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Originally added for reproducers, it is now only used for test code. While we could make it a test helper, I think that after llvm#145015 it is simple enough to not be needed. Also squeeze in a change to make ConnectionFileDescriptor accept a unique_ptr<Socket>.
|
@llvm/pr-subscribers-lldb Author: Pavel Labath (labath) ChangesOriginally added for reproducers, it is now only used for test code. While we could make it a test helper, I think that after #145015 it is simple enough to not be needed. Also squeeze in a change to make ConnectionFileDescriptor accept a unique_ptr<Socket>. Full diff: https://github.com/llvm/llvm-project/pull/145293.diff 11 Files Affected:
diff --git a/lldb/include/lldb/Host/posix/ConnectionFileDescriptorPosix.h b/lldb/include/lldb/Host/posix/ConnectionFileDescriptorPosix.h
index b771f9c3f45a2..df0e196fea688 100644
--- a/lldb/include/lldb/Host/posix/ConnectionFileDescriptorPosix.h
+++ b/lldb/include/lldb/Host/posix/ConnectionFileDescriptorPosix.h
@@ -18,13 +18,10 @@
#include "lldb/Host/Pipe.h"
#include "lldb/Host/Socket.h"
#include "lldb/Utility/Connection.h"
-#include "lldb/Utility/IOObject.h"
namespace lldb_private {
class Status;
-class Socket;
-class SocketAddress;
class ConnectionFileDescriptor : public Connection {
public:
@@ -35,7 +32,7 @@ class ConnectionFileDescriptor : public Connection {
ConnectionFileDescriptor(int fd, bool owns_fd);
- ConnectionFileDescriptor(Socket *socket);
+ ConnectionFileDescriptor(std::unique_ptr<Socket> socket_up);
~ConnectionFileDescriptor() override;
@@ -136,8 +133,6 @@ class ConnectionFileDescriptor : public Connection {
std::string m_uri;
private:
- void InitializeSocket(Socket *socket);
-
ConnectionFileDescriptor(const ConnectionFileDescriptor &) = delete;
const ConnectionFileDescriptor &
operator=(const ConnectionFileDescriptor &) = delete;
diff --git a/lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp b/lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
index ae935dda5af4e..57dce44812c89 100644
--- a/lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
+++ b/lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
@@ -72,9 +72,11 @@ ConnectionFileDescriptor::ConnectionFileDescriptor(int fd, bool owns_fd)
OpenCommandPipe();
}
-ConnectionFileDescriptor::ConnectionFileDescriptor(Socket *socket)
- : Connection(), m_pipe(), m_mutex(), m_shutting_down(false) {
- InitializeSocket(socket);
+ConnectionFileDescriptor::ConnectionFileDescriptor(
+ std::unique_ptr<Socket> socket_up)
+ : m_shutting_down(false) {
+ m_uri = socket_up->GetRemoteConnectionURI();
+ m_io_sp = std::move(socket_up);
}
ConnectionFileDescriptor::~ConnectionFileDescriptor() {
@@ -796,8 +798,3 @@ ConnectionStatus ConnectionFileDescriptor::ConnectSerialPort(
#endif // LLDB_ENABLE_POSIX
llvm_unreachable("this function should be only called w/ LLDB_ENABLE_POSIX");
}
-
-void ConnectionFileDescriptor::InitializeSocket(Socket *socket) {
- m_io_sp.reset(socket);
- m_uri = socket->GetRemoteConnectionURI();
-}
diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
index d1f57cc22d8bd..0d3ead840b080 100644
--- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
@@ -1128,8 +1128,8 @@ Status GDBRemoteCommunication::StartDebugserverProcess(
Socket *accepted_socket = nullptr;
error = sock_up->Accept(/*timeout=*/std::nullopt, accepted_socket);
if (accepted_socket) {
- SetConnection(
- std::make_unique<ConnectionFileDescriptor>(accepted_socket));
+ SetConnection(std::make_unique<ConnectionFileDescriptor>(
+ std::unique_ptr<Socket>(accepted_socket)));
}
}
@@ -1138,20 +1138,6 @@ Status GDBRemoteCommunication::StartDebugserverProcess(
void GDBRemoteCommunication::DumpHistory(Stream &strm) { m_history.Dump(strm); }
-llvm::Error
-GDBRemoteCommunication::ConnectLocally(GDBRemoteCommunication &client,
- GDBRemoteCommunication &server) {
- llvm::Expected<Socket::Pair> pair = Socket::CreatePair();
- if (!pair)
- return pair.takeError();
-
- client.SetConnection(
- std::make_unique<ConnectionFileDescriptor>(pair->first.release()));
- server.SetConnection(
- std::make_unique<ConnectionFileDescriptor>(pair->second.release()));
- return llvm::Error::success();
-}
-
GDBRemoteCommunication::ScopedTimeout::ScopedTimeout(
GDBRemoteCommunication &gdb_comm, std::chrono::seconds timeout)
: m_gdb_comm(gdb_comm), m_saved_timeout(0), m_timeout_modified(false) {
diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h
index ee87629d9077b..fc86f801f0d8a 100644
--- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h
+++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h
@@ -149,9 +149,6 @@ class GDBRemoteCommunication : public Communication {
void DumpHistory(Stream &strm);
- static llvm::Error ConnectLocally(GDBRemoteCommunication &client,
- GDBRemoteCommunication &server);
-
/// Expand GDB run-length encoding.
static std::optional<std::string> ExpandRLE(std::string);
diff --git a/lldb/tools/lldb-server/lldb-platform.cpp b/lldb/tools/lldb-server/lldb-platform.cpp
index 37ec8455c63a7..3c79dc001f65e 100644
--- a/lldb/tools/lldb-server/lldb-platform.cpp
+++ b/lldb/tools/lldb-server/lldb-platform.cpp
@@ -485,8 +485,8 @@ int main_platform(int argc, char *argv[]) {
GDBRemoteCommunicationServerPlatform platform(socket->GetSocketProtocol(),
gdbserver_port);
- platform.SetConnection(std::unique_ptr<Connection>(
- new ConnectionFileDescriptor(socket.release())));
+ platform.SetConnection(
+ std::make_unique<ConnectionFileDescriptor>(std::move(socket)));
client_handle(platform, inferior_arguments);
return 0;
}
diff --git a/lldb/unittests/Core/CommunicationTest.cpp b/lldb/unittests/Core/CommunicationTest.cpp
index fd07b4da9f8c1..51e218af8baf4 100644
--- a/lldb/unittests/Core/CommunicationTest.cpp
+++ b/lldb/unittests/Core/CommunicationTest.cpp
@@ -41,7 +41,7 @@ static void CommunicationReadTest(bool use_read_thread) {
ThreadedCommunication comm("test");
comm.SetConnection(
- std::make_unique<ConnectionFileDescriptor>(pair->second.release()));
+ std::make_unique<ConnectionFileDescriptor>(std::move(pair->second)));
comm.SetCloseOnEOF(true);
if (use_read_thread) {
@@ -126,7 +126,7 @@ TEST_F(CommunicationTest, SynchronizeWhileClosing) {
ThreadedCommunication comm("test");
comm.SetConnection(
- std::make_unique<ConnectionFileDescriptor>(pair->second.release()));
+ std::make_unique<ConnectionFileDescriptor>(std::move(pair->second)));
comm.SetCloseOnEOF(true);
ASSERT_TRUE(comm.StartReadThread());
diff --git a/lldb/unittests/Host/ConnectionFileDescriptorTest.cpp b/lldb/unittests/Host/ConnectionFileDescriptorTest.cpp
index 250ed6fe8fd37..171481acca01b 100644
--- a/lldb/unittests/Host/ConnectionFileDescriptorTest.cpp
+++ b/lldb/unittests/Host/ConnectionFileDescriptorTest.cpp
@@ -22,11 +22,11 @@ class ConnectionFileDescriptorTest : public testing::Test {
std::unique_ptr<TCPSocket> socket_a_up;
std::unique_ptr<TCPSocket> socket_b_up;
CreateTCPConnectedSockets(ip, &socket_a_up, &socket_b_up);
- auto *socket = socket_a_up.release();
- ConnectionFileDescriptor connection_file_descriptor(socket);
+ uint16_t socket_a_remote_port = socket_a_up->GetRemotePortNumber();
+ ConnectionFileDescriptor connection_file_descriptor(std::move(socket_a_up));
std::string uri(connection_file_descriptor.GetURI());
- EXPECT_EQ((URI{"connect", ip, socket->GetRemotePortNumber(), "/"}),
+ EXPECT_EQ((URI{"connect", ip, socket_a_remote_port, "/"}),
*URI::Parse(uri));
}
};
diff --git a/lldb/unittests/Process/gdb-remote/GDBRemoteClientBaseTest.cpp b/lldb/unittests/Process/gdb-remote/GDBRemoteClientBaseTest.cpp
index ed77e86ac3701..b28b8013bcdc7 100644
--- a/lldb/unittests/Process/gdb-remote/GDBRemoteClientBaseTest.cpp
+++ b/lldb/unittests/Process/gdb-remote/GDBRemoteClientBaseTest.cpp
@@ -10,6 +10,7 @@
#include "GDBRemoteTestUtils.h"
#include "Plugins/Process/Utility/LinuxSignals.h"
#include "Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h"
+#include "lldb/Host/ConnectionFileDescriptor.h"
#include "lldb/Utility/GDBRemote.h"
#include "lldb/Utility/Listener.h"
#include "llvm/ADT/StringRef.h"
@@ -51,8 +52,12 @@ struct TestClient : public GDBRemoteClientBase {
class GDBRemoteClientBaseTest : public GDBRemoteTest {
public:
void SetUp() override {
- ASSERT_THAT_ERROR(GDBRemoteCommunication::ConnectLocally(client, server),
- llvm::Succeeded());
+ llvm::Expected<Socket::Pair> pair = Socket::CreatePair();
+ ASSERT_THAT_EXPECTED(pair, llvm::Succeeded());
+ client.SetConnection(
+ std::make_unique<ConnectionFileDescriptor>(std::move(pair->first)));
+ server.SetConnection(
+ std::make_unique<ConnectionFileDescriptor>(std::move(pair->second)));
ASSERT_EQ(TestClient::eBroadcastBitRunPacketSent,
listener_sp->StartListeningForEvents(
&client, TestClient::eBroadcastBitRunPacketSent));
diff --git a/lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp b/lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp
index 96f377ec7dfac..f940229985887 100644
--- a/lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp
+++ b/lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp
@@ -8,6 +8,7 @@
#include "Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h"
#include "GDBRemoteTestUtils.h"
#include "lldb/Core/ModuleSpec.h"
+#include "lldb/Host/ConnectionFileDescriptor.h"
#include "lldb/Host/XML.h"
#include "lldb/Target/MemoryRegionInfo.h"
#include "lldb/Utility/DataBuffer.h"
@@ -63,8 +64,12 @@ std::string one_register_hex = "41424344";
class GDBRemoteCommunicationClientTest : public GDBRemoteTest {
public:
void SetUp() override {
- ASSERT_THAT_ERROR(GDBRemoteCommunication::ConnectLocally(client, server),
- llvm::Succeeded());
+ llvm::Expected<Socket::Pair> pair = Socket::CreatePair();
+ ASSERT_THAT_EXPECTED(pair, llvm::Succeeded());
+ client.SetConnection(
+ std::make_unique<ConnectionFileDescriptor>(std::move(pair->first)));
+ server.SetConnection(
+ std::make_unique<ConnectionFileDescriptor>(std::move(pair->second)));
}
protected:
diff --git a/lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationTest.cpp b/lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationTest.cpp
index 3da1f0a718fc5..e96d587b10e25 100644
--- a/lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationTest.cpp
+++ b/lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationTest.cpp
@@ -5,7 +5,9 @@
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//===----------------------------------------------------------------------===//
+
#include "GDBRemoteTestUtils.h"
+#include "lldb/Host/ConnectionFileDescriptor.h"
#include "llvm/Testing/Support/Error.h"
using namespace lldb_private::process_gdb_remote;
@@ -28,8 +30,12 @@ class TestClient : public GDBRemoteCommunication {
class GDBRemoteCommunicationTest : public GDBRemoteTest {
public:
void SetUp() override {
- ASSERT_THAT_ERROR(GDBRemoteCommunication::ConnectLocally(client, server),
- llvm::Succeeded());
+ llvm::Expected<Socket::Pair> pair = Socket::CreatePair();
+ ASSERT_THAT_EXPECTED(pair, llvm::Succeeded());
+ client.SetConnection(
+ std::make_unique<ConnectionFileDescriptor>(std::move(pair->first)));
+ server.SetConnection(
+ std::make_unique<ConnectionFileDescriptor>(std::move(pair->second)));
}
protected:
diff --git a/lldb/unittests/tools/lldb-server/tests/TestClient.cpp b/lldb/unittests/tools/lldb-server/tests/TestClient.cpp
index 48e089bc0fcff..f3510cad22e7f 100644
--- a/lldb/unittests/tools/lldb-server/tests/TestClient.cpp
+++ b/lldb/unittests/tools/lldb-server/tests/TestClient.cpp
@@ -125,7 +125,8 @@ TestClient::launchCustom(StringRef Log, bool disable_stdio,
listen_socket.Accept(2 * GetDefaultTimeout(), accept_socket)
.takeError())
return E;
- auto Conn = std::make_unique<ConnectionFileDescriptor>(accept_socket);
+ auto Conn = std::make_unique<ConnectionFileDescriptor>(
+ std::unique_ptr<Socket>(accept_socket));
auto Client = std::unique_ptr<TestClient>(new TestClient(std::move(Conn)));
if (Error E = Client->initializeConnection())
|
JDevlieghere
approved these changes
Jun 23, 2025
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/11/builds/17943 Here is the relevant piece of the build log for the reference |
labath
added a commit
that referenced
this pull request
Jun 24, 2025
Mid-flight collision with #145293.
|
Hi @labath, |
labath
added a commit
that referenced
this pull request
Jun 24, 2025
|
Thanks. I just noticed the windows failure. 3e98d2b ought to take care of it. |
DrSergei
pushed a commit
to DrSergei/llvm-project
that referenced
this pull request
Jun 24, 2025
Originally added for reproducers, it is now only used for test code. While we could make it a test helper, I think that after llvm#145015 it is simple enough to not be needed. Also squeeze in a change to make ConnectionFileDescriptor accept a unique_ptr<Socket>.
DrSergei
pushed a commit
to DrSergei/llvm-project
that referenced
this pull request
Jun 24, 2025
Mid-flight collision with llvm#145293.
DrSergei
pushed a commit
to DrSergei/llvm-project
that referenced
this pull request
Jun 24, 2025
anthonyhatran
pushed a commit
to anthonyhatran/llvm-project
that referenced
this pull request
Jun 26, 2025
Originally added for reproducers, it is now only used for test code. While we could make it a test helper, I think that after llvm#145015 it is simple enough to not be needed. Also squeeze in a change to make ConnectionFileDescriptor accept a unique_ptr<Socket>.
anthonyhatran
pushed a commit
to anthonyhatran/llvm-project
that referenced
this pull request
Jun 26, 2025
Mid-flight collision with llvm#145293.
anthonyhatran
pushed a commit
to anthonyhatran/llvm-project
that referenced
this pull request
Jun 26, 2025
rlavaee
pushed a commit
to rlavaee/llvm-project
that referenced
this pull request
Jul 1, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Originally added for reproducers, it is now only used for test code.
While we could make it a test helper, I think that after #145015 it is simple enough to not be needed.
Also squeeze in a change to make ConnectionFileDescriptor accept a unique_ptr.