[Lldb-commits] [lldb] [lldb/platform-gdb] Do not assume a persistent connection (PR #131736)

[Igor Kudrin via lldb-commits]

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 <ikudrin at 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());
+}