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

[Pavel Labath via lldb-commits]

Author: Pavel Labath
Date: 2021-03-04T14:46:02+01:00
New Revision: bf3ac994c4d526b74044a977176e8e07d83f2049

URL: https://github.com/llvm/llvm-project/commit/bf3ac994c4d526b74044a977176e8e07d83f2049
DIFF: https://github.com/llvm/llvm-project/commit/bf3ac994c4d526b74044a977176e8e07d83f2049.diff

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

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. It also adds a test which checks that the timeout value is
being honored.

Differential Revision: https://reviews.llvm.org/D97769

Added: 
    

Modified: 
    lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
    lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
    lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
    lldb/test/API/commands/platform/connect/TestPlatformConnect.py
    lldb/test/API/functionalities/gdb_remote_client/TestPlatformClient.py

Removed: 
    


################################################################################
diff  --git a/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp b/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
index 6a4275d249f6..8227199f52b6 100644
--- a/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
+++ b/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
@@ -30,6 +30,7 @@
 #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;
@@ -202,7 +203,10 @@ Status PlatformRemoteGDBServer::GetFileWithUUID(const FileSpec &platform_file,
 /// 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.
 ///

diff  --git a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
index a2e41e738dbd..4a8eb0777886 100644
--- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -213,6 +213,10 @@ ProcessGDBRemote::CreateInstance(lldb::TargetSP target_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)

diff  --git a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
index 0921bf17c4e4..9c74369fd6c5 100644
--- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
+++ b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
@@ -68,6 +68,8 @@ class ProcessGDBRemote : public Process,
 
   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;

diff  --git a/lldb/test/API/commands/platform/connect/TestPlatformConnect.py b/lldb/test/API/commands/platform/connect/TestPlatformConnect.py
index cc9726ad303a..5c6ee5743208 100644
--- a/lldb/test/API/commands/platform/connect/TestPlatformConnect.py
+++ b/lldb/test/API/commands/platform/connect/TestPlatformConnect.py
@@ -13,7 +13,6 @@ class TestPlatformProcessConnect(TestBase):
     @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()

diff  --git a/lldb/test/API/functionalities/gdb_remote_client/TestPlatformClient.py b/lldb/test/API/functionalities/gdb_remote_client/TestPlatformClient.py
index e05089ecc18e..30262fce7480 100644
--- a/lldb/test/API/functionalities/gdb_remote_client/TestPlatformClient.py
+++ b/lldb/test/API/functionalities/gdb_remote_client/TestPlatformClient.py
@@ -1,6 +1,7 @@
 import lldb
 import binascii
 import os
+import time
 from lldbsuite.test.lldbtest import *
 from lldbsuite.test.decorators import *
 from gdbclientutils import *
@@ -64,3 +65,41 @@ def qsProcessInfo(self):
                         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()