DavidSpickett updated this revision to Diff 307356. DavidSpickett added a comment. Herald added a subscriber: mgorny.
- Move PortMap into a class we can unittest - Use llvm::Expected for GetNextAvailablePort Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91634/new/ https://reviews.llvm.org/D91634 Files: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.h lldb/tools/lldb-server/lldb-platform.cpp lldb/unittests/tools/lldb-server/tests/CMakeLists.txt lldb/unittests/tools/lldb-server/tests/PortMapTest.cpp
Index: lldb/unittests/tools/lldb-server/tests/PortMapTest.cpp =================================================================== --- /dev/null +++ lldb/unittests/tools/lldb-server/tests/PortMapTest.cpp @@ -0,0 +1,117 @@ +//===-- PortMapTest.cpp ---------------------------------------------------===// +// +// 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 "llvm/Testing/Support/Error.h" +#include "gmock/gmock.h" +#include "gtest/gtest.h" + +#include "Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.h" + +using namespace lldb_private::process_gdb_remote; + +TEST(PortMapTest, Constructors) { + // Default construct to empty map + GDBRemoteCommunicationServerPlatform::PortMap p1; + ASSERT_TRUE(p1.empty()); + + // Empty means no restrictions, return 0 and and bind to get a port + llvm::Expected<uint16_t> available_port = p1.GetNextAvailablePort(); + ASSERT_THAT_EXPECTED(available_port, llvm::Succeeded()); + ASSERT_EQ(0, *available_port); + + // Adding any port makes it not empty + p1.AllowPort(1); + ASSERT_FALSE(p1.empty()); + + // So we will return the added port this time + available_port = p1.GetNextAvailablePort(); + ASSERT_THAT_EXPECTED(available_port, llvm::Succeeded()); + ASSERT_EQ(1, *available_port); + + // Construct from a range of ports + GDBRemoteCommunicationServerPlatform::PortMap p2(1, 4); + ASSERT_FALSE(p2.empty()); + + // Use up all the ports + for (uint16_t expected = 1; expected < 4; ++expected) { + available_port = p2.GetNextAvailablePort(); + ASSERT_THAT_EXPECTED(available_port, llvm::Succeeded()); + EXPECT_EQ(expected, *available_port); + p2.AssociatePortWithProcess(*available_port, 1); + } + + // Now we fail since we're not an empty port map but all ports are used + available_port = p2.GetNextAvailablePort(); + ASSERT_THAT_EXPECTED(available_port, llvm::Failed()); +} + +TEST(PortMapTest, FreePort) { + GDBRemoteCommunicationServerPlatform::PortMap p(1, 4); + // Use up all the ports + for (uint16_t port = 1; port < 4; ++port) { + p.AssociatePortWithProcess(port, 1); + } + + llvm::Expected<uint16_t> available_port = p.GetNextAvailablePort(); + ASSERT_THAT_EXPECTED(available_port, llvm::Failed()); + + // Can't free a port that isn't in the map + ASSERT_FALSE(p.FreePort(0)); + ASSERT_FALSE(p.FreePort(4)); + + // After freeing a port it becomes available + ASSERT_TRUE(p.FreePort(2)); + available_port = p.GetNextAvailablePort(); + ASSERT_THAT_EXPECTED(available_port, llvm::Succeeded()); + ASSERT_EQ(2, *available_port); +} + +TEST(PortMapTest, FreePortForProcess) { + GDBRemoteCommunicationServerPlatform::PortMap p; + p.AllowPort(1); + p.AllowPort(2); + ASSERT_TRUE(p.AssociatePortWithProcess(1, 11)); + ASSERT_TRUE(p.AssociatePortWithProcess(2, 22)); + + // All ports have been used + llvm::Expected<uint16_t> available_port = p.GetNextAvailablePort(); + ASSERT_THAT_EXPECTED(available_port, llvm::Failed()); + + // Can't free a port for a process that doesn't have any + ASSERT_FALSE(p.FreePortForProcess(33)); + + ASSERT_TRUE(p.FreePortForProcess(22)); + available_port = p.GetNextAvailablePort(); + ASSERT_THAT_EXPECTED(available_port, llvm::Succeeded()); + ASSERT_EQ(2, *available_port); + + // proces 22 no longer has a port + ASSERT_FALSE(p.FreePortForProcess(22)); +} + +TEST(PortMapTest, AllowPort) { + GDBRemoteCommunicationServerPlatform::PortMap p; + + // Allow port 1 and tie it to process 11 + p.AllowPort(1); + ASSERT_TRUE(p.AssociatePortWithProcess(1, 11)); + + // Allowing it a second time shouldn't change existing mapping + p.AllowPort(1); + llvm::Expected<uint16_t> available_port = p.GetNextAvailablePort(); + ASSERT_THAT_EXPECTED(available_port, llvm::Failed()); + + // A new port is marked as free when allowed + p.AllowPort(2); + available_port = p.GetNextAvailablePort(); + ASSERT_THAT_EXPECTED(available_port, llvm::Succeeded()); + ASSERT_EQ(2, *available_port); + + // 11 should still be tied to port 1 + ASSERT_TRUE(p.FreePortForProcess(11)); +} Index: lldb/unittests/tools/lldb-server/tests/CMakeLists.txt =================================================================== --- lldb/unittests/tools/lldb-server/tests/CMakeLists.txt +++ lldb/unittests/tools/lldb-server/tests/CMakeLists.txt @@ -4,6 +4,7 @@ TestBase.cpp TestClient.cpp ThreadIdsInJstopinfoTest.cpp + PortMapTest.cpp LINK_LIBS lldbHost Index: lldb/tools/lldb-server/lldb-platform.cpp =================================================================== --- lldb/tools/lldb-server/lldb-platform.cpp +++ lldb/tools/lldb-server/lldb-platform.cpp @@ -231,7 +231,7 @@ break; } if (ch == 'P') - gdbserver_portmap[portnum] = LLDB_INVALID_PROCESS_ID; + gdbserver_portmap.AllowPort(static_cast<uint16_t>(portnum)); else if (ch == 'm') min_gdbserver_port = portnum; else @@ -250,8 +250,9 @@ // Make a port map for a port range that was specified. if (min_gdbserver_port && min_gdbserver_port < max_gdbserver_port) { - for (uint16_t port = min_gdbserver_port; port < max_gdbserver_port; ++port) - gdbserver_portmap[port] = LLDB_INVALID_PROCESS_ID; + gdbserver_portmap = GDBRemoteCommunicationServerPlatform::PortMap( + static_cast<uint16_t>(min_gdbserver_port), + static_cast<uint16_t>(max_gdbserver_port)); } else if (min_gdbserver_port || max_gdbserver_port) { fprintf(stderr, "error: --min-gdbserver-port (%u) is not lower than " "--max-gdbserver-port (%u)\n", Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.h =================================================================== --- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.h +++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.h @@ -16,13 +16,39 @@ #include "GDBRemoteCommunicationServerCommon.h" #include "lldb/Host/Socket.h" +#include "llvm/Support/Error.h" + namespace lldb_private { namespace process_gdb_remote { class GDBRemoteCommunicationServerPlatform : public GDBRemoteCommunicationServerCommon { public: - typedef std::map<uint16_t, lldb::pid_t> PortMap; + class PortMap { + public: + PortMap() = default; + PortMap(uint16_t min_port, uint16_t max_port); + // Add a port to the map. Does not change anything if it is already in the + // map. + void AllowPort(uint16_t port); + + // If we are using a port map where we can only use certain ports, + // get the next available port. + // + // If we are using a port map and we are out of ports, return an error. + // + // If we aren't using a port map, return 0 to indicate we should bind to + // port 0 and then figure out which port we used. + llvm::Expected<uint16_t> GetNextAvailablePort(); + + bool AssociatePortWithProcess(uint16_t port, lldb::pid_t pid); + bool FreePort(uint16_t port); + bool FreePortForProcess(lldb::pid_t pid); + bool empty() const; + + private: + std::map<uint16_t, lldb::pid_t> m_port_map; + }; GDBRemoteCommunicationServerPlatform( const Socket::SocketProtocol socket_protocol, const char *socket_scheme); @@ -35,21 +61,6 @@ // a port chosen by the OS. void SetPortMap(PortMap &&port_map); - // If we are using a port map where we can only use certain ports, - // get the next available port. - // - // If we are using a port map and we are out of ports, return UINT16_MAX - // - // If we aren't using a port map, return 0 to indicate we should bind to - // port 0 and then figure out which port we used. - uint16_t GetNextAvailablePort(); - - bool AssociatePortWithProcess(uint16_t port, lldb::pid_t pid); - - bool FreePort(uint16_t port); - - bool FreePortForProcess(lldb::pid_t pid); - void SetPortOffset(uint16_t port_offset); void SetInferiorArguments(const lldb_private::Args &args); Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp =================================================================== --- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp +++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp @@ -42,6 +42,69 @@ using namespace lldb_private::process_gdb_remote; using namespace lldb_private; +GDBRemoteCommunicationServerPlatform::PortMap::PortMap(uint16_t min_port, + uint16_t max_port) { + for (; min_port < max_port; ++min_port) + m_port_map[min_port] = LLDB_INVALID_PROCESS_ID; +} + +void GDBRemoteCommunicationServerPlatform::PortMap::AllowPort(uint16_t port) { + // Do not modify existing mappings + m_port_map.insert({port, LLDB_INVALID_PROCESS_ID}); +} + +llvm::Expected<uint16_t> +GDBRemoteCommunicationServerPlatform::PortMap::GetNextAvailablePort() { + if (m_port_map.empty()) + return 0; // Bind to port zero and get a port, we didn't have any + // limitations + + for (auto &pair : m_port_map) { + if (pair.second == LLDB_INVALID_PROCESS_ID) { + pair.second = ~(lldb::pid_t)LLDB_INVALID_PROCESS_ID; + return pair.first; + } + } + return llvm::createStringError(llvm::inconvertibleErrorCode(), + "No free port found in port map"); +} + +bool GDBRemoteCommunicationServerPlatform::PortMap::AssociatePortWithProcess( + uint16_t port, lldb::pid_t pid) { + std::map<uint16_t, lldb::pid_t>::iterator pos = m_port_map.find(port); + if (pos != m_port_map.end()) { + pos->second = pid; + return true; + } + return false; +} + +bool GDBRemoteCommunicationServerPlatform::PortMap::FreePort(uint16_t port) { + std::map<uint16_t, lldb::pid_t>::iterator pos = m_port_map.find(port); + if (pos != m_port_map.end()) { + pos->second = LLDB_INVALID_PROCESS_ID; + return true; + } + return false; +} + +bool GDBRemoteCommunicationServerPlatform::PortMap::FreePortForProcess( + lldb::pid_t pid) { + if (!m_port_map.empty()) { + for (auto &pair : m_port_map) { + if (pair.second == pid) { + pair.second = LLDB_INVALID_PROCESS_ID; + return true; + } + } + } + return false; +} + +bool GDBRemoteCommunicationServerPlatform::PortMap::empty() const { + return m_port_map.empty(); +} + // GDBRemoteCommunicationServerPlatform constructor GDBRemoteCommunicationServerPlatform::GDBRemoteCommunicationServerPlatform( const Socket::SocketProtocol socket_protocol, const char *socket_scheme) @@ -95,8 +158,13 @@ Status GDBRemoteCommunicationServerPlatform::LaunchGDBServer( const lldb_private::Args &args, std::string hostname, lldb::pid_t &pid, uint16_t &port, std::string &socket_name) { - if (port == UINT16_MAX) - port = GetNextAvailablePort(); + if (port == UINT16_MAX) { + llvm::Expected<uint16_t> available_port = m_port_map.GetNextAvailablePort(); + if (available_port) + port = *available_port; + else + return Status(available_port.takeError()); + } // Spawn a new thread to accept the port that gets bound after binding to // port 0 (zero). @@ -152,10 +220,10 @@ std::lock_guard<std::recursive_mutex> guard(m_spawned_pids_mutex); m_spawned_pids.insert(pid); if (port > 0) - AssociatePortWithProcess(port, pid); + m_port_map.AssociatePortWithProcess(port, pid); } else { if (port > 0) - FreePort(port); + m_port_map.FreePort(port); } return error; } @@ -453,7 +521,7 @@ bool GDBRemoteCommunicationServerPlatform::DebugserverProcessReaped( lldb::pid_t pid) { std::lock_guard<std::recursive_mutex> guard(m_spawned_pids_mutex); - FreePortForProcess(pid); + m_port_map.FreePortForProcess(pid); m_spawned_pids.erase(pid); return true; } @@ -499,51 +567,6 @@ m_port_map = port_map; } -uint16_t GDBRemoteCommunicationServerPlatform::GetNextAvailablePort() { - if (m_port_map.empty()) - return 0; // Bind to port zero and get a port, we didn't have any - // limitations - - for (auto &pair : m_port_map) { - if (pair.second == LLDB_INVALID_PROCESS_ID) { - pair.second = ~(lldb::pid_t)LLDB_INVALID_PROCESS_ID; - return pair.first; - } - } - return UINT16_MAX; -} - -bool GDBRemoteCommunicationServerPlatform::AssociatePortWithProcess( - uint16_t port, lldb::pid_t pid) { - PortMap::iterator pos = m_port_map.find(port); - if (pos != m_port_map.end()) { - pos->second = pid; - return true; - } - return false; -} - -bool GDBRemoteCommunicationServerPlatform::FreePort(uint16_t port) { - PortMap::iterator pos = m_port_map.find(port); - if (pos != m_port_map.end()) { - pos->second = LLDB_INVALID_PROCESS_ID; - return true; - } - return false; -} - -bool GDBRemoteCommunicationServerPlatform::FreePortForProcess(lldb::pid_t pid) { - if (!m_port_map.empty()) { - for (auto &pair : m_port_map) { - if (pair.second == pid) { - pair.second = LLDB_INVALID_PROCESS_ID; - return true; - } - } - } - return false; -} - const FileSpec &GDBRemoteCommunicationServerPlatform::GetDomainSocketDir() { static FileSpec g_domainsocket_dir; static llvm::once_flag g_once_flag; Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h =================================================================== --- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h +++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h @@ -27,7 +27,6 @@ class GDBRemoteCommunicationServer : public GDBRemoteCommunication { public: - using PortMap = std::map<uint16_t, lldb::pid_t>; using PacketHandler = std::function<PacketResult(StringExtractorGDBRemote &packet, Status &error, bool &interrupt, bool &quit)>;
_______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
