[lldb/platform-gdb] Do not assume a persistent connection

[igorkudrin]

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.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Igor Kudrin (igorkudrin)

Changes

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.

---

Full diff: https://github.com/llvm/llvm-project/pull/131736.diff

4 Files Affected:

• (modified) lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp (+1-5)
• (modified) lldb/unittests/Platform/CMakeLists.txt (+1)
• (added) lldb/unittests/Platform/gdb-server/CMakeLists.txt (+7)
• (added) lldb/unittests/Platform/gdb-server/PlatformRemoteGDBServerTest.cpp (+68)

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