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 updates the `attach` request to the typed
`RequestHandler<protocol::AttachRequestArguments,
protocol::AttachResponse>`.
Added a few more overlapping configurations to
`lldb_dap::protocol::Configuration` that are shared between launching
and attaching.
There may be some additional code we could clean-up that is no longer
referenced now that this has migrated to use well defined types.
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
Currently, we are only using the executable name when attaching. The
AttachRequestHandler isn't setting the path in the attach info, which
means that we rely on the target when attaching by name. When wo go down
this path, we only look at the executable's filename, not its full path.
Since we know the full path from the attach arguments, we should specify
it in the attach info.
Fixes#138197
While debugging the flakiness of the launch and attach tests, I noticed
that we have some places in lldb-dap where we put the debugger in
synchronous mode and have an early exit, that would leave the debugger
in this state. This PR introduces an RAII helper to avoid such mistakes.
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 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.
Replace Get{Signed,Unsigned} with GetInteger<T> and return std::optional
so you can distinguish between the value not being present and it being
explicitly set to the previous fail_value. All existing uses are
replaced by calling value_or(fail_value).
Continuation of #129818
Return a std::optional<bool> from GetBoolean so you can distinguish
between the value not being present and it being explicitly set to true
or false. All existing uses are replaced by calling
`value_or(fail_value`).
Motivated by #129753
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.
