This patch fixes a possible data race between main and event handler
threads. Terminated event can be sent from `Disconnect` function or
event handler. Consequently, there are some possible sequences of
events. We must check events twice, because without getting an exited
event, `exit_status` will be None. But, we don't know the order of
events (for example, we can get terminated event before exited event),
so we check events by filter. It is correct, because terminated event
will be sent only once (guarded by `llvm::call_once`).
This patch moved from
[145010](https://github.com/llvm/llvm-project/pull/145010) and based on
idea from this
[comment](https://github.com/llvm/llvm-project/pull/145010#discussion_r2159637210).
.. from the guts of GDBRemoteCommunication to ~top level.
This is motivated by #131519 and by the fact that's impossible to guess
whether the author of a symlink intended it to be a "convenience
shortcut" -- meaning it should be resolved before looking for related
files; or an "implementation detail" -- meaning the related files should
be located near the symlink itself.
This debate is particularly ridiculous when it comes to lldb-server
running in platform mode, because it also functions as a debug server,
so what we really just need to do is to pass /proc/self/exe in a
platform-independent manner.
Moving the location logic higher up achieves that as lldb-platform (on
non-macos) can pass `HostInfo::GetProgramFileSpec`, while liblldb can
use the existing complex logic (which only worked on liblldb anyway as
lldb-platform doesn't have a lldb_private::Platform instance).
Another benefit of this patch is a reduction in dependency from
GDBRemoteCommunication to the rest of liblldb (achieved by avoiding the
Platform dependency).
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.
# Benefit
This patch fixes:
1. After `platform select ios-simulator`, `platform process list` will
now print processes which are running in the iOS simulator. Previously,
no process will be listed.
2. After `platform select ios-simulator`, `platform attach --name
<name>` will succeed. Previously, it will error out saying no process is
found.
# Several bugs that is being fixed
1. During the process listing, add `aarch64` to the list of CPU types
for which iOS simulators are checked for.
2. Given a candidate process, when checking for simulators, the original
code will find the desired environment variable (`SIMULATOR_UDID`) and
set the OS to iOS, but then the immediate next environment variable will
set it back to macOS.
3. For processes running on simulator, set the triple's `Environment` to
`Simulator`, so that such processes can pass the filtering [in this
line](https://fburl.com/8nivnrjx). The original code leave it as the
default `UnknownEnvironment`.
# Manual test
**With this patch:**
```
royshi-mac-home ~/public_llvm/build % bin/lldb
(lldb) platform select ios-simulator
(lldb) platform process list
240 matching processes were found on "ios-simulator"
PID PARENT USER TRIPLE NAME
====== ====== ========== ============================== ============================
40511 28844 royshi arm64-apple-ios-simulator FocusPlayground // my toy iOS app running on simulator
... // omit
28844 1 royshi arm64-apple-ios-simulator launchd_sim
(lldb) process attach --name FocusPlayground
Process 40511 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = signal SIGSTOP
frame #0: 0x0000000104e3cb70 libsystem_kernel.dylib`mach_msg2_trap + 8
libsystem_kernel.dylib`mach_msg2_trap:
-> 0x104e3cb70 <+8>: ret
... // omit
```
**Without this patch:**
```
$ bin/lldb
(lldb) platform select ios-simulator
(lldb) platform process list
error: no processes were found on the "ios-simulator" platform
(lldb) process attach --name FocusPlayground
error: attach failed: could not find a process named FocusPlayground
```
# Unittest
See PR.
```
Traceback (most recent call last):
File "/home/worker/2.0.1/lldb-x86_64-debian/llvm-project/lldb/test/API/dotest.py", line 8, in <module>
lldbsuite.test.run_suite()
File "/home/worker/2.0.1/lldb-x86_64-debian/llvm-project/lldb/packages/Python/lldbsuite/test/dotest.py", line 1064, in run_suite
visit("Test", dirpath, filenames)
File "/home/worker/2.0.1/lldb-x86_64-debian/llvm-project/lldb/packages/Python/lldbsuite/test/dotest.py", line 706, in visit
visit_file(dir, name)
File "/home/worker/2.0.1/lldb-x86_64-debian/llvm-project/lldb/packages/Python/lldbsuite/test/dotest.py", line 649, in visit_file
module = __import__(base)
^^^^^^^^^^^^^^^^
File "/home/worker/2.0.1/lldb-x86_64-debian/llvm-project/lldb/test/API/tools/lldb-dap/runInTerminal/TestDAP_runInTerminal.py", line 14, in <module>
class TestDAP_runInTerminal(lldbdap_testcase.DAPTestCaseBase):
File "/home/worker/2.0.1/lldb-x86_64-debian/llvm-project/lldb/test/API/tools/lldb-dap/runInTerminal/TestDAP_runInTerminal.py", line 29, in TestDAP_runInTerminal
@skipIfAsan
^^^^^^^^^^
NameError: name 'skipIfAsan' is not defined
```
These fail with on the macOS ASAN CI with following error:
```
07:10:31 ======================================================================
07:10:31 FAIL: test_runInTerminalWithObjectEnv (TestDAP_runInTerminal.TestDAP_runInTerminal)
07:10:31 Tests the "runInTerminal" reverse request. It makes sure that the IDE can
07:10:31 ----------------------------------------------------------------------
07:10:31 Traceback (most recent call last):
07:10:31 File "/Users/ec2-user/jenkins/workspace/llvm.org/lldb-cmake-sanitized/llvm-project/lldb/packages/Python/lldbsuite/test/decorators.py", line 149, in wrapper
07:10:31 return func(*args, **kwargs)
07:10:31 File "/Users/ec2-user/jenkins/workspace/llvm.org/lldb-cmake-sanitized/llvm-project/lldb/test/API/tools/lldb-dap/runInTerminal/TestDAP_runInTerminal.py", line 83, in test_runInTerminalWithObjectEnv
07:10:31 self.build_and_launch(program, runInTerminal=True, env={"FOO": "BAR"})
07:10:31 File "/Users/ec2-user/jenkins/workspace/llvm.org/lldb-cmake-sanitized/llvm-project/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py", line 488, in build_and_launch
07:10:31 return self.launch(program, **kwargs)
07:10:31 File "/Users/ec2-user/jenkins/workspace/llvm.org/lldb-cmake-sanitized/llvm-project/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py", line 470, in launch
07:10:31 self.assertTrue(
07:10:31 AssertionError: False is not true : launch failed (process exited during launch or attach)
07:10:31 Config=arm64-/Users/ec2-user/jenkins/workspace/llvm.org/lldb-cmake-sanitized/lldb-build/bin/clang
```
These fail ever since they were enabled in https://github.com/llvm/llvm-project/pull/144954.
Skip these again when testing with ASAN.
It is failing on our Windows on Arm bot:
AssertionError: False is not true : launch failed (Unimplemented)
Given that all the others are skipped on Windows, I assume this
failure is expected too.
Replace `isTestSupported` function with `skipIfBuildType` annotation.
Test that uses the `IsTestSupported` function are no longer run, as the
size of lldb-dap binary is now more than `1mb`.
Update the broken test.
Fixes#108621
We could probably check if the test now passes on `linux arm` since it
was disabled because it timed out. I experienced the timeout after
replacing the `IsTestSupported` with `skipIfBuildType`.
In DebugCommunication, we currently are using 2 thread to drive
lldb-dap. At the moment, they make an attempt at only synchronizing the
`recv_packets` between the reader thread and the main test thread. Other
stateful properties of the debug session are not guarded by a
locks/mutex.
To mitigate this, I am moving any state updates to the main thread
inside the `_recv_packet` method to ensure that between calls to
`_recv_packet` the state does not change out from under us in a test.
This does mean the precise timing of events has changed slightly as a
result and I've updated the existing tests that fail for me locally with
this new behavior.
I think this should result in overall more predictable behavior, even if
the test is slow due to the host workload or architecture differences.
---------
Co-authored-by: Ebuka Ezike <yerimyah1@gmail.com>
Currently when jitting expressions, LLDB scans the IR instructions of
the `$__lldb_expr` and will insert a call to a utility function for each
load/store instruction. The purpose of the utility funciton is to
dereference the load/store operand. If that operand was an invalid
pointer the utility function would trap and LLDB asks the IR checker
whether it was responsible for the trap, in which case it prints out an
error message saying the expression dereferenced an invalid pointer.
This is a lot of setup for not much gain. In fact, creating/running this
utility expression shows up as ~2% of the expression evaluation time
(though we cache them for subsequent expressions). And the error message
we get out of it is arguably less useful than if we hadn't instrumented
the IR. It was also untested.
Before:
```
(lldb) expr int a = *returns_invalid_ptr()
error: Execution was interrupted, reason: Attempted to dereference an invalid pointer..
The process has been returned to the state before expression evaluation.
```
After:
```
(lldb) expr int a = *returns_invalid_ptr()
error: Expression execution was interrupted: EXC_BAD_ACCESS (code=1, address=0x5).
The process has been returned to the state before expression evaluation.
```
This patch removes this IR checker.
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.
This adds a new 'CapabilitiesEventBody' type for having a well
structured type for the event and updates the 'restart' request
to dynamically set their capabilities.
This reverts commit 4b6c608615a285d81132acf8e33b81b2ec2c9bf9 and
follow-up commits 159de3633640a5cb2d322ebe8cc4ec0c1c9a896d and
c9e1c52e2e75a91a44a98df818cc9bd11655e51d because this breaks
TestDAP_stepInTargets.py on Darwin.
https://green.lab.llvm.org/job/llvm.org/view/LLDB/job/as-lldb-cmake
Trimming unused imports, adjusting the test to use the `DEFAULT_TIMEOUT`
instead of a custom timeout and adjusting the flow to stopOnEntry for
improving consistency.
Previously, we incorrectly handled the disconnect operation if we signal
lldb-dap running in server mode.
Updated to correctly disconnect the attach vs launch behavior on server
shutdown.
Attempt to improve tests by synchronously waiting for breakpoints to
resolve. Not sure if it will fix all the tests but I think it should
make the tests more stable
This reverts commit 8a49db35f45e56c92522c6079e51553e80c07aec.
Due to failures on Arm and AArch64 Linux:
https://lab.llvm.org/buildbot/#/builders/59/builds/18540https://lab.llvm.org/buildbot/#/builders/18/builds/16759
File "/home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/llvm-project/lldb/test/API/tools/lldb-dap/evaluate/TestDAP_evaluate.py", line 22, in assertEvaluateFailure
self.assertNotIn(
AssertionError: 'result' unexpectedly found in {'memoryReference': '0xFFFFF7CB3060', 'result': '0x0000000000000000', 'type': 'int *', 'variablesReference': 7}
FAIL: test_generic_evaluate_expressions (TestDAP_evaluate.TestDAP_evaluate)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/llvm-project/lldb/test/API/tools/lldb-dap/evaluate/TestDAP_evaluate.py", line 228, in test_generic_evaluate_expressions
self.run_test_evaluate_expressions(enableAutoVariableSummaries=False)
File "/home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/llvm-project/lldb/test/API/tools/lldb-dap/evaluate/TestDAP_evaluate.py", line 117, in run_test_evaluate_expressions
self.assertEvaluateFailure("list") # local variable of a_function
File "/home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/llvm-project/lldb/test/API/tools/lldb-dap/evaluate/TestDAP_evaluate.py", line 22, in assertEvaluateFailure
self.assertNotIn(
AssertionError: 'result' unexpectedly found in {'memoryReference': '0xFFFFF7CB3060', 'result': '0x0000000000000000', 'type': 'int *', 'variablesReference': 7}
Config=aarch64-/home/tcwg-buildbot/worker/lldb-aarch64-ubuntu/build/bin/clang
The second one is because our bots have the libc debug info package installed,
the first, no idea.
When a target thread returned an empty but not `nullptr` string as its
name, the thread would show up with an empty name in lldb-dap.
I don't know how this works on macOS and Linux, but on Windows,
[`TargetThreadWindows::GetName`](deedc8a181/lldb/source/Plugins/Process/Windows/Common/TargetThreadWindows.cpp (L178-L204))
returns a non-null pointer to an empty string, because on MSVC's STL,
`std::string{}.c_str()` returns a pointer to inside the object (the SSO
storage).
This changes the check in `CreateThread`, when no custom thread
formatter is set, to check for the length of the thread and queue name
instead of it being `nullptr`.
Improving the readability and correctness of DebugCommunication by
adding type annotations to many parts of the library and trying to
improve the implementation of a few key areas of the code to better
handle correctness.
Specifically, this refactored the
`DebugCommunication._handle_recv_packet` function to ensure consistency
with the reader thread when handling state changes and improved the
`DebugCommunication._recv_packet` helper to make it easier to follow by
adding some additional helpers.
The architectures provided to skipIf / expectedFail are regular
expressions (v. _match_decorator_property() in decorators.py
so on Darwin systems "arm64" would match the skips for "arm" (32-bit
Linux). Update these to "arm$" to prevent this, and also update
three tests (TestBuiltinFormats.py, TestCrossDSOTailCalls.py,
TestCrossObjectTailCalls.py) that were skipped for arm64 via this
behavior, and need to be skipped or they will fail.
This was moviated by the new TestDynamicValue.py test which has
an expected-fail for arm, but the test was passing on arm64 Darwin
resulting in failure for the CIs.
- `CreateRunInTerminalReverseRequest` is passed a `StringMap` by value,
whose keys are owned and deallocated after return.
- Target args are wrongly specified as reverse request (launcher) args.
- Test case error message did not catch up with a0aa5f8.
All runInTerminal tests are passing with this applied.
New test added by https://github.com/llvm/llvm-project/pull/139969.
On Windows we need debug information to be able to break on the
function (we don't need it for main, but I assume that's a special case).
So disable the test on Windows.
I tried a few tricks like making a global label in assembly, but
that doesn't show up without debug info either.
Fix the handling of the `instructionOffset` parameter, which resulted in
always returning the wrong disassembly because VSCode always uses
`instructionOffset = -50` and expects 50 instructions before the given
address, instead of 50 bytes before
Recently, I got a report how a user 'hung', and come to find out it's
actually because DAP is checking percentage, and on non-deterministic
events, we will effectively never send a progress event to DAP. Here we
short circuit and don't view percentages for DAP messages and solely
depend on the DAP 250ms throttle, which is probably still too
aggressive, but better than no updates.
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.
Adding an assert that the 'continue' request succeeds caused a number of
tests to fail. This showed a number of tests that were not specifying if
they should be stopped or not at key points in the test. This is likely
contributing to these tests being flaky since the debugger is not in the
expected state.
Additionally, I spent a little time trying to improve the readability of
the dap_server.py and lldbdap_testcase.py.
To improve logging this adjusts two properties of the existing tests:
* Forwards stderr from lldb-dap to the process in case errors are
reported to stderr.
* Adjusts `DebugAdapterServer.terminate` to close stdin and wait for the
process to exit instead of sending SIGTERM. Additionally, if we end up
with a non-zero exit status we now raise an error to note the unexpected
exit status.
With these changes, I did find one test case in
`TestDAP_console.test_diagnositcs` that was not waiting to ensure the
expected event had arrived by the time it performed an assert.
test_common is force-included into every compilation, which causes
problems when we're compiling assembly code, as we were in #138805.
This avoids that as we can include the header only when it's needed.
Here we were initializing & locking a shared_mutex in a thread, while
releasing it in the parent which may/often turned out to be a different
thread (shared_mutex::unlock_shared is undefined behavior if called from
a thread that doesn't hold the lock).
Switch to counter to more simply keep track of number of readers and
simply lock/unlock rather than utilizing reader mutex to verify last
freed (and so requiring this matching thread init/destroy behavior).
These are currently failing on Windows on Arm:
https://lab.llvm.org/buildbot/#/builders/141/builds/8556
```
********************
Unresolved Tests (1):
lldb-api :: tools/lldb-dap/memory/TestDAP_memory.py
********************
Failed Tests (1):
lldb-api :: tools/lldb-dap/variables/TestDAP_variables.py
```
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.
Since https://github.com/llvm/llvm-project/pull/138981 / aeeb9a3c09
were landed and tests re-enabled, these tests have been failing
on our Windows on Arm bot:
https://lab.llvm.org/buildbot/#/builders/141/builds/8523
********************
Unresolved Tests (1):
lldb-api :: tools/lldb-dap/send-event/TestDAP_sendEvent.py
********************
Failed Tests (2):
lldb-api :: tools/lldb-dap/launch/TestDAP_launch.py
lldb-api :: tools/lldb-dap/stackTrace/TestDAP_stackTrace.py
Re-enable the lldb-dap tests. We've spent the last week improving the
reliability of the test suite and the tests now pass reliably on macOS
and Linux at desk. Let's see how things fare on the bots.
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
