[lldb] Apply gdb-remote timeout to platform connections as well

[labath]

Summary

We have a plugin.process.gdb-remote.packet-timeout setting, which can be
used to control how long the lldb client is willing to wait before
declaring the server side dead. Our test suite makes use of this
feature, and sets the setting value fairly high, as the low default
value can cause flaky tests, particularly on slower bots.

After fixing TestPlatformConnect (one of the few tests exercising the
remote platform capabilities of lldb) in 4b284b9ca, it immediately
started being flaky on the arm bots. It turns out this is because the
packet-timeout setting is not being applied to platform connections.

This patch makes the platform connections also respect the value of this
setting. This does create a slightly weird dependency, where one plugin
depends on the setting value from another plugin (of a completely
different kind). However, it seemed to me that creating a separate
setting for platform connections would be even more confusing. I'm open
to other ideas, though.

I also add a test which checks that the timeout value is being honored.

[clayborg]

LGTM

lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp

 #include "lldb/Utility/FileSpec.h"
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/ProcessInfo.h"
 #include "lldb/Utility/Status.h"
 #include "lldb/Utility/StreamString.h"
 #include "lldb/Utility/UriParser.h"
 
 #include "Plugins/Process/Utility/GDBRemoteSignals.h"
+#include "Plugins/Process/gdb-remote/ProcessGDBRemote.h"
 
 using namespace lldb;
 using namespace lldb_private;
 using namespace lldb_private::platform_gdb_server;
 
 LLDB_PLUGIN_DEFINE_ADV(PlatformRemoteGDBServer, PlatformGDB)
 
 static bool g_initialized = false;
Status PlatformRemoteGDBServer::GetFileWithUUID(const FileSpec &platform_file,
   // Default to the local case
   local_file = platform_file;
   return Status();
 }
 
 /// Default Constructor
 PlatformRemoteGDBServer::PlatformRemoteGDBServer()
     : Platform(false), // This is a remote platform
-      m_gdb_client() {}
+      m_gdb_client() {
+  m_gdb_client.SetPacketTimeout(
+      process_gdb_remote::ProcessGDBRemote::GetPacketTimeout());
+}
 
 /// Destructor.
 ///
 /// The destructor is virtual since this class is designed to be
 /// inherited from by the plug-in instance.
 PlatformRemoteGDBServer::~PlatformRemoteGDBServer() {}
 
 bool PlatformRemoteGDBServer::GetSupportedArchitectureAtIndex(uint32_t idx,

lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h

public:
   static void DebuggerInitialize(Debugger &debugger);
 
   static void Terminate();
 
   static ConstString GetPluginNameStatic();
 
   static const char *GetPluginDescriptionStatic();
 
+  static std::chrono::seconds GetPacketTimeout();
+
   // Check if a given Process
   bool CanDebug(lldb::TargetSP target_sp,
                 bool plugin_specified_by_name) override;
 
   CommandObject *GetPluginCommandObject() override;
 
   // Creating a new process, or attaching to an existing one
   Status WillLaunch(Module *module) override;

lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

ProcessGDBRemote::CreateInstance(lldb::TargetSP target_sp,
                                  const FileSpec *crash_file_path,
                                  bool can_connect) {
   lldb::ProcessSP process_sp;
   if (crash_file_path == nullptr)
     process_sp = std::make_shared<ProcessGDBRemote>(target_sp, listener_sp);
   return process_sp;
 }
 
+std::chrono::seconds ProcessGDBRemote::GetPacketTimeout() {
+  return std::chrono::seconds(GetGlobalPluginProperties()->GetPacketTimeout());
+}
+
 bool ProcessGDBRemote::CanDebug(lldb::TargetSP target_sp,
                                 bool plugin_specified_by_name) {
   if (plugin_specified_by_name)
     return true;
 
   // For now we are just making sure the file exists for a given module
   Module *exe_module = target_sp->GetExecutableModulePointer();
   if (exe_module) {

lldb/test/API/commands/platform/connect/TestPlatformConnect.py

 import socket
 import gdbremote_testcase
 import lldbgdbserverutils
 from lldbsuite.test.decorators import *
 from lldbsuite.test.lldbtest import *
 from lldbsuite.test import lldbutil
 
 class TestPlatformProcessConnect(TestBase):
     mydir = TestBase.compute_mydir(__file__)
     NO_DEBUG_INFO_TESTCASE = True
 
     @skipIfRemote
     @expectedFailureAll(hostoslist=["windows"], triple='.*-android')
     @skipIfWindows # lldb-server does not terminate correctly
     @skipIfDarwin # lldb-server not found correctly
-    @skipIf(oslist=["linux"], archs=["arm", "aarch64"])  # Fails randomly
     @add_test_categories(["lldb-server"])
     def test_platform_process_connect(self):
         self.build()
 
         hostname = socket.getaddrinfo("localhost", 0, proto=socket.IPPROTO_TCP)[0][4][0]
         listen_url = "[%s]:0"%hostname
 
         port_file = self.getBuildArtifact("port")

lldb/test/API/functionalities/gdb_remote_client/TestPlatformClient.py

 import lldb
 import binascii
 import os
+import time
 from lldbsuite.test.lldbtest import *
 from lldbsuite.test.decorators import *
 from gdbclientutils import *
 
 def hexlify(string):
     return binascii.hexlify(string.encode()).decode()
 
 class TestPlatformClient(GDBRemoteTestBase):
def test_process_list_with_all_users(self):
                             "10     1      2          3          4          5                                         /system/bin/sh -c /data/local/tmp/lldb-server",
                             "11     2      3          4          5          6"])
             self.expect("platform process list -xv", substrs=["/system/bin/ls"], matching=False)
             self.expect("platform process list",
                         error=True,
                         substrs=["error: no processes were found on the \"remote-linux\" platform"])
         finally:
             self.dbg.GetSelectedPlatform().DisconnectRemote()
+
+    class TimeoutResponder(MockGDBServerResponder):
+        """A mock server, which takes a very long time to compute the working
+        directory."""
+        def __init__(self):
+            MockGDBServerResponder.__init__(self)
+
+        def qGetWorkingDir(self):
+            time.sleep(10)
+            return hexlify("/foo/bar")
+
+    def test_no_timeout(self):
+        """Test that we honor the timeout setting. With a large enough timeout,
+        we should get the CWD successfully."""
+
+        self.server.responder = TestPlatformClient.TimeoutResponder()
+        self.runCmd("settings set plugin.process.gdb-remote.packet-timeout 30")
+        plat = lldb.SBPlatform("remote-linux")
+        try:
+            self.assertSuccess(plat.ConnectRemote(lldb.SBPlatformConnectOptions("connect://"
+                + self.server.get_connect_address())))
+            self.assertEqual(plat.GetWorkingDirectory(), "/foo/bar")
+        finally:
+            plat.DisconnectRemote()
+
+    def test_timeout(self):
+        """Test that we honor the timeout setting. With a small timeout, CWD
+        retrieval should fail."""
+
+        self.server.responder = TestPlatformClient.TimeoutResponder()
+        self.runCmd("settings set plugin.process.gdb-remote.packet-timeout 3")
+        plat = lldb.SBPlatform("remote-linux")
+        try:
+            self.assertSuccess(plat.ConnectRemote(lldb.SBPlatformConnectOptions("connect://"
+                + self.server.get_connect_address())))
+            self.assertIsNone(plat.GetWorkingDirectory())
+        finally:
+            plat.DisconnectRemote()