Minor fix to connect-url to support unix-connect sockets on localhost

[satyajanga]

Summary:

when the unix-socket connections on localhost are used to for platform connect i.e.
platform connect unix-connect:///path/to/socket.sock
then PlatformRemoteGDBServer.m_platform_hostname is empty.

Based on the current logic, for the process attach, when the connection param returned by platform server as qLaunchGDBServer is this socket_name:/path/to/processgdbserver.sock
then the subsequent connect url for the process url looks like this unix-connect://[]/path/to/processgdbserver.sock and the connection fail.

This change is only adding the braces when the hostname is not empty.

Test Plan:

Added unittest and existing tests pass.

satyajanga@devvm21837:toolchain $ LLDB_COMMAND_TRACE=YES ./bin/llvm-lit --verbose  ~/llvm-sand/external/llvm-project/lldb/test/API/commands/platform
-- Testing: 9 tests, 9 workers --
UNSUPPORTED: lldb-api :: commands/platform/sdk/TestPlatformSDK.py (1 of 9)
PASS: lldb-api :: commands/platform/file/read/TestPlatformFileRead.py (2 of 9)
PASS: lldb-api :: commands/platform/file/close/TestPlatformFileClose.py (3 of 9)
PASS: lldb-api :: commands/platform/basic/TestPlatformPython.py (4 of 9)
PASS: lldb-api :: commands/platform/basic/TestPlatformCommand.py (5 of 9)
PASS: lldb-api :: commands/platform/process/launch/TestPlatformProcessLaunch.py (6 of 9)
PASS: lldb-api :: commands/platform/connect/TestPlatformConnect.py (7 of 9)
PASS: lldb-api :: commands/platform/launchgdbserver/TestPlatformLaunchGDBServer.py (8 of 9)
PASS: lldb-api :: commands/platform/process/list/TestProcessList.py (9 of 9)

Testing Time: 13.24s

Total Discovered Tests: 9
  Unsupported: 1 (11.11%)
  Passed     : 8 (88.89%)
satyajanga@devvm21837:toolchain $ 

Reviewers:

@clayborg
@Jlalond

Subscribers:

Tasks:

Tags:

[llvmbot]

@llvm/pr-subscribers-lldb

Author: None (satyajanga)

Changes

Summary:
when the unix-socket connections on localhost are used to for platform connect i.e.
platform connect unix-connect:///path/to/socket.sock
then PlatformRemoteGDBServer.m_platform_hostname is empty.

Based on the current logic, for the process attach, when the connection param returned by platform server as qLaunchGDBServer is this socket_name:/path/to/processgdbserver.sock
then the subsequent connect url for the process url looks like this unix-connect://[]/path/to/processgdbserver.sock and the connection fail.

This change is only adding the braces when the hostname is not empty.

Test Plan:
Added unittest and existing tests pass.

Reviewers:

Subscribers:

Tasks:

Tags:

---

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

3 Files Affected:

• (modified) lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp (+5-1)
• (modified) lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp (+2-2)
• (modified) lldb/test/API/commands/platform/connect/TestPlatformConnect.py (+47)

diff --git a/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp b/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
index 26ca6ed128972..7eebe8220e016 100644
--- a/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
+++ b/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
@@ -800,7 +800,11 @@ std::string PlatformRemoteGDBServer::MakeUrl(const char *scheme,
                                              const char *hostname,
                                              uint16_t port, const char *path) {
   StreamString result;
-  result.Printf("%s://[%s]", scheme, hostname);
+  if (strlen(hostname) > 0)
+    result.Printf("%s://[%s]", scheme, hostname);
+  else
+    result.Printf("%s://", scheme);
+
   if (port != 0)
     result.Printf(":%u", port);
   if (path)
diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
index adbf06b9a19a0..f7fed3b45f8ad 100644
--- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
@@ -538,8 +538,8 @@ GDBRemoteCommunicationClient::SendThreadSpecificPacketAndWaitForResponse(
     if (Log *log = GetLog(GDBRLog::Process | GDBRLog::Packets))
       LLDB_LOGF(log,
                 "GDBRemoteCommunicationClient::%s: Didn't get sequence mutex "
-                "for %s packet.",
-                __FUNCTION__, payload.GetData());
+                "for %s packet for thread %lu.",
+                __FUNCTION__, payload.GetData(), tid);
     return PacketResult::ErrorNoSequenceLock;
   }
 
diff --git a/lldb/test/API/commands/platform/connect/TestPlatformConnect.py b/lldb/test/API/commands/platform/connect/TestPlatformConnect.py
index fc6c2ee98df44..68fa22afaed32 100644
--- a/lldb/test/API/commands/platform/connect/TestPlatformConnect.py
+++ b/lldb/test/API/commands/platform/connect/TestPlatformConnect.py
@@ -57,3 +57,50 @@ def test_platform_process_connect(self):
         self.assertEqual(frame.GetFunction().GetName(), "main")
         self.assertEqual(frame.FindVariable("argc").GetValueAsSigned(), 2)
         process.Continue()
+
+
+    @skipIfRemote
+    @expectedFailureAll(hostoslist=["windows"], triple=".*-android")
+    @skipIfDarwin  # lldb-server not found correctly
+    @expectedFailureAll(oslist=["windows"])  # process modules not loaded
+    # lldb-server platform times out waiting for the gdbserver port number to be
+    # written to the pipe, yet it seems the gdbserver already has written it.
+    @expectedFailureAll(
+        archs=["aarch64"],
+        oslist=["freebsd"],
+        bugnumber="https://github.com/llvm/llvm-project/issues/84327",
+    )
+    @add_test_categories(["lldb-server"])
+    def test_platform_process_connect_with_unix_connect(self):
+        self.build()
+        import time
+        timestamp = int(time.time())
+        listen_url = "/tmp/listen_url_%s" % timestamp
+        port_file = "/tmp/port_file_%s" % timestamp
+        commandline_args = [
+            "platform",
+            "--listen",
+            listen_url,
+            "--socket-file",
+            port_file,
+            "--",
+            self.getBuildArtifact("a.out"),
+            "foo",
+        ]
+        self.spawnSubprocess(lldbgdbserverutils.get_lldb_server_exe(), commandline_args)
+
+        socket_file = lldbutil.wait_for_file_on_target(self, port_file)
+        new_platform = lldb.SBPlatform("remote-" + self.getPlatform())
+        self.dbg.SetSelectedPlatform(new_platform)
+        connect_url = "unix-connect://%s" % socket_file
+        self.runCmd("platform connect %s" % connect_url)
+
+        lldbutil.run_break_set_by_symbol(self, "main")
+        process = self.process()
+
+        process.Continue()
+
+        frame = self.frame()
+        self.assertEqual(frame.GetFunction().GetName(), "main")
+        self.assertEqual(frame.FindVariable("argc").GetValueAsSigned(), 2)
+        process.Continue()

[github-actions]

✅ With the latest revision this PR passed the Python code formatter.

[labath]

Looks good. Just a couple of tweaks.

[labath]

It's shorter and more in line with the rest of the code.

Suggested change

	if (strlen(hostname) > 0)
	result.Printf("%s://[%s]", scheme, hostname);
	else
	result.Printf("%s://", scheme);
	result.Printf("%s://", scheme);
	if (strlen(hostname) > 0)
	result.Printf("[%s]", hostname);

[satyajanga]

updated the logic based on the suggestion.

[labath]

Putting files into /tmp like this is fairly rude. For the port file, you can just use the build directory (like the other test). Named pipes are somewhat tricky as they have a fairly short limit on the length. I might use SBHostOS.GetLLDBPath(ePathTypeLLDBTempSystemDir) for that as it's likely to be shorter than the build dir.

[satyajanga]

named pipes max length is 108 chars, so i selected to use /tmp dir. thank for the pointer to do it correctly.

struct sockaddr_un
  {
    __SOCKADDR_COMMON (sun_);
    char sun_path[108];		/* Path name.  */
  };