[lldb][mcp] Fix unix domain socket protocol server addresses #146603
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-lldb Author: Alexandre Perez (aperez) ChangesWhen starting an MCP protocol server that uses unix sockets as the transport, a local This change makes it so that the URI path is used if the socket protocol is Full diff: https://github.com/llvm/llvm-project/pull/146603.diff 1 Files Affected:
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
approved these changes
Jul 2, 2025
|
Shall we add a test for this? |
JDevlieghere
approved these changes
Jul 2, 2025
|
@kusmour @JDevlieghere |
ashgti
approved these changes
Jul 2, 2025
| Test if we can start an MCP protocol-server accepting unix sockets | ||
| """ | ||
|
|
||
| temp_directory = tempfile.TemporaryDirectory() |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Thanks! I've pushed a change to make sure we skip if the socket path is over that limit
|
LLVM Buildbot has detected a new failure on builder 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 |
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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:This change makes it so that the URI path is used if the socket protocol is
ProtocolUnixDomain: