Skip to content

[lldb][mcp] Fix unix domain socket protocol server addresses #146603

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jul 3, 2025

Conversation

aperez
Copy link
Member

@aperez aperez commented Jul 1, 2025

When starting an MCP protocol server that uses unix sockets as the transport, a local '[0.0.0.0]:0' file is used instead of the supplied socket path, e.g:

(lldb) protocol-server start MCP accept:///tmp/some/path.sock
MCP server started with connection listeners: unix-connect://[0.0.0.0]:0
(lldb) shell ls '[*'
[0.0.0.0]:0

This change makes it so that the URI path is used if the socket protocol is ProtocolUnixDomain:

(lldb) protocol-server start MCP accept:///tmp/some/path.sock
MCP server started with connection listeners: unix-connect:///tmp/some/path.sock

@aperez aperez requested a review from JDevlieghere as a code owner July 1, 2025 21:27
@llvmbot llvmbot added the lldb label Jul 1, 2025
@llvmbot
Copy link
Member

llvmbot commented Jul 1, 2025

@llvm/pr-subscribers-lldb

Author: Alexandre Perez (aperez)

Changes

When starting an MCP protocol server that uses unix sockets as the transport, a local '[0.0.0.0]:0' file is used instead of the supplied socket path, e.g:

(lldb) protocol-server start MCP accept:///tmp/some/path.sock
MCP server started with connection listeners: unix-connect://[0.0.0.0]:0
(lldb) shell ls '[*'
[0.0.0.0]:0

This change makes it so that the URI path is used if the socket protocol is ProtocolUnixDomain:

(lldb) protocol-server start MCP accept:///tmp/some/path.sock
MCP server started with connection listeners: unix-connect:///tmp/some/path.sock

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

1 Files Affected:

  • (modified) lldb/source/Commands/CommandObjectProtocolServer.cpp (+6-3)
diff --git a/lldb/source/Commands/CommandObjectProtocolServer.cpp b/lldb/source/Commands/CommandObjectProtocolServer.cpp
index 55bd42ed1a533..4cafd2266e235 100644
--- a/lldb/source/Commands/CommandObjectProtocolServer.cpp
+++ b/lldb/source/Commands/CommandObjectProtocolServer.cpp
@@ -75,9 +75,12 @@ class CommandObjectProtocolServerStart : public CommandObjectParsed {
 
     ProtocolServer::Connection connection;
     connection.protocol = protocol_and_mode->first;
-    connection.name =
-        formatv("[{0}]:{1}", uri->hostname.empty() ? "0.0.0.0" : uri->hostname,
-                uri->port.value_or(0));
+    if (connection.protocol == Socket::SocketProtocol::ProtocolUnixDomain)
+      connection.name = uri->path;
+    else
+      connection.name = formatv(
+          "[{0}]:{1}", uri->hostname.empty() ? "0.0.0.0" : uri->hostname,
+          uri->port.value_or(0));
 
     if (llvm::Error error = server->Start(connection)) {
       result.AppendErrorWithFormatv("{0}", llvm::fmt_consume(std::move(error)));

@kusmour
Copy link
Contributor

kusmour commented Jul 2, 2025

Shall we add a test for this?

@aperez
Copy link
Member Author

aperez commented Jul 2, 2025

@kusmour @JDevlieghere
I added a test for this. Since my change is in command parsing I created a test in lldb/test/API/commands/protocol/ rather than extend lldb/unittests/Protocol/ProtocolMCPServerTest.cpp. LMK what you think.

Copy link
Contributor

@ashgti ashgti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One comment, otherwise LGTM

Test if we can start an MCP protocol-server accepting unix sockets
"""

temp_directory = tempfile.TemporaryDirectory()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One note, for unix sockets there is often a character limit of around 104~108 chars depending on the platform.

This will often fetch a directory using TEMP_DIR, which may be overridden by say a CI system to something pretty long.

We may either want to skip the test if the tempdir is to long or try to find a shorter directory like "/tmp", if it exists.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I've pushed a change to make sure we skip if the socket path is over that limit

@aperez aperez merged commit a068ed2 into llvm:main Jul 3, 2025
9 checks passed
@aperez aperez deleted the mcp_unix_socket branch July 3, 2025 00:03
@llvm-ci
Copy link
Collaborator

llvm-ci commented Jul 3, 2025

LLVM Buildbot has detected a new failure on builder lldb-remote-linux-win running on as-builder-10 while building lldb at step 17 "test-check-lldb-api".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/197/builds/6596

Here is the relevant piece of the build log for the reference
Step 17 (test-check-lldb-api) failure: Test just built components: check-lldb-api completed (failure)
******************** TEST 'lldb-api :: commands/protocol/TestMCPUnixSocket.py' FAILED ********************
Script:
--
C:/Python312/python.exe C:/buildbot/as-builder-10/lldb-x-aarch64/llvm-project/lldb\test\API\dotest.py -u CXXFLAGS -u CFLAGS --env LLVM_LIBS_DIR=C:/buildbot/as-builder-10/lldb-x-aarch64/build/./lib --env LLVM_INCLUDE_DIR=C:/buildbot/as-builder-10/lldb-x-aarch64/build/include --env LLVM_TOOLS_DIR=C:/buildbot/as-builder-10/lldb-x-aarch64/build/./bin --arch aarch64 --build-dir C:/buildbot/as-builder-10/lldb-x-aarch64/build/lldb-test-build.noindex --lldb-module-cache-dir C:/buildbot/as-builder-10/lldb-x-aarch64/build/lldb-test-build.noindex/module-cache-lldb\lldb-api --clang-module-cache-dir C:/buildbot/as-builder-10/lldb-x-aarch64/build/lldb-test-build.noindex/module-cache-clang\lldb-api --executable C:/buildbot/as-builder-10/lldb-x-aarch64/build/./bin/lldb.exe --compiler C:/buildbot/as-builder-10/lldb-x-aarch64/build/./bin/clang.exe --dsymutil C:/buildbot/as-builder-10/lldb-x-aarch64/build/./bin/dsymutil.exe --make C:/ninja/make.exe --llvm-tools-dir C:/buildbot/as-builder-10/lldb-x-aarch64/build/./bin --lldb-obj-root C:/buildbot/as-builder-10/lldb-x-aarch64/build/tools/lldb --lldb-libs-dir C:/buildbot/as-builder-10/lldb-x-aarch64/build/./lib --cmake-build-type Release --platform-url connect://jetson-agx-0086.lab.llvm.org:1234 --platform-working-dir /home/ubuntu/lldb-tests --sysroot c:/buildbot/fs/jetson-agx-ubuntu --env ARCH_CFLAGS=-mcpu=cortex-a78 --platform-name remote-linux --skip-category=lldb-server C:\buildbot\as-builder-10\lldb-x-aarch64\llvm-project\lldb\test\API\commands\protocol -p TestMCPUnixSocket.py
--
Exit Code: 1

Command Output (stdout):
--
lldb version 21.0.0git (https://github.com/llvm/llvm-project.git revision a068ed288af16df0d6189fd830216bbfa6257016)
  clang revision a068ed288af16df0d6189fd830216bbfa6257016
  llvm revision a068ed288af16df0d6189fd830216bbfa6257016
Setting up remote platform 'remote-linux'

Connecting to remote platform 'remote-linux' at 'connect://jetson-agx-0086.lab.llvm.org:1234'...

Connected.

Setting remote platform working directory to '/home/ubuntu/lldb-tests'...

Skipping the following test categories: ['lldb-server', 'dsym', 'gmodules', 'debugserver', 'objc', 'lldb-dap']


--
Command Output (stderr):
--
FAIL: LLDB (C:\buildbot\as-builder-10\lldb-x-aarch64\build\bin\clang.exe-aarch64) :: test_unix_socket (TestMCPUnixSocket.MCPUnixSocketCommandTestCase.test_unix_socket)

======================================================================

FAIL: test_unix_socket (TestMCPUnixSocket.MCPUnixSocketCommandTestCase.test_unix_socket)

   Test if we can start an MCP protocol-server accepting unix sockets

----------------------------------------------------------------------

Traceback (most recent call last):

  File "C:\buildbot\as-builder-10\lldb-x-aarch64\llvm-project\lldb\packages\Python\lldbsuite\test\decorators.py", line 444, in wrapper

    return func(self, *args, **kwargs)

           ^^^^^^^^^^^^^^^^^^^^^^^^^^^

  File "C:\buildbot\as-builder-10\lldb-x-aarch64\llvm-project\lldb\test\API\commands\protocol\TestMCPUnixSocket.py", line 29, in test_unix_socket

    self.expect(

  File "C:\buildbot\as-builder-10\lldb-x-aarch64\llvm-project\lldb\packages\Python\lldbsuite\test\lldbtest.py", line 2406, in expect

...

aperez added a commit that referenced this pull request Jul 3, 2025
It looks like #146603 broke the
[lldb-remote-linux-win](https://lab.llvm.org/buildbot/#/builders/197)
build bot because `MCPUnixSocketCommandTestCase` is trying to start a
protocol-server via unix domain sockets on windows.
This change makes it so the test is skipped if it is remote.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants