aadsm created this revision.
aadsm added a reviewer: labath.
Herald added subscribers: lldb-commits, jfb, mgorny.
Herald added a project: LLDB.
This is a general fix for the ConnectionFileDescriptor class but my main
motivation was to make lldb-server working with IPv6.
The connect URI can use square brackets ([]) to wrap the interface part of the
URI (e.g.: <scheme>://[<interface>]:<port>). For IPv6 addresses this is a must
since its ip can include colons and it will overlap with the port colon
otherwise. The URIParser class parses the square brackets correctly but the
ConnectionFileDescriptor doesn't generate them for IPv6 addresses making it
impossible to connect to the gdb server when using this protocol.
How to reproduce the issue:
$ lldb-server p --server --listen [::1]:8080
...
$ lldb
(lldb) platform select remote-macosx
(lldb) platform connect connect://[::1]:8080
(lldb) platform process -p <pid>
error: unable to launch a GDB server on 'computer'
The server was actually launched we were just not able to connect to it. With
this fix lldb will correctly connect.
I fixed this by wrapping the ip portion with []. It was a simple fix but the
tests was what took more time to do.
@labath I need your help figuring out a good way to set these tests up. I
wanted to reuse the CreateConnectedSockets part of the SocketTests. Initially I
thought about creating a few functions in a test socket utility file, but then
I realized the tests need to initialize the Socket plugin for these functions
to work. In the end I created a new class (SocketBasedTest) so I can ensure
this plugin is correctly initialized and teared down. However, I'm not a fan of
subclassing as a means of code shared (as I prefer composition or delegation).
What are your thoughts on this?
Another question I have (and this is more a n00b question as I don't have prior
c++ experience), the CreateConnectedSockets functions that exist use unique
pointers, and after reading the documentation about this I decided to use
.release() so I could safely pass them to the ConnectionFileDescriptor in the
test I created for it. Should I have changed the unique pointers to shared
pointers instead?
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D61833
Files:
lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
lldb/unittests/Host/CMakeLists.txt
lldb/unittests/Host/ConnectionFileDescriptorTest.cpp
lldb/unittests/Host/SocketBasedTest.cpp
lldb/unittests/Host/SocketBasedTest.h
lldb/unittests/Host/SocketTest.cpp
Index: lldb/unittests/Host/SocketTest.cpp
===================================================================
--- lldb/unittests/Host/SocketTest.cpp
+++ lldb/unittests/Host/SocketTest.cpp
@@ -6,82 +6,18 @@
//
//===----------------------------------------------------------------------===//
-#include <cstdio>
-#include <functional>
-#include <thread>
-
+#include "SocketBasedTest.h"
#include "gtest/gtest.h"
-#include "lldb/Host/Config.h"
-#include "lldb/Host/Socket.h"
-#include "lldb/Host/common/TCPSocket.h"
-#include "lldb/Host/common/UDPSocket.h"
-#include "llvm/Support/FileSystem.h"
-#include "llvm/Support/Path.h"
-#include "llvm/Testing/Support/Error.h"
-
-#ifndef LLDB_DISABLE_POSIX
-#include "lldb/Host/posix/DomainSocket.h"
-#endif
-
using namespace lldb_private;
-class SocketTest : public testing::Test {
+class SocketTest : public SocketBasedTest {
public:
void SetUp() override {
ASSERT_THAT_ERROR(Socket::Initialize(), llvm::Succeeded());
}
void TearDown() override { Socket::Terminate(); }
-
-protected:
- static void AcceptThread(Socket *listen_socket,
- bool child_processes_inherit, Socket **accept_socket,
- Status *error) {
- *error = listen_socket->Accept(*accept_socket);
- }
-
- template <typename SocketType>
- void CreateConnectedSockets(
- llvm::StringRef listen_remote_address,
- const std::function<std::string(const SocketType &)> &get_connect_addr,
- std::unique_ptr<SocketType> *a_up, std::unique_ptr<SocketType> *b_up) {
- bool child_processes_inherit = false;
- Status error;
- std::unique_ptr<SocketType> listen_socket_up(
- new SocketType(true, child_processes_inherit));
- EXPECT_FALSE(error.Fail());
- error = listen_socket_up->Listen(listen_remote_address, 5);
- EXPECT_FALSE(error.Fail());
- EXPECT_TRUE(listen_socket_up->IsValid());
-
- Status accept_error;
- Socket *accept_socket;
- std::thread accept_thread(AcceptThread, listen_socket_up.get(),
- child_processes_inherit, &accept_socket,
- &accept_error);
-
- std::string connect_remote_address = get_connect_addr(*listen_socket_up);
- std::unique_ptr<SocketType> connect_socket_up(
- new SocketType(true, child_processes_inherit));
- EXPECT_FALSE(error.Fail());
- error = connect_socket_up->Connect(connect_remote_address);
- EXPECT_FALSE(error.Fail());
- EXPECT_TRUE(connect_socket_up->IsValid());
-
- a_up->swap(connect_socket_up);
- EXPECT_TRUE(error.Success());
- EXPECT_NE(nullptr, a_up->get());
- EXPECT_TRUE((*a_up)->IsValid());
-
- accept_thread.join();
- b_up->reset(static_cast<SocketType *>(accept_socket));
- EXPECT_TRUE(accept_error.Success());
- EXPECT_NE(nullptr, b_up->get());
- EXPECT_TRUE((*b_up)->IsValid());
-
- listen_socket_up.reset();
- }
};
TEST_F(SocketTest, DecodeHostAndPort) {
@@ -159,38 +95,23 @@
std::unique_ptr<DomainSocket> socket_a_up;
std::unique_ptr<DomainSocket> socket_b_up;
- CreateConnectedSockets<DomainSocket>(
- Path, [=](const DomainSocket &) { return Path.str().str(); },
- &socket_a_up, &socket_b_up);
+ CreateDomainConnectedSockets(Path, &socket_a_up, &socket_b_up);
}
#endif
TEST_F(SocketTest, TCPListen0ConnectAccept) {
std::unique_ptr<TCPSocket> socket_a_up;
std::unique_ptr<TCPSocket> socket_b_up;
- CreateConnectedSockets<TCPSocket>(
- "127.0.0.1:0",
- [=](const TCPSocket &s) {
- char connect_remote_address[64];
- snprintf(connect_remote_address, sizeof(connect_remote_address),
- "127.0.0.1:%u", s.GetLocalPortNumber());
- return std::string(connect_remote_address);
- },
- &socket_a_up, &socket_b_up);
+ CreateTCPConnectedSockets("127.0.0.1", &socket_a_up, &socket_b_up);
}
TEST_F(SocketTest, TCPGetAddress) {
std::unique_ptr<TCPSocket> socket_a_up;
std::unique_ptr<TCPSocket> socket_b_up;
- CreateConnectedSockets<TCPSocket>(
- "127.0.0.1:0",
- [=](const TCPSocket &s) {
- char connect_remote_address[64];
- snprintf(connect_remote_address, sizeof(connect_remote_address),
- "127.0.0.1:%u", s.GetLocalPortNumber());
- return std::string(connect_remote_address);
- },
- &socket_a_up, &socket_b_up);
+ if (!CreateTCPConnectedSockets("127.0.0.1", &socket_a_up, &socket_b_up)) {
+ // Skip this test if the host doesn't support this interface
+ return;
+ }
EXPECT_EQ(socket_a_up->GetLocalPortNumber(),
socket_b_up->GetRemotePortNumber());
Index: lldb/unittests/Host/SocketBasedTest.h
===================================================================
--- /dev/null
+++ lldb/unittests/Host/SocketBasedTest.h
@@ -0,0 +1,56 @@
+//===--------------------- SocketBasedTest.h --------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLDB_UNITTESTS_HOST_SOCKETTESTUTILITIES_H
+#define LLDB_UNITTESTS_HOST_SOCKETTESTUTILITIES_H
+
+#include <cstdio>
+#include <functional>
+#include <thread>
+
+#include "lldb/Host/Config.h"
+#include "lldb/Host/Socket.h"
+#include "lldb/Host/common/TCPSocket.h"
+#include "lldb/Host/common/UDPSocket.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Testing/Support/Error.h"
+
+#ifndef LLDB_DISABLE_POSIX
+#include "lldb/Host/posix/DomainSocket.h"
+#endif
+
+using namespace lldb_private;
+
+class SocketBasedTest : public testing::Test {
+public:
+ void SetUp() override {
+ ASSERT_THAT_ERROR(Socket::Initialize(), llvm::Succeeded());
+ }
+
+ void TearDown() override { Socket::Terminate(); }
+
+ static void AcceptThread(Socket *listen_socket, bool child_processes_inherit,
+ Socket **accept_socket, Status *error);
+
+ template <typename SocketType>
+ void CreateConnectedSockets(
+ llvm::StringRef listen_remote_address,
+ const std::function<std::string(const SocketType &)> &get_connect_addr,
+ std::unique_ptr<SocketType> *a_up, std::unique_ptr<SocketType> *b_up);
+ bool CreateTCPConnectedSockets(std::string listen_remote_ip,
+ std::unique_ptr<TCPSocket> *a_up,
+ std::unique_ptr<TCPSocket> *b_up);
+#ifndef LLDB_DISABLE_POSIX
+ void CreateDomainConnectedSockets(llvm::StringRef path,
+ std::unique_ptr<DomainSocket> *a_up,
+ std::unique_ptr<DomainSocket> *b_up);
+#endif
+};
+
+#endif
\ No newline at end of file
Index: lldb/unittests/Host/SocketBasedTest.cpp
===================================================================
--- /dev/null
+++ lldb/unittests/Host/SocketBasedTest.cpp
@@ -0,0 +1,92 @@
+//===--------------------- SocketBasedTest.cpp ------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "SocketBasedTest.h"
+#include "lldb/Utility/StreamString.h"
+
+void SocketBasedTest::AcceptThread(Socket *listen_socket,
+ bool child_processes_inherit,
+ Socket **accept_socket, Status *error) {
+ *error = listen_socket->Accept(*accept_socket);
+}
+
+template <typename SocketType>
+void SocketBasedTest::CreateConnectedSockets(
+ llvm::StringRef listen_remote_address,
+ const std::function<std::string(const SocketType &)> &get_connect_addr,
+ std::unique_ptr<SocketType> *a_up, std::unique_ptr<SocketType> *b_up) {
+ bool child_processes_inherit = false;
+ Status error;
+ std::unique_ptr<SocketType> listen_socket_up(
+ new SocketType(true, child_processes_inherit));
+ EXPECT_FALSE(error.Fail());
+ error = listen_socket_up->Listen(listen_remote_address, 5);
+ EXPECT_FALSE(error.Fail());
+ EXPECT_TRUE(listen_socket_up->IsValid());
+
+ Status accept_error;
+ Socket *accept_socket;
+ std::thread accept_thread(AcceptThread, listen_socket_up.get(),
+ child_processes_inherit, &accept_socket,
+ &accept_error);
+
+ std::string connect_remote_address = get_connect_addr(*listen_socket_up);
+ std::unique_ptr<SocketType> connect_socket_up(
+ new SocketType(true, child_processes_inherit));
+ EXPECT_FALSE(error.Fail());
+ error = connect_socket_up->Connect(connect_remote_address);
+ EXPECT_FALSE(error.Fail());
+ EXPECT_TRUE(connect_socket_up->IsValid());
+
+ a_up->swap(connect_socket_up);
+ EXPECT_TRUE(error.Success());
+ EXPECT_NE(nullptr, a_up->get());
+ EXPECT_TRUE((*a_up)->IsValid());
+
+ accept_thread.join();
+ b_up->reset(static_cast<SocketType *>(accept_socket));
+ EXPECT_TRUE(accept_error.Success());
+ EXPECT_NE(nullptr, b_up->get());
+ EXPECT_TRUE((*b_up)->IsValid());
+
+ listen_socket_up.reset();
+}
+
+bool SocketBasedTest::CreateTCPConnectedSockets(
+ std::string listen_remote_ip, std::unique_ptr<TCPSocket> *socket_a_up,
+ std::unique_ptr<TCPSocket> *socket_b_up) {
+ // Return false if the host doesn't support this interface.
+ auto addresses = lldb_private::SocketAddress::GetAddressInfo(
+ listen_remote_ip.c_str(), NULL, AF_UNSPEC, SOCK_STREAM, IPPROTO_TCP);
+ if (addresses.size() == 0) {
+ return false;
+ }
+
+ StreamString strm;
+ strm.Printf("[%s]:0", listen_remote_ip.c_str());
+ CreateConnectedSockets<TCPSocket>(
+ strm.GetString(),
+ [=](const TCPSocket &s) {
+ char connect_remote_address[64];
+ snprintf(connect_remote_address, sizeof(connect_remote_address),
+ "[%s]:%u", listen_remote_ip.c_str(), s.GetLocalPortNumber());
+ return std::string(connect_remote_address);
+ },
+ socket_a_up, socket_b_up);
+ return true;
+}
+
+#ifndef LLDB_DISABLE_POSIX
+void SocketBasedTest::CreateDomainConnectedSockets(
+ llvm::StringRef path, std::unique_ptr<DomainSocket> *socket_a_up,
+ std::unique_ptr<DomainSocket> *socket_b_up) {
+ return CreateConnectedSockets<DomainSocket>(
+ path, [=](const DomainSocket &) { return path.str(); }, socket_a_up,
+ socket_b_up);
+}
+#endif
Index: lldb/unittests/Host/ConnectionFileDescriptorTest.cpp
===================================================================
--- /dev/null
+++ lldb/unittests/Host/ConnectionFileDescriptorTest.cpp
@@ -0,0 +1,51 @@
+//===-- ConnectionFileDescriptorTest.cpp ------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "SocketBasedTest.h"
+#include "gtest/gtest.h"
+
+#include "lldb/Utility/UriParser.h"
+#include "lldb/Host/posix/ConnectionFileDescriptorPosix.h"
+
+using namespace lldb_private;
+
+class ConnectionFileDescriptorTest : public SocketBasedTest {
+public:
+ void SetUp() override {
+ ASSERT_THAT_ERROR(Socket::Initialize(), llvm::Succeeded());
+ }
+
+ void TearDown() override { Socket::Terminate(); }
+
+ void TestGetURI(std::string ip) {
+ std::unique_ptr<TCPSocket> socket_a_up;
+ std::unique_ptr<TCPSocket> socket_b_up;
+ if (!CreateTCPConnectedSockets(ip, &socket_a_up, &socket_b_up)) {
+ // Skip this test if the host doesn't support this interface.
+ return;
+ }
+ auto socket = socket_a_up.release();
+ ConnectionFileDescriptor connection_file_descriptor(socket);
+
+ llvm::StringRef scheme;
+ llvm::StringRef hostname;
+ int port;
+ llvm::StringRef path;
+ EXPECT_TRUE(UriParser::Parse(connection_file_descriptor.GetURI(), scheme, hostname, port, path));
+ EXPECT_STREQ(ip.c_str(), hostname.str().c_str());
+ EXPECT_EQ(socket->GetRemotePortNumber(), port);
+ }
+};
+
+TEST_F(ConnectionFileDescriptorTest, TCPGetURIv4) {
+ TestGetURI("127.0.0.1");
+}
+
+TEST_F(ConnectionFileDescriptorTest, TCPGetURIv6) {
+ TestGetURI("::1");
+}
\ No newline at end of file
Index: lldb/unittests/Host/CMakeLists.txt
===================================================================
--- lldb/unittests/Host/CMakeLists.txt
+++ lldb/unittests/Host/CMakeLists.txt
@@ -9,6 +9,8 @@
SocketAddressTest.cpp
SocketTest.cpp
TaskPoolTest.cpp
+ SocketBasedTest.cpp
+ ConnectionFileDescriptorTest.cpp
)
if (CMAKE_SYSTEM_NAME MATCHES "Linux|Android")
Index: lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
===================================================================
--- lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
+++ lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
@@ -764,7 +764,7 @@
m_write_sp.reset(socket);
m_read_sp = m_write_sp;
StreamString strm;
- strm.Printf("connect://%s:%u", tcp_socket->GetRemoteIPAddress().c_str(),
+ strm.Printf("connect://[%s]:%u", tcp_socket->GetRemoteIPAddress().c_str(),
tcp_socket->GetRemotePortNumber());
m_uri = strm.GetString();
}
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
