Fixes:
```
[6373/7138] Building CXX object tools\lldb\tools\lldb-dap\CMakeFiles\lldb-dap.dir\DAP.cpp.obj
C:\git\llvm-project\lldb\tools\lldb-dap\DAP.cpp(725) : warning C4715: '`lldb_dap::DAP::HandleObject'::`30'::<lambda_2>::operator()': not all control paths return a value
[6421/7138] Building CXX object tools\lldb\tools\lldb-dap\CMakeFiles\lldb-dap.dir\Protocol\ProtocolTypes.cpp.obj
C:\git\llvm-project\lldb\tools\lldb-dap\Protocol\ProtocolTypes.cpp(203) : warning C4715: 'lldb_dap::protocol::ToString': not all control paths return a value
C:\git\llvm-project\lldb\tools\lldb-dap\Protocol\ProtocolTypes.cpp(98) : warning C4715: 'lldb_dap::protocol::toJSON': not all control paths return a value
C:\git\llvm-project\lldb\tools\lldb-dap\Protocol\ProtocolTypes.cpp(72) : warning C4715: 'lldb_dap::protocol::toJSON': not all control paths return a value
C:\git\llvm-project\lldb\tools\lldb-dap\Protocol\ProtocolTypes.cpp(111) : warning C4715: 'lldb_dap::protocol::toJSON': not all control paths return a value
[6426/7138] Building CXX object tools\lldb\tools\lldb-dap\CMakeFiles\lldb-dap.dir\Protocol\ProtocolBase.cpp.obj
C:\git\llvm-project\lldb\tools\lldb-dap\Protocol\ProtocolBase.cpp(287) : warning C4715: 'lldb_dap::protocol::fromJSON': not all control paths return a value
```
Handle signals in a separate thread in the driver so that we can stop
worrying about signal safety of functions in libLLDB that may get called
from a signal handler.
debugserver takes the address of a watchpoint exception and calculates
which watchpoint was responsible for it. There was an off-by-one error
in the range calculation which causes two watchpoints on consecutive
ranges to not correctly identify hits to the second watchpoint. The
result is that lldb wouldn't show the second watchpoint as ever being
hit.
Re-landing this test with a modification to only require two
watchpoints in the test, instead of four. If four watchpoints can
be set, it will test them.
rdar://145107575
This reverts commit 21d912121c9f41385b165a736be787527f5bd7c2.
Failure on the aarch64 ubuntu bot when setting the 4th watchpoint;
may be a hardware limitation on that bot. I thought creating four
watchpoints would be generally safe, but I don't need to do that
for my test, will re-land without it.
debugserver takes the address of a watchpoint exception and calculates
which watchpoint was responsible for it. There was an off-by-one error
in the range calculation which causes two watchpoints on consecutive
ranges to not correctly identify hits to the second watchpoint. The
result is that lldb wouldn't show the second watchpoint as ever being
hit.
rdar://145107575
debugserver isn't saving and restoring the SVE/SME register state around
inferior function calls.
Making arbitrary function calls while in Streaming SVE mode is generally
a poor idea because a NEON instruction can be hit and crash the
expression execution, which is how I missed this, but they should be
handled correctly if the user knows it is safe to do.
Re-landing this change after fixing an incorrect behavior on systems
without SME support.
rdar://146886210
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.
This reverts commit 4e40c7c4bd66d98f529a807dbf410dc46444f4ca.
arm64 CI is getting a failure in
lldb-api.tools/lldb-server.TestGdbRemoteRegisterState.py
with this commit, need to investigate and re-land.
debugserver isn't saving and restoring the SVE/SME register state around
inferior function calls.
Making arbitrary function calls while in Streaming SVE mode is generally
a poor idea because a NEON instruction can be hit and crash the
expression execution, which is how I missed this, but they should be
handled correctly if the user knows it is safe to do.
rdar://146886210
Add a -v/--version command line argument to print the version of both
the lldb-dap binary and the liblldb it's linked against.
This is motivated by me trying to figure out which lldb-dap I had in my
PATH.
Since lldb 20, these have had no effect:
https://releases.llvm.org/20.1.0/docs/ReleaseNotes.html#changes-to-lldb
> lldb-server now listens to a single port for gdbserver connections and
> provides that port to the connection handler processes. This means
that
> only 2 ports need to be opened in the firewall (one for the
lldb-server
> platform, one for gdbserver connections). In addition, due to this
work,
lldb-server now works on Windows in the server mode.
Remove them.
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.
Hey,
This solves an issue where running lldb-server-20 with a non-absolute
path (for example, when it's installed into `/usr/bin` and the user runs
it as `lldb-server-20 ...` and not `/usr/bin/lldb-server-20 ...`) fails
with `error: spawn_process failed: execve failed: No such file or
directory`. The underlying issue is that when run that way, it attempts
to execute a binary named `lldb-server-20` from its current directory.
This is also a mild security hazard because lldb-server is often being
run as root in the directory /tmp, meaning that an unprivileged user can
create the file /tmp/lldb-server-20 and lldb-server will execute it as
root. (although, well, it's a debugging server we're talking about, so
that may not be a real concern)
I haven't previously contributed to this project; if you want me to
change anything in the code please don't hesitate to let me know.
Running tests with ubsan enabled showed that the current
`protocol::AdapterFeature` and `protocol::ClientFeature` enums are
incorrectly defined if we want to use them in a `llvm::DenseSet`. The
enums are currently untyped and this results in the undefined behavior.
Adding `FLAGS_ENUM()` wrappers to all the enums in the
`lldb-dap::protocol` namespace to ensure they're typed so they can be
used with types like `llvm::DenseSet`.
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>
Added a new setting called `lldb-dap.arguments` and a debug
configuration attribute called `debugAdapterArgs` that can be used to
set the arguments used to launch the debug adapter. Right now this is
mostly useful for debugging purposes to add the `--wait-for-debugger`
option to lldb-dap.
Additionally, the extension will now check for a changed lldb-dap
executable or arguments when launching a debug session in server mode. I
had to add a new `DebugConfigurationProvider` to do this because VSCode
will show an unhelpful error modal when the
`DebugAdapterDescriptorFactory` returns `undefined`.
In order to facilitate this, I had to add two new properties to the
launch configuration that are used by the
`DebugAdapterDescriptorFactory` to tell VS Code how to launch the debug
adapter:
- `debugAdapterHostname` - the hostname for an existing lldb-dap server
- `debugAdapterPort` - the port for an existing lldb-dap server
I've also removed the check for the `executable` argument in
`LLDBDapDescriptorFactory.createDebugAdapterDescriptor()`. This argument
is only set by VS Code when the debug adapter executable properties are
set in the `package.json`. The LLDB DAP extension does not currently do
this (and I don't think it ever will). So, this makes the debug adapter
descriptor factory a little easier to read.
The check for whether or not `lldb-dap` exists has been moved into the
new `DebugConfigurationProvider` as well. This way the extension won't
get in the user's way unless they actually try to start a debugging
session. The error will show up as a modal which will also make it more
obvious when something goes wrong, rather than popping up as a warning
at the bottom right of the screen.
We've been dealing with UBSAN issues around this code for some time now
(see `9c36859b33b386fbfa9599646de1e2ae01158180` and
`1a2122e9e9d1d495fdf337a4a9445b61ca56df6f`). On recent macOS versions, a
UBSAN-enabled debugserver will crash when performing a `memcpy` of the
input `mach_exception_data_t`. The pointer to the beginning of the
exception data may not be aligned on a doubleword boundary, leading to
UBSAN failures such as:
```
$ ./bin/debugserver 0.0.0.0:5555 /Volumes/SSD/llvm-builds/llvm-worktrees/clang-work/build-sanitized-release/tools/lldb/test/Shell/Recognizer/Output/verbose_trap.test.tmp.out
/Volumes/SSD/llvm-builds/llvm-worktrees/clang-work/lldb/tools/debugserver/source/MacOSX/MachException.cpp:35:12: runtime error: store to misaligned address 0x00016ddfa634 for type 'mach_exception_data_type_t *' (aka 'long long *'), which requires 8 byte alignment
0x00016ddfa634: note: pointer points here
02 00 00 00 03 00 01 00 00 00 00 00 11 00 00 00 00 00 00 00 00 00 00 00 08 00 00 00 00 00 00 00
^
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /Volumes/SSD/llvm-builds/llvm-worktrees/clang-work/lldb/tools/debugserver/source/MacOSX/MachException.cpp:35:12
```
Work around these failures by pretending the input data is a `char*`
buffer.
Drive-by changes:
* I factored out some duplicated code into a static
`AppendExceptionData` and made the types consistent
---------
Co-authored-by: Jonas Devlieghere <jonas@devlieghere.com>
The `DAPError` can be used to craft an error message that is displayed
to a user (with showUser=true).
Any request handler implementation using subclassing `RequestHandler<>`
should be able to use this.
I updated SourceRequestHandler to report DAPError's specifically.
In 2013 we added the QSaveRegisterState and QRestoreRegisterState
packets to checkpoint a thread's register state while executing an
inferior function call, instead of using the g packet to read all
registers into lldb, then the G packet to set them again after the func
call.
Since then, lldb has not sent g/G (except as a bug) - it either asks for
registers individually (p/P) or or asks debugserver to save and restore
the entire register set with these lldb extensions.
Felipe recently had a codepath that fell back to using g/G and found
that it does not work with the modern signed fp/sp/pc/lr registers that
we can get -- it sidesteps around the clearing of the non-addressable
bits that we do when reading/writing them, and results in a crash. (
https://github.com/llvm/llvm-project/pull/132079 )
Instead of fixing that issue, I am removing g/G from debugserver because
it's not needed by lldb, and it will would be easy for future bugs to
creep in to this packet that lldb should not use, but it can
accidentally fall back to and result in subtle bugs.
This does mean that a debugger using debugserver on darwin which doesn't
use QSaveRegisterState/QRestoreRegisterState will need to fall back to
reading & writing each register individually. I'm open to re-evaluating
this decision if this proves to be needed, and supporting these lldb
extensions is onerous.
This fixes an uncommon bug with debugserver controlling an inferior
process that is hitting an internal breakpoint & continuing when
multiple interrupts are sent by SB API to lldb -- with the result being
that lldb never stops the inferior process, ignoring the interrupt/stops
being sent by the driver layer (Xcode, in this case).
In the reproducing setup (which required a machine with unique timing
characteristics), lldb is sent SBProcess::Stop and then shortly after,
SBProcess::SendAsyncInterrupt. The driver process only sees that the
inferior is publicly running at this point, even though it's hitting an
internal breakpoint (new dylib being loaded), disabling the bp, step
instructioning, re-enabling the breakpoint, then continuing.
The packet sequence lldb sends to debugserver looks like
1. vCont;s // instruction step
2. ^c // async interrupt
3. Z.... // re-enable breakpoint
4. c // resume inferior execution
5. ^c // async interrupt
When debugserver needs to interrupt a running process
(`MachProcess::Interrupt`), the main thread in debugserver sends a
SIGSTOP posix signal to the inferior process, and notes that it has sent
this signal by setting `m_sent_interrupt_signo`.
When we send the first async interrupt while instruction stepping, the
signal is sent (probably after the inferior has already stopped) but
lldb can only *receive* the mach exception that includes the SIGSTOP
when the process is running. So at the point of step (3), we have a
SIGSTOP outstanding in the kernel, and
`m_sent_interrupt_signo` is set to SIGSTOP.
When we resume the inferior (`c` in step 4), debugserver sees that
`m_sent_interrupt_signo` is still set for an outstanding SIGSTOP, but at
this point we've already stopped so it's an unnecessary stop. It records
that (1) we've got a SIGSTOP still coming that debugserver sent and (2)
we should ignore it by also setting `m_auto_resume_signo` to the same
signal value.
Once we've resumed the process, the mach exception thread
(`MachTask::ExceptionThread`) receives the outstanding mach exception,
adds it to a queue to be processed
(`MachProcess::ExceptionMessageReceived`) and when we've collected all
outstanding mach exceptions, it calls
`MachProcess::ExceptionMessageBundleComplete` top evaluate them.
`MachProcess::ExceptionMessageBundleComplete` halts the process (without
updating the MachProcess `m_state`) while evaluating them. It sees that
this incoming SIGSTOP was meant to be ignored (`m_auto_resume_signo` is
set), so it `MachProcess::PrivateResume`'s the process again.
At the same time `MachTask::ExceptionThread` is receiving and processing
the ME, `MachProcess::Interrupt` is called with another interrupt that
debugserver received. This method checks that we're still eStateRunning
(we are) but then sees that we have an outstanding SIGSTOP already
(`m_sent_interrupt_signo`) and does nothing, assuming that we will stop
shortly from that one. It then returns to call
`RNBRemote::HandlePacket_last_signal` to print the status -- but because
the process is still `eStateRunning`, this does nothing.
So the first ^c (resulting in a pending SIGSTOP) is received and we
resume the process silently. And the second ^c is ignored because we've
got one interrupt already being processed.
The fix was very simple. In `MachProcess::Interrupt` when we detect that
we have a SIGSTOP out in the wild (`m_sent_interrupt_signo`), we need to
clear `m_auto_resume_signo` which is used to indicate that this SIGSTOP
is meant to be ignored, because it was from before our most recent
resume.
MachProcess::Interrupt holds the `m_exception_and_signal_mutex` mutex
already (after Jonas's commit last week), and all of
`MachProcess::ExceptionMessageBundleComplete` holds that same mutex, so
we know we can modify `m_auto_resume_signo` here and it will be handled
correctly when the outstanding mach exception is finally processed.
rdar://145872120
I can't figure out why this would be necessary. Nothing is checking if
libpthread is available, nothing in lldb-dap is relying on libpthread
directly and nothing else in LLDB is doing this.
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>
This reverts commit
87b7f63a11,
reapplying
7e66cf74fb
with a small (and probably temporary)
change to generate more debug info to help with diagnosing buildbot
issues.
Match the description of the Visual Studio Code extension with the
wording used by popular/official extensions. Most extension spell out
"Visual Studio Code" and use "in" instead of "from".
This commit adds support for starting debug sessions through special
`vscode://llvm-vs-code-extensions.lldb-dap/start?config={launch-config}`
URIs. This allows tighter integration with custom scripts. One potential
use case is providing similar functionality to `xcdebug`, see #125777
for some discussion on that use case.
The functionality was inspired by @vadimcn's CodeLLDB extension, which
[provides similar
functionality](https://github.com/vadimcn/codelldb/blob/master/MANUAL.md#debugging-externally-launched-code).
The mutex in RNBRemote::CommDataReceived protects m_rx_packets and
should extend to the end of the function to cover the read where we
check if the list is empty.
This PR fixes a race condition in debugserver where the main thread
calls MachProcess::Interrupt, setting `m_sent_interrupt_signo` while the
exception monitoring thread is checking the value of the variable.
I was on the fence between introducing a new mutex and reusing the
existing exception mutex. With the notable exception of
MachProcess::Interrupt, all the other places where we were already
locking this mutex before accessing the variable. I renamed the mutex to
make it clear that it's now protecting more than the exception messages.
Jason, while investigating a real issue, had a suspicion there was race
condition related to interrupts and I was able to narrow it down by
building debugserver with TSan.
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>
This should ensure the extension only uses server mode if the binary
supports the feature, otherwise it will fallback to the existing
behavior.
Fixes#130854
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.
Per the DAP spec, the event 'body' field should contain any additional
data related to the event. I updated the lldb-dap 'statistics' extension
into the terminated event's body like:
```
{
"type": "event",
"seq": 0,
"event": "terminated",
"body": {
"$__lldb_statistics": {...}
}
}
```
This allows us to more uniformly handle event messages.
While running lldb-dap over stdin/stdout the `stdout` and `stderr` FD's
are replaced with a pipe that is reading the output to forward to the
dap client. During shutdown we were not properly restoring those FDs,
which means if any component attempted to write to stderr it would
trigger a SIGPIPE due to the pipe being closed during the shutdown
process. This can happen if we have an error reported from the
`DAP::Loop` call that would then log to stderr, such as an error parsing
a malformed DAP message or if lldb-dap crashed and it was trying to
write the stack trace to stderr.
There is one place we were not handling an `llvm::Error` if there was no
logging setup that could trigger this condition.
To address this, I updated the OutputRedirector to restore the FD to the
prior state when `Stop` is called.
---------
Co-authored-by: Jonas Devlieghere <jonas@devlieghere.com>
