🍒 Backport MCP changes before JSONTransport change #11598 #11599
+711
−540
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
This PR moves the MCP protocol code into its own library (`lldbProtocolMCP`) so the code can be shared between the `ProtocolServerMCP` plugin in LLDB as well as `lldb-mcp`. The goal is to do the same thing for DAP (which, for now, would be used exclusively from `lldb-dap`). To make it clear that it's neither part of the `lldb` nor the `lldb_private` namespace, I created a new `lldb_protocol` namespace. Depending on how much code would be reused by lldb-dap, we may move more code into the protocol library. (cherry picked from commit ed294c2)
This moves all the generic MCP code into the new ProtocolMCP library so it can be shared between the MCP implementation in LLDB (built on the private API) and lldb-mcp (built on the public API). This PR doesn't include the actual MCP server. The reason is that the transport mechanism will be different between the LLDB implementation (using sockets) and the lldb-mcp implementation (using stdio). The goal s to abstract away that difference by making the server use JSONTransport (again) but I'm saving that for a separate PR. (cherry picked from commit dbaa82b)
…#152396) This is a continuation of llvm#152188, which started splitting up the MCP implementation into a generic implementation in Protocol/MCP that will be shared between LLDB and lldb-mcp. For now I kept all the networking code in the MCP server plugin. Once the changes to JSONTransport land, we might be able to move more of it into the Protocol library. (cherry picked from commit 44aedac)
* This adjusts the `Request`/`Response` types to have an `id` that is either a string or a number. * Merges 'Error' into 'Response' to have a single response type that represents both errors and results. * Adjusts the `Error.data` field to by any JSON value. * Adds `operator==` support to the base protocol types and simplifies the tests. (cherry picked from commit 350f6ab)
Caused by llvm#153297. This is likely not Windows on Arm specific but the other Windows bots don't have this problem. json::parse tries to construct llvm::Expected<Message> by moving another instance of Message into it. This would normally use this constructor: ``` /// Create an Expected<T> success value from the given OtherT value, which /// must be convertible to T. template <typename OtherT> Expected(OtherT &&Val, std::enable_if_t<std::is_convertible_v<OtherT, T>> * = nullptr) <...> ``` Note that llvm::Expected does not have a T&& constructor. Presumably the authors thought the converting one would be used. Except that in our build, using clang-cl 19.1.7, Visual Studio 2022, MSVC STL 202208, somehow is_convertible_v is false for Message. If you add a static_assert to check that this is the case, it suddenly becomes convertible. As best I can understand, this is because evaluation of the properties of Message are delayed. Delayed so much that by the time the constructor is called, it's still false. So we can "fix" this by asserting that it is convertible some time before it is checked by the constructor. I'm not sure if that's a compiler problem or the way MSVC STL's variant is written. I couldn't reproduce this behaviour in smaller examples or on Linux systems. This is the least invasive fix and only touches the new lldb code, so I'm going with it. Including the whole error here because it's a strange one and maybe later someone who has a clue about this can fix it in a better way. ``` C:\Users\tcwg\llvm-worker\lldb-aarch64-windows\build>ninja [5/15] Building CXX object tools\lldb\source\Pro...CP\CMakeFiles\lldbProtocolMCP.dir\Server.cpp.obj FAILED: tools/lldb/source/Protocol/MCP/CMakeFiles/lldbProtocolMCP.dir/Server.cpp.obj C:\Users\tcwg\scoop\shims\ccache.exe C:\Users\tcwg\scoop\apps\llvm-arm64\current\bin\clang-cl.exe /nologo -TP -DGTEST_HAS_RTTI=0 -DUNICODE -D_CRT_NONSTDC_NO_DEPRECATE -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_SECURE_NO_DEPRECATE -D_CRT_SECURE_NO_WARNINGS -D_ENABLE_EXTENDED_ALIGNED_STORAGE -D_HAS_EXCEPTIONS=0 -D_SCL_SECURE_NO_DEPRECATE -D_SCL_SECURE_NO_WARNINGS -D_UNICODE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -IC:\Users\tcwg\llvm-worker\lldb-aarch64-windows\build\tools\lldb\source\Protocol\MCP -IC:\Users\tcwg\llvm-worker\lldb-aarch64-windows\llvm-project\lldb\source\Protocol\MCP -IC:\Users\tcwg\llvm-worker\lldb-aarch64-windows\llvm-project\lldb\include -IC:\Users\tcwg\llvm-worker\lldb-aarch64-windows\build\tools\lldb\include -IC:\Users\tcwg\llvm-worker\lldb-aarch64-windows\build\include -IC:\Users\tcwg\llvm-worker\lldb-aarch64-windows\llvm-project\llvm\include -IC:\Users\tcwg\scoop\apps\python\current\include -IC:\Users\tcwg\llvm-worker\lldb-aarch64-windows\llvm-project\llvm\..\clang\include -IC:\Users\tcwg\llvm-worker\lldb-aarch64-windows\build\tools\lldb\..\clang\include -IC:\Users\tcwg\llvm-worker\lldb-aarch64-windows\llvm-project\lldb\source -IC:\Users\tcwg\llvm-worker\lldb-aarch64-windows\build\tools\lldb\source /DWIN32 /D_WINDOWS /Zc:inline /Zc:__cplusplus /Oi /Brepro /bigobj /permissive- -Werror=unguarded-availability-new /W4 -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -Wmisleading-indentation -Wctad-maybe-unsupported /Gw -Wno-vla-extension /O2 /Ob2 /DNDEBUG -std:c++17 -MD -wd4018 -wd4068 -wd4150 -wd4201 -wd4251 -wd4521 -wd4530 -wd4589 /EHs-c- /GR- /showIncludes /Fotools\lldb\source\Protocol\MCP\CMakeFiles\lldbProtocolMCP.dir\Server.cpp.obj /Fdtools\lldb\source\Protocol\MCP\CMakeFiles\lldbProtocolMCP.dir\lldbProtocolMCP.pdb -c -- C:\Users\tcwg\llvm-worker\lldb-aarch64-windows\llvm-project\lldb\source\Protocol\MCP\Server.cpp In file included from C:\Users\tcwg\llvm-worker\lldb-aarch64-windows\llvm-project\lldb\source\Protocol\MCP\Server.cpp:9: In file included from C:\Users\tcwg\llvm-worker\lldb-aarch64-windows\llvm-project\lldb\include\lldb/Protocol/MCP/Server.h:12: In file included from C:\Users\tcwg\llvm-worker\lldb-aarch64-windows\llvm-project\lldb\include\lldb/Protocol/MCP/Protocol.h:17: C:\Users\tcwg\llvm-worker\lldb-aarch64-windows\llvm-project\llvm\include\llvm/Support/JSON.h(938,12): error: no viable conversion from returned value of type 'remove_reference_t<variant<Request, Response, Notification> &>' (aka 'std::variant<lldb_protocol::mcp::Request, lldb_protocol::mcp::Response, lldb_protocol::mcp::Notification>') to function return type 'Expected<variant<Request, Response, Notification>>' 938 | return std::move(Result); | ^~~~~~~~~~~~~~~~~ C:\Users\tcwg\llvm-worker\lldb-aarch64-windows\llvm-project\lldb\source\Protocol\MCP\Server.cpp(57,30): note: in instantiation of function template specialization 'llvm::json::parse<std::variant<lldb_protocol::mcp::Request, lldb_protocol::mcp::Response, lldb_protocol::mcp::Notification>>' requested here 57 | auto message = llvm::json::parse<Message>(/*JSON=*/data); | ^ C:\Users\tcwg\llvm-worker\lldb-aarch64-windows\llvm-project\llvm\include\llvm/Support/Error.h(485,40): note: candidate constructor (the implicit copy constructor) not viable: no known conversion from 'remove_reference_t<variant<Request, Response, Notification> &>' (aka 'std::variant<lldb_protocol::mcp::Request, lldb_protocol::mcp::Response, lldb_protocol::mcp::Notification>') to 'const Expected<variant<Request, Response, Notification>> &' for 1st argument 485 | template <class T> class [[nodiscard]] Expected { | ^~~~~~~~ C:\Users\tcwg\llvm-worker\lldb-aarch64-windows\llvm-project\llvm\include\llvm/Support/Error.h(507,3): note: candidate constructor not viable: no known conversion from 'remove_reference_t<variant<Request, Response, Notification> &>' (aka 'std::variant<lldb_protocol::mcp::Request, lldb_protocol::mcp::Response, lldb_protocol::mcp::Notification>') to 'Error &&' for 1st argument 507 | Expected(Error &&Err) | ^ ~~~~~~~~~~~ C:\Users\tcwg\llvm-worker\lldb-aarch64-windows\llvm-project\llvm\include\llvm/Support/Error.h(521,3): note: candidate constructor not viable: no known conversion from 'remove_reference_t<variant<Request, Response, Notification> &>' (aka 'std::variant<lldb_protocol::mcp::Request, lldb_protocol::mcp::Response, lldb_protocol::mcp::Notification>') to 'ErrorSuccess' for 1st argument 521 | Expected(ErrorSuccess) = delete; | ^ ~~~~~~~~~~~~ C:\Users\tcwg\llvm-worker\lldb-aarch64-windows\llvm-project\llvm\include\llvm/Support/Error.h(539,3): note: candidate constructor not viable: no known conversion from 'remove_reference_t<variant<Request, Response, Notification> &>' (aka 'std::variant<lldb_protocol::mcp::Request, lldb_protocol::mcp::Response, lldb_protocol::mcp::Notification>') to 'Expected<variant<Request, Response, Notification>> &&' for 1st argument 539 | Expected(Expected &&Other) { moveConstruct(std::move(Other)); } | ^ ~~~~~~~~~~~~~~~~ C:\Users\tcwg\llvm-worker\lldb-aarch64-windows\llvm-project\llvm\include\llvm/Support/Error.h(526,3): note: candidate template ignored: requirement 'std::is_convertible_v<std::variant<lldb_protocol::mcp::Request, lldb_protocol::mcp::Response, lldb_protocol::mcp::Notification>, std::variant<lldb_protocol::mcp::Request, lldb_protocol::mcp::Response, lldb_protocol::mcp::Notification>>' was not satisfied [with OtherT = remove_reference_t<variant<Request, Response, Notification> &>] 526 | Expected(OtherT &&Val, | ^ C:\Users\tcwg\llvm-worker\lldb-aarch64-windows\llvm-project\llvm\include\llvm/Support/Error.h(544,3): note: candidate template ignored: could not match 'Expected' against 'variant' 544 | Expected(Expected<OtherT> &&Other, | ^ C:\Users\tcwg\llvm-worker\lldb-aarch64-windows\llvm-project\llvm\include\llvm/Support/Error.h(552,12): note: explicit constructor is not a candidate 552 | explicit Expected( | ^ 1 error generated. [8/15] Building CXX object tools\lldb\source\Plu...nProtocolServerMCP.dir\ProtocolServerMCP.cpp.obj ninja: build stopped: subcommand failed. ``` (cherry picked from commit dece902)
|
@swift-ci test |
|
Related: #11598 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
This PR backports the MCP changes leading up to the big JSONTransport refactor.