This abstracts the base Transport handler to have a MessageHandler
component and allows us to generalize both JSON-RPC 2.0 for MCP (or an
LSP) and DAP format.
This should allow us to create clearly defined clients and servers for
protocols, both for testing and for RPC between the lldb instances and
an lldb-mcp multiplexer.
This basic model is inspiried by the clangd/Transport.h file and the
mlir/lsp-server-support/Transport.h that are both used for LSP servers
within the llvm project.
Additionally, this helps with testing by subclassing `Transport` to
allow us to simplify sending/receiving messages without needing to use a
toJSON/fromJSON and a pair of pipes, see `TestTransport` in
DAP/TestBase.h.
Reapply "[lldb] Update JSONTransport to use MainLoop for reading."
(#152155)
This reverts commit cd40281685f642ad879e33f3fda8d1faa136ebf4.
This also includes some updates to try to address the platforms with
failing tests.
I updated the JSONTransport and tests to use std::function instead of
llvm:unique_function. I think the tests were failing due to the
unique_function not being moved correctly in the loop on some platforms.
This updates JSONTransport to use a MainLoop for reading messages.
This also allows us to read in larger chunks than we did previously.
With the event driven reading operations we can read in chunks and store
the contents in an internal buffer. Separately we can parse the buffer
and split the contents up into messages.
Our previous version approach would read a byte at a time, which is less
efficient.
When there is a function that is inlined at the current program counter.
If you get the current `line_entry` using the program counter's address
it will point to the location of the inline function that may be in
another file. (this is in implicit step-in and should not happen what
step over is called).
Use the current frame to get the `line_entry`
The
[protocol](https://microsoft.github.io/debug-adapter-protocol//specification.html#Types_Source)
expects that `sourceReference` be less than `(2^31)-1`, but we currently
represent memory address as source reference, this can be truncated
either when sending through json or by the client. Instead, generate new
source references based on the memory address.
Make the `ResolveSource` function return an optional source.
This adds new types for setExceptionBreakpoints and adds support for
`supportsExceptionFilterOptions`, which allows exception breakpoints to
set a condition.
While testing this, I noticed that obj-c exception catch breakpoints may
not be working correctly in lldb-dap.
Moving `threads` request to structured types. Adding helper types for
this and moving helpers from JSONUtils to ProtocolUtils.
---------
Co-authored-by: Ebuka Ezike <yerimyah1@gmail.com>
Co-authored-by: Jonas Devlieghere <jonas@devlieghere.com>
Move the command plugins out of the DAP header and into their file. This
PR also renames the classes from "RequestHandler" to "Command". Although
they are implemented in terms of sending requests, they are not
"handlers".
Move the Variables struct out of DAP.h and into its own file to reduce
the complexity of the latter. This PR also makes the members that are
implementation details private and adds a handful of basic unit tests.
- Use in-class member initialization to simplify the constructor.
- Remove unimplemented SetConfigurationDone.
- Consistently use Doxygen-style comments.
This is more straight forward refactor of the startup sequence that
reverts parts of ba29e60f9a2222bd5e883579bb78db13fc5a7588. Unlike my
previous attempt, I ended up removing the pending request queue and not
including an `AsyncReqeustHandler` because I don't think we actually
need that at the moment.
The key is that during the startup flow there are 2 parallel operations
happening in the DAP that have different triggers.
* The `initialize` request is sent and once the response is received the
`launch` or `attach` is sent.
* When the `initialized` event is recieved the `setBreakpionts` and
other config requests are made followed by the `configurationDone`
event.
I moved the `initialized` event back to happen in the `PostRun` of the
`launch` or `attach` request handlers. This ensures that we have a valid
target by the time the configuration calls are made. I added also added
a few extra validations that to the `configurationeDone` handler to
ensure we're in an expected state.
I've also fixed up the tests to match the new flow. With the other
additional test fixes in 087a5d2ec7897cd99d3787820711fec76a8e1792 I
think we've narrowed down the main source of test instability that
motivated the startup sequence change.
This is a very simple case that currently only validates we can create a
DAP instance and send a message over the transport layer. More in-depth
tests will require additional helpers and possibly refactors of DAP to
make it more testable, however this is some ground work to have basic
support for unit tests.
---------
Co-authored-by: Jonas Devlieghere <jonas@devlieghere.com>
This PR changes how we treat the launch sequence in lldb-dap.
- Send the initialized event after we finish handling the initialize
request, rather than after we finish attaching or launching.
- Delay handling the launch and attach request until we have handled
the configurationDone request. The latter is now largely a NO-OP and
only exists to signal lldb-dap that it can handle the launch and
attach requests.
- Delay handling the initial threads requests until we have handled
the launch or attach request.
- Make all attaching and launching synchronous, including when we have
attach or launch commands. This removes the need to synchronize
between the request and event thread.
Background:
https://discourse.llvm.org/t/reliability-of-the-lldb-dap-tests/86125
This PR changes how we treat the launch sequence in lldb-dap.
- Send the initialized event after we finish handling the initialize
request, rather than after we finish attaching or launching.
- Delay handling the launch and attach request until we have handled
the configurationDone request. The latter is now largely a NO-OP and
only exists to signal lldb-dap that it can handle the launch and
attach requests.
- Delay handling the initial threads requests until we have handled
the launch or attach request.
- Make all attaching and launching synchronous, including when we have
attach or launch commands. This removes the need to synchronize
between the request and event thread.
Background:
https://discourse.llvm.org/t/reliability-of-the-lldb-dap-tests/86125
This converts a number of json::Value's into well defined types that are
used throughout lldb-dap and updates the 'launch' command to use the new
well defined types.
---------
Co-authored-by: Jonas Devlieghere <jonas@devlieghere.com>
This updates the 'next' request to use well structured types. While
working on this I also simplified the 'RequestHandler' implementation to
better handle void responses by allowing requests to return a
'llvm::Error' instead of an 'llvm::Expected<std::monostate>'. This makes
it easier to write and understand request handles that have simple ack
responses.
# Summary
This PR updates `SBProcess::GetNumThreads()` and
`SBProcess::GetThreadAtIndex()` to listen to the stop locker.
`SBProcess::GetNumThreads()` will return 0 if the process is running.
## Problem Description
Recently upon debugging a program with thousands of threads in VS Code,
lldb-dap would hang at a `threads` request sent right after receiving
the `configurationDone` response. Soon after it will end the debug
session with the following error
```
Process <pid> exited with status = -1 (0xffffffff) lost connection
```
This is because LLDB is still in the middle of resuming all the threads.
And requesting threads will end up interrupt the process on Linux. From
the gdb-remote log it ended up getting `lldb::StateType::eStateInvalid`
and just exit with status -1.
I don't think it's reasonable to allow getting threads from a running
process. There are a few approaches to fix this:
1) Send the stopped event to IDE after `configurationDone`. This aligns
with the CLI behavior.
2) However, the above approach will break the existing user facing
behavior. The alternative will be reject the `threads` request if the
process is not stopped.
3) Improve the run lock. This is a synchronize issue where process was
in the middle of resuming while lldb-dap attempts to interrupt it.
**This PR implements the option 3**
## HOWEVER
This fixed the "lost connection" issue below but new issue has surfaced.
From testing, and also from checking the [VSCode source
code](174af221c9/src/vs/workbench/contrib/debug/browser/debugSession.ts (L791)),
it expects having threadID to perform `pause`. So after attaching,
without any threads reported to the client, the user will not be able to
pause the attached process. `setBreakpoint` will still work and once we
make a stop at the bp (or any stop that will report threads, client can
perform pause again.
## NEXT
1) Made an attempt to return initial thread list so that VSCode can
pause (second commit in the PR)
2) Investigate why threads will trigger unwinding the second frame of a
thread, which leads to sending the interrupt
3) Decided if we want to support `stopOnEntry` for attaching, given
i. This is not an official specification
ii. If enable stopOnEntry, we need to fix attaching on Linux, to send
only one stopped event. Currently, all threads upon attaching will have
stop reason `SIGSTOP` and lldb-dap will send `stopped` event for each
one of them. Every `stopped` will trigger the client request for
threads.
iii. Alternatively, we can support auto continue correspond to `(lldb)
process attach --continue`. This require the ii above.
### Additionally
lldb-dap will not send a `continued` event after `configurationDone`
because it checks `dap.focus_tid == LLDB_INVALID_THREAD_ID` (so that we
don't send it for `launch` request). Notice `dap.focus_tid` will only
get assigned when handling stop or stepping.
According to DAP
> Please note: a debug adapter is not expected to send this event in
response to a request that implies that execution continues, e.g. launch
or continue.
It is only necessary to send a continued event if there was no previous
request that implied this.
So I guess we are not violating DAP if we don't send `continued` event.
But I'd like to get some sense about this.
## Test Plan
Used following program for testing:
https://gist.github.com/kusmour/1729d2e07b7b1063897db77de194e47d
**NOTE: Utilize stdin to get pid and attach AFTER hitting enter. Attach
should happen when all the threads start running.**
DAP messages before the change
<img width="1165" alt="image"
src="https://github.com/user-attachments/assets/a9ad85fb-81ce-419c-95e5-612639905c66"
/>
DAP message after the change - report zero threads after attaching
<img width="1165" alt="image"
src="https://github.com/user-attachments/assets/a1179e18-6844-437a-938c-0383702294cd"
/>
---------
Co-authored-by: Jonas Devlieghere <jonas@devlieghere.com>
Adding support for cancelling requests.
There are two forms of request cancellation.
* Preemptively cancelling a request that is in the queue.
* Actively cancelling the in progress request as a best effort attempt
using `SBDebugger.RequestInterrupt()`.
This moves all the common settings of the launch and attach operations
into the `lldb_dap::protocol::Configuration`. These common settings
can be in both `launch` and `attach` requests and allows us to isolate
the DAP configuration operations into a single common location.
This is split out from #133624.
Protect the various SetBreakpoint functions with the API mutex. This
fixes a race condition between the breakpoint being created and the DAP
label getting added. This was causing `TestDAP_breakpointEvents.py` to
be flaky.
Fixes#131242.
Convert Breakpoint & Watchpoints structs to classes to provide proper
access control. This is in preparation for adopting SBMutex to protect
the underlying SBBreakpoint and SBWatchpoint.
This adds new types and helpers to support the 'initialize' request with
the new typed RequestHandler. While working on this I found there were a
few cases where we incorrectly treated initialize arguments as
capabilities. The new `lldb_dap::protocol::InitializeRequestArguments`
and `lldb_dap::protocol::Capabilities` uncovered the inconsistencies.
---------
Co-authored-by: Jonas Devlieghere <jonas@devlieghere.com>
I noticed this while debugging some unit tests that the logs
occasionally would intersperse two log statements.
Previously, it was possible for a log statement to have two messages
interspersed since the timestamp and log statement were two different
writes to the log stream.
I created a Log helper to ensure we have a lock while attempting to write
to the logs.
This distributes the registration of request related capabilities to the
corresponding request handler. Global and unsupported capabilities are
registered at the DAP level.
This is a work in progress refactor to add explicit types instead of
generic 'llvm::json::Value' types to the DAP protocol.
This updates RequestHandler to have take the type of the arguments and
response body for serialization for requests.
The 'source' and 'disconnect' request is updated to show how the new
flow
works and includes serialization handling for optional arguments and
'void'
responses.
This is built on top of #130026
---------
Co-authored-by: Adrian Vogelsgesang <adrian.vogelsgesang@tum.de>
Instead of having two discrete InputStream and OutputStream helpers,
this merges the two into a unifed 'Transport' handler.
This handler is responsible for reading the DAP message headers, parsing
the resulting JSON and converting the messages into
`lldb_dap::protocol::Message`s for both input and output.
---------
Co-authored-by: Jonas Devlieghere <jonas@devlieghere.com>
Improving logging by defining new helpers for more uniform log handling.
This should help us clearly identify log messages and helps abstract the
underlying log type within the macro in case we want to update the log
handler in the future.
Both spellings are considered correct and acceptable, with adapter being
more common in American English. Given that DAP stands for Debug Adapter
Protocol (with an e) let's go with that as the canonical spelling.
This simplifies the IOStream.cpp implementation by building on top of
the existing lldb::IOObjectSP.
Additionally, this should help ensure clients connected of a
`--connection` specifier properly detect shutdown requests when the
Socket is closed. Previously, the StreamDescriptor was just accessing
the underlying native handle and was not aware of the `Close()` call to
the Socket itself.
This is both nice to have for simplifying the existing code and this
unblocks an upcoming refactor to support the cancel request.
---------
Co-authored-by: Jonas Devlieghere <jonas@devlieghere.com>
This refactors the response handlers for reverse request to follow the
same architecture as the request handlers. With only two implementation
that might be overkill, but it reduces code duplication and improves
error reporting by storing the sequence ID. This PR also fixes an
unchecked Expected in the old callback for unknown sequence IDs.
The TestDAP_ouput test is flaky due to the order of events during
shutdown. We were stopping the output and error handle redirection after
we finished the disconnect request, which can cause us to miss output
events due to timing. Moving when we stop the redirection to ensure we
have consistent output prior to disconnect responding.
Fixes#128567
Currently, all request handlers are implemented as free functions in
lldb-dap.cpp. That file has grown to over 5000 lines and is starting to
become hard to maintain. This PR moves the request handlers into their
own class (and file), together with their documentation.
This PR migrates about a third of the request handlers and the rest will
be migrated in subsequent commits. I'm merging this in an incomplete
state because almost any lldb-dap change is going to result in merge
conflicts and migrating request handlers one by one is easier to review.
This adjusts the lldb-dap listening mode to accept multiple clients.
Each client initializes a new instance of DAP and an associated
`lldb::SBDebugger` instance.
The listening mode is configured with the `--connection` option and
supports listening on a port or a unix socket on supported platforms.
When running in server mode launch and attach performance should
be improved by lldb sharing symbols for core libraries between debug
sessions.
This commit adds support for column breakpoints to lldb-dap
To do so, support for the `breakpointLocations` request was
added. To find all available breakpoint positions, we iterate over
the line table.
The `setBreakpoints` request already forwarded the column correctly to
`SBTarget::BreakpointCreateByLocation`. However, `SourceBreakpointMap`
did not keep track of multiple breakpoints in the same line. To do so,
the `SourceBreakpointMap` is now indexed by line+column instead of by
line only.
This was previously submitted as #113787, but got reverted due to
failures on ARM and macOS. This second attempt has less strict test
case expectations. Also, I added a release note.
This moves the ownership of the threads that forward stdout/stderr to
the DAP object itself to ensure that the threads are joined and that the
forwarding is cleaned up when the DAP connection is disconnected.
This is part of a larger refactor to allow lldb-dap to run in a
listening mode and accept multiple connections.
This reverts the previous revert and now that the underlying Windows
issue was fixed by 3ea2b546a8d17014d3ecf05356ecfaadf26ed846.
