https://github.com/igorkudrin created https://github.com/llvm/llvm-project/pull/131736
After https://reviews.llvm.org/D116539, when `m_gdb_client_up` in `PlatformRemoteGDBServer` is not null, the connection to a server is expected to exist. However, `PlatformRemoteGDBServer::DisconnectRemote()` is not the only way to close the connection; `GDBRemoteCommunication::WaitForPacketNoLock()` can disconnect if the server stops responding, and in this case `m_gdb_client_up` is not cleared. The patch removes this assumption and checks the connection status directly. >From 3de79119222a87709709934702d1de51ab805994 Mon Sep 17 00:00:00 2001 From: Igor Kudrin <ikud...@accesssoftek.com> Date: Mon, 17 Mar 2025 23:02:07 -0700 Subject: [PATCH] [lldb/platform-gdb] Avoid an assertion failure when the connection is severed After https://reviews.llvm.org/D116539, when `m_gdb_client_up` in `PlatformRemoteGDBServer` is not null, the connection to a server is expected to exist. However, `PlatformRemoteGDBServer::DisconnectRemote()` is not the only way to close the connection; `GDBRemoteCommunication::WaitForPacketNoLock()` can disconnect if the server stops responding, and in this case `m_gdb_client_up` is not cleared. The patch removes this assumption and checks the connection status directly. --- .../gdb-server/PlatformRemoteGDBServer.cpp | 6 +- lldb/unittests/Platform/CMakeLists.txt | 1 + .../Platform/gdb-server/CMakeLists.txt | 7 ++ .../PlatformRemoteGDBServerTest.cpp | 68 +++++++++++++++++++ 4 files changed, 77 insertions(+), 5 deletions(-) create mode 100644 lldb/unittests/Platform/gdb-server/CMakeLists.txt create mode 100644 lldb/unittests/Platform/gdb-server/PlatformRemoteGDBServerTest.cpp diff --git a/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp b/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp index 58d2ecd94836d..26ca6ed128972 100644 --- a/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp +++ b/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp @@ -206,11 +206,7 @@ bool PlatformRemoteGDBServer::SetRemoteWorkingDirectory( } bool PlatformRemoteGDBServer::IsConnected() const { - if (m_gdb_client_up) { - assert(m_gdb_client_up->IsConnected()); - return true; - } - return false; + return m_gdb_client_up && m_gdb_client_up->IsConnected(); } Status PlatformRemoteGDBServer::ConnectRemote(Args &args) { diff --git a/lldb/unittests/Platform/CMakeLists.txt b/lldb/unittests/Platform/CMakeLists.txt index 963975602d671..5c0ef5ca6ef22 100644 --- a/lldb/unittests/Platform/CMakeLists.txt +++ b/lldb/unittests/Platform/CMakeLists.txt @@ -15,3 +15,4 @@ add_lldb_unittest(LLDBPlatformTests ) add_subdirectory(Android) +add_subdirectory(gdb-server) diff --git a/lldb/unittests/Platform/gdb-server/CMakeLists.txt b/lldb/unittests/Platform/gdb-server/CMakeLists.txt new file mode 100644 index 0000000000000..41f94b06f6f2c --- /dev/null +++ b/lldb/unittests/Platform/gdb-server/CMakeLists.txt @@ -0,0 +1,7 @@ +add_lldb_unittest(PlatformGdbRemoteTests + PlatformRemoteGDBServerTest.cpp + + LINK_LIBS + lldbPluginPlatformGDB + LLVMTestingSupport + ) diff --git a/lldb/unittests/Platform/gdb-server/PlatformRemoteGDBServerTest.cpp b/lldb/unittests/Platform/gdb-server/PlatformRemoteGDBServerTest.cpp new file mode 100644 index 0000000000000..2ea4dac006cd8 --- /dev/null +++ b/lldb/unittests/Platform/gdb-server/PlatformRemoteGDBServerTest.cpp @@ -0,0 +1,68 @@ +//===-- PlatformRemoteGDBServerTest.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 "Plugins/Platform/gdb-server/PlatformRemoteGDBServer.h" +#include "lldb/Utility/Connection.h" +#include "gmock/gmock.h" + +using namespace lldb; +using namespace lldb_private; +using namespace lldb_private::platform_gdb_server; +using namespace lldb_private::process_gdb_remote; +using namespace testing; + +namespace { + +class PlatformRemoteGDBServerHack : public PlatformRemoteGDBServer { +public: + void + SetGDBClient(std::unique_ptr<GDBRemoteCommunicationClient> gdb_client_up) { + m_gdb_client_up = std::move(gdb_client_up); + } +}; + +class MockConnection : public lldb_private::Connection { +public: + MOCK_METHOD(lldb::ConnectionStatus, Connect, + (llvm::StringRef url, Status *error_ptr), (override)); + MOCK_METHOD(lldb::ConnectionStatus, Disconnect, (Status * error_ptr), + (override)); + MOCK_METHOD(bool, IsConnected, (), (const, override)); + MOCK_METHOD(size_t, Read, + (void *dst, size_t dst_len, const Timeout<std::micro> &timeout, + lldb::ConnectionStatus &status, Status *error_ptr), + (override)); + MOCK_METHOD(size_t, Write, + (const void *dst, size_t dst_len, lldb::ConnectionStatus &status, + Status *error_ptr), + (override)); + MOCK_METHOD(std::string, GetURI, (), (override)); + MOCK_METHOD(bool, InterruptRead, (), (override)); +}; + +} // namespace + +TEST(PlatformRemoteGDBServerTest, IsConnected) { + bool is_connected = true; + + auto connection = std::make_unique<NiceMock<MockConnection>>(); + ON_CALL(*connection, IsConnected()) + .WillByDefault(ReturnPointee(&is_connected)); + + auto client = std::make_unique<GDBRemoteCommunicationClient>(); + client->SetConnection(std::move(connection)); + + PlatformRemoteGDBServerHack server; + EXPECT_FALSE(server.IsConnected()); + + server.SetGDBClient(std::move(client)); + EXPECT_TRUE(server.IsConnected()); + + is_connected = false; + EXPECT_FALSE(server.IsConnected()); +} _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
