c72c0b298c fixed a race condition in Target::GetExecutableModule. The
patch originally added the lock_guard but I suggested using the locking
ModuleList::Modules() helper instead. That didn't consider that the
fallback would still access the ModuleList without holding the lock.
This patch fixes the remaining issue.
…ess" (#144810)
This relands commit e0933ab5ae. The
original commit was causing the test TestCreateAfterAttach.py to fail on
ARM Ubuntu bots. It's possible that this could've been happening because
the test for wait-attach progress reporting is waiting on a process
named "a.out" which could be too generic as multiple other tests (when
run in parallel on the bots) could also be using processes named
"a.out". This commit changes the wait-attach progress report test to
wait on a unique process name.
Original PR description:
This commit adds a progress report when wait-attaching to a process as
well as a test for this.
Original PR link: https://github.com/llvm/llvm-project/pull/144768
This is breaking TestCreateAfterAttach.py on Ubuntu:
```
======================================================================
FAIL: test_create_after_attach_dwo (TestCreateAfterAttach.CreateAfterAttachTestCase.test_create_after_attach_dwo)
Test thread creation after process attach.
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/buildbot/worker/as-builder-9/lldb-remote-linux-ubuntu/llvm-project/lldb/packages/Python/lldbsuite/test/lldbtest.py", line 1804, in test_method
return attrvalue(self)
^^^^^^^^^^^^^^^
File "/home/buildbot/worker/as-builder-9/lldb-remote-linux-ubuntu/llvm-project/lldb/packages/Python/lldbsuite/test/decorators.py", line 149, in wrapper
return func(*args, **kwargs)
^^^^^^^^^^^^^^^^^^^^^
File "/home/buildbot/worker/as-builder-9/lldb-remote-linux-ubuntu/llvm-project/lldb/test/API/functionalities/thread/create_after_attach/TestCreateAfterAttach.py", line 36, in test_create_after_attach
self.runCmd("process attach -p " + str(pid))
File "/home/buildbot/worker/as-builder-9/lldb-remote-linux-ubuntu/llvm-project/lldb/packages/Python/lldbsuite/test/lldbtest.py", line 1005, in runCmd
self.assertTrue(self.res.Succeeded(), msg + output)
AssertionError: False is not true : Command 'process attach -p 1474309' did not return successfully
Error output:
error: attach failed: lost connection
```
on the buildbots for lldb-remote-linux-ubuntu, lldb-arm-ubuntu,
lldb-aarch64-ubuntu, lldb-arm-ubuntu.
Since this PR: https://github.com/llvm/llvm-project/pull/141670/ We
started to override the Platform/Arch for a target if needed. However we
may have already registered locate module callback with the old
platform.
This PR will move the locate module callback to the new Platform
whenever Target changes architecture.
Co-authored-by: George Hu <georgehuyubo@gmail.com>
We're reading from the object's vtable to determine the pointer to the
full object. The vtable is normally in the "rodata" section of the
executable, which is often not included in the core file because it's
not supposed to change and the debugger can extrapolate its contents
from the executable file. We weren't doing that.
This patch changes the read operation to use the target class (which
falls back onto the executable module as expected) and adds the missing
ReadSignedIntegerFromMemory API. The fix is tested by creating a core
(minidump) file which deliberately omits the vtable pointer.
stop-hooks are supposed to trigger every time the process stops, but as
initially implemented they would only fire when control was returned to
the user. So for instance when a process was launched the stop hook
would only trigger when the process hit a breakpoint or crashed.
However, it would be really useful to be able to trigger a stop hook
when lldb first gains control over the process. One way to do that would
be to implement general "target lifecycle events" and then send process
created events that users could bind actions to.
OTOH, extending the stop hooks to fire when lldb first gains control
over the process is a pretty natural extension to the notion of a stop
hook. So this patch takes the shorter route to that ability by making
stop-hooks fire when lldb first gains control over the process.
I also added the ability to specify whether to trigger the stop hook "on
gaining control". I'm on the fence about whether to set the default to
be "trigger on gaining control" or "don't trigger on gaining control".
Since I think it's a generally useful feature, I've set the default to
"trigger on gaining control".
This patch improves LLDB launch time on Linux machines for **preload
scenarios**, particularly for executables with a lot of shared library
dependencies (or modules). Specifically:
* Launching a binary with `target.preload-symbols = true`
* Attaching to a process with `target.preload-symbols = true`.
It's completely controlled by a new flag added in the first commit
`plugin.dynamic-loader.posix-dyld.parallel-module-load`, which *defaults
to false*. This was inspired by similar work on Darwin #110646.
Some rough numbers to showcase perf improvement, run on a very beefy
machine:
* Executable with ~5600 modules: baseline 45s, improvement 15s
* Executable with ~3800 modules: baseline 25s, improvement 10s
* Executable with ~6650 modules: baseline 67s, improvement 20s
* Executable with ~12500 modules: baseline 185s, improvement 85s
* Executable with ~14700 modules: baseline 235s, improvement 120s
A lot of targets we deal with have a *ton* of modules, and unfortunately
we're unable to convince other folks to reduce the number of modules, so
performance improvements like this can be very impactful for user
experience.
This patch achieves the performance improvement by parallelizing
`DynamicLoaderPOSIXDYLD::RefreshModules` for the launch scenario, and
`DynamicLoaderPOSIXDYLD::LoadAllCurrentModules` for the attach scenario.
The commits have some context on their specific changes as well --
hopefully this helps the review.
# More context on implementation
We discovered the bottlenecks by via `perf record -g -p <lldb's pid>` on
a Linux machine. With an executable known to have 1000s of shared
library dependencies, I ran
```
(lldb) b main
(lldb) r
# taking a while
```
and showed the resulting perf trace (snippet shown)
```
Samples: 85K of event 'cycles:P', Event count (approx.): 54615855812
Children Self Command Shared Object Symbol
- 93.54% 0.00% intern-state libc.so.6 [.] clone3
clone3
start_thread
lldb_private::HostNativeThreadBase::ThreadCreateTrampoline(void*) r
std::_Function_handler<void* (), lldb_private::Process::StartPrivateStateThread(bool)::$_0>::_M_invoke(std::_Any_data const&)
lldb_private::Process::RunPrivateStateThread(bool) n
- lldb_private::Process::HandlePrivateEvent(std::shared_ptr<lldb_private::Event>&)
- 93.54% lldb_private::Process::ShouldBroadcastEvent(lldb_private::Event*)
- 93.54% lldb_private::ThreadList::ShouldStop(lldb_private::Event*)
- lldb_private::Thread::ShouldStop(lldb_private::Event*) *
- 93.53% lldb_private::StopInfoBreakpoint::ShouldStopSynchronous(lldb_private::Event*) t
- 93.52% lldb_private::BreakpointSite::ShouldStop(lldb_private::StoppointCallbackContext*) i
lldb_private::BreakpointLocationCollection::ShouldStop(lldb_private::StoppointCallbackContext*) k
lldb_private::BreakpointLocation::ShouldStop(lldb_private::StoppointCallbackContext*) b
lldb_private::BreakpointOptions::InvokeCallback(lldb_private::StoppointCallbackContext*, unsigned long, unsigned long) i
DynamicLoaderPOSIXDYLD::RendezvousBreakpointHit(void*, lldb_private::StoppointCallbackContext*, unsigned long, unsigned lo
- DynamicLoaderPOSIXDYLD::RefreshModules() O
- 93.42% DynamicLoaderPOSIXDYLD::RefreshModules()::$_0::operator()(DYLDRendezvous::SOEntry const&) const u
- 93.40% DynamicLoaderPOSIXDYLD::LoadModuleAtAddress(lldb_private::FileSpec const&, unsigned long, unsigned long, bools
- lldb_private::DynamicLoader::LoadModuleAtAddress(lldb_private::FileSpec const&, unsigned long, unsigned long, boos
- 83.90% lldb_private::DynamicLoader::FindModuleViaTarget(lldb_private::FileSpec const&) o
- 83.01% lldb_private::Target::GetOrCreateModule(lldb_private::ModuleSpec const&, bool, lldb_private::Status*
- 77.89% lldb_private::Module::PreloadSymbols()
- 44.06% lldb_private::Symtab::PreloadSymbols()
- 43.66% lldb_private::Symtab::InitNameIndexes()
...
```
We saw that majority of time was spent in `RefreshModules`, with the
main culprit within it `LoadModuleAtAddress` which eventually calls
`PreloadSymbols`.
At first, `DynamicLoaderPOSIXDYLD::LoadModuleAtAddress` appears fairly
independent -- most of it deals with different files and then getting or
creating Modules from these files. The portions that aren't independent
seem to deal with ModuleLists, which appear concurrency safe. There were
members of `DynamicLoaderPOSIXDYLD` I had to synchronize though: namely
`m_loaded_modules` which `DynamicLoaderPOSIXDYLD` maintains to map its
loaded modules to their link addresses. Without synchronizing this, I
ran into SEGFAULTS and other issues when running `check-lldb`. I also
locked the assignment and comparison of `m_interpreter_module`, which
may be unnecessary.
# Alternate implementations
When creating this patch, another implementation I considered was
directly background-ing the call to `Module::PreloadSymbol` in
`Target::GetOrCreateModule`. It would have the added benefit of working
across platforms generically, and appeared to be concurrency safe. It
was done via `Debugger::GetThreadPool().async` directly. However, there
were a ton of concurrency issues, so I abandoned that approach for now.
# Testing
With the feature active, I tested via `ninja check-lldb` on both Debug
and Release builds several times (~5 or 6 altogether?), and didn't spot
additional failing or flaky tests.
I also tested manually on several different binaries, some with around
14000 modules, but just basic operations: launching, reaching main,
setting breakpoint, stepping, showing some backtraces.
I've also tested with the flag off just to make sure things behave
properly synchronously.
Introduce `StopHookResult::NoPreference` and
simplify control flow in `Target::RunStopHooks()`.
The algorithm is (in order):
1. "Auto continue" set on any hook -> continue
2. "Stop demanded" by any hook -> stop
3. "Continue requested" by any hook -> continue
4. No hooks, or "no preference" only (default
stance) -> stop
The new `NoPreference` lets us keep the default
stance, distinguishing case 3. and 4.
We didn't also copy over the next stop hook id, which meant we would
overwrite the stop hooks from the dummy target with stop hooks set after
they are copied over.
Remove Debugger::GetOutputStream and Debugger::GetErrorStream in
preparation for replacing both with a new variant that needs to be
locked and hence can't be handed out like we do right now.
The patch replaces most uses with GetAsyncOutputStream and
GetAsyncErrorStream respectively. There methods return new StreamSP
objects that automatically get flushed on destruction.
See #126630 for more details.
Lots of code around LLDB was directly accessing the target's section
load list. This NFC patch makes the section load list private so the
Target class can access it, but everyone else now uses accessor
functions. This allows us to control the resolving of addresses and will
allow for functionality in LLDB which can lazily resolve addresses in
JIT plug-ins with a future patch.
Compared to the python version, this also does type checking and error
handling, so it's slightly longer, however, it's still comfortably
under 500 lines.
Relanding with more explicit type conversions.
This reverts commit f6012a209d.
Revert "[lldb] Add cast to fix compile error on 32-but platforms"
This reverts commit d300337e93.
Revert "[lldb] Improve log message to include missing strings"
This reverts commit 0be3348485.
Revert "[lldb] Add comment"
This reverts commit e2bb47443d.
Revert "[lldb] Implement a formatter bytecode interpreter in C++"
This reverts commit 9a9c1d4a61.
Compared to the python version, this also does type checking and error
handling, so it's slightly longer, however, it's still comfortably
under 500 lines.
Add support for type summaries embedded into the binary.
These embedded summaries will typically be generated by Swift macros,
but can also be generated by any other means.
rdar://115184658
Prior to this patch, the function returned an exit status, sometimes a
ValueObject with an error and a Status object. This patch removes the
Status object and ensures the error is consistently returned as the
error of the ValueObject.
"statistics dump" currently report the statistics of all targets in
debugger instead of current target. This is wrong because there is a
"statistics dump --all-targets" option that supposed to include
everything.
This PR fixes the issue by only report statistics for current target
instead of all. It also includes the change to reset statistics debug
info/symbol table parsing/indexing time during debugger destroy. This is
required so that we report current statistics if we plan to reuse
lldb/lldb-dap across debug sessions
---------
Co-authored-by: jeffreytan81 <jeffreytan@fb.com>
Add the framework code for hooking up and calling the Data Inspection
Language (DIL) implementation, as an alternate implementation for the
'frame variable' command. For now, this is an opt-in option, via a
target setting 'target.experimental.use-DIL'. See
https://discourse.llvm.org/t/rfc-data-inspection-language/69893 for more
information about this project.
This PR does not actually call any of the DIL code; instead the piece
that will eventually call the DIL code
(StackFrame::DILEvaluateVariableExpression) calls back into the original
'frame variable' implementation.
Add the ability to override the disassembly CPU and CPU features through
a target setting (`target.disassembly-cpu` and
`target.disassembly-features`) and a `disassemble` command option
(`--cpu` and `--features`).
This is especially relevant for architectures like RISC-V which relies
heavily on CPU extensions.
The majority of this patch is plumbing the options through. I recommend
looking at DisassemblerLLVMC and the test for the observable change in
behavior.
Internally we use bazel in a way in which it can drop you in a LLDB
session with the target launched in a particular cwd, which is needed
for things to work. We've been making this automation work via `process
launch -w`. However, if later the user wants to restart the process with
`r`, then they end up using a different cwd for relaunching the process.
As a way to fix this, I'm adding a target-level setting that allows
configuring a default cwd used for launching the process without needing
the user to specify it manually.
Create dependent modules in parallel in Target::SetExecutableModule.
This change was inspired by #110646 which takes the same approach when
attaching. Jason suggested we could use the same approach when you
create a target in LLDB.
I used Slack for benchmarking, which loads 902 images.
```
Benchmark 1: ./bin/lldb /Applications/Slack.app/Contents/MacOS/Slack
Time (mean ± σ): 1.225 s ± 0.003 s [User: 3.977 s, System: 1.521 s]
Range (min … max): 1.220 s … 1.229 s 10 runs
Benchmark 2: ./bin/lldb /Applications/Slack.app/Contents/MacOS/Slack
Time (mean ± σ): 3.253 s ± 0.037 s [User: 3.013 s, System: 0.248 s]
Range (min … max): 3.211 s … 3.310 s 10 runs
```
We see about a 2x speedup, which matches what Jason saw for the attach
scenario. I also ran this under TSan to confirm this doesn't introduce
any races or deadlocks.
Create dependent modules in parallel in Target::SetExecutableModule.
This change was inspired by #110646 which takes the same approach when
attaching. Jason suggested we could use the same approach when you
create a target in LLDB.
I used Slack for benchmarking, which loads 902 images.
```
Benchmark 1: ./bin/lldb /Applications/Slack.app/Contents/MacOS/Slack
Time (mean ± σ): 1.225 s ± 0.003 s [User: 3.977 s, System: 1.521 s]
Range (min … max): 1.220 s … 1.229 s 10 runs
Benchmark 2: ./bin/lldb /Applications/Slack.app/Contents/MacOS/Slack
Time (mean ± σ): 3.253 s ± 0.037 s [User: 3.013 s, System: 0.248 s]
Range (min … max): 3.211 s … 3.310 s 10 runs
```
We see about a 2x speedup, which matches what Jason saw for the attach
scenario. I also ran this under TSan to confirm this doesn't introduce
any races or deadlocks.
ValueObject is part of lldbCore for historical reasons, but conceptually
it deserves to be its own library. This does introduce a (link-time) circular
dependency between lldbCore and lldbValueObject, which is unfortunate
but probably unavoidable because so many things in LLDB rely on
ValueObject. We already have cycles and these libraries are never built
as dylibs so while this doesn't improve the situation, it also doesn't
make things worse.
The header includes were updated with the following command:
```
find . -type f -exec sed -i.bak "s%include \"lldb/Core/ValueObject%include \"lldb/ValueObject/ValueObject%" '{}' \;
```
…NFC]
This patch is the first patch in a series reworking of Pete Lawrence's
(@PortalPete) amazing proposal for better expression evaluator error
messages (https://github.com/llvm/llvm-project/pull/80938)
This patch is preparatory patch for improving the rendering of
expression evaluator diagnostics. Currently diagnostics are rendered
into a string and the command interpreter layer then textually parses
words like "error:" to (sometimes) color the output accordingly. In
order to enable user interfaces to do better with diagnostics, we need
to store them in a machine-readable fromat. This patch does this by
adding a new llvm::Error kind wrapping a DiagnosticDetail struct that
is used when the error type is eErrorTypeExpression. Multiple
diagnostics are modeled using llvm::ErrorList.
Right now the extra information is not used by the CommandInterpreter,
this will be added in a follow-up patch!
...and "[lldb/Interpreter] Introduce `ScriptedStopHook{,Python}Interface` & make use of it (#105449)"
This reverts commit 76b827bb4d, and commit 1e131ddfa8
because the first commit caused the test command-stop-hook-output.test to fail.
This patch introduces new `ScriptedStopHook{,Python}Interface` classes
that make use of the Scripted Interface infrastructure and makes use of
it in `StopHookScripted`.
It also relax the requirement on the number of argument for initializing
scripting extension if the size of the interface parameter pack contains
1 less element than the extension maximum number of positional arguments
for this initializer.
This addresses the cases where the embedded interpreter session
dictionary is passed to the extension initializer which is not used most
of the time.
---------
Signed-off-by: Med Ismail Bennani <ismail@bennani.ma>
As specified in the docs,
1) raw_string_ostream is always unbuffered and
2) the underlying buffer may be used directly
( 65b13610a5 for further reference )
* Don't call raw_string_ostream::flush(), which is essentially a no-op.
* Avoid unneeded calls to raw_string_ostream::str(), to avoid excess
indirection.
This PR adds a statistics provider cache, which allows an individual
target to keep a rolling tally of it's total time and number of
invocations for a given summary provider. This information is then
available in statistics dump to help slow summary providers, and gleam
more into insight into LLDB's time use.
This patch removes all of the Set.* methods from Status.
This cleanup is part of a series of patches that make it harder use the
anti-pattern of keeping a long-lives Status object around and updating
it while dropping any errors it contains on the floor.
This patch is largely NFC, the more interesting next steps this enables
is to:
1. remove Status.Clear()
2. assert that Status::operator=() never overwrites an error
3. remove Status::operator=()
Note that step (2) will bring 90% of the benefits for users, and step
(3) will dramatically clean up the error handling code in various
places. In the end my goal is to convert all APIs that are of the form
` ResultTy DoFoo(Status& error)
`
to
` llvm::Expected<ResultTy> DoFoo()
`
How to read this patch?
The interesting changes are in Status.h and Status.cpp, all other
changes are mostly
` perl -pi -e 's/\.SetErrorString/ = Status::FromErrorString/g' $(git
grep -l SetErrorString lldb/source)
`
plus the occasional manual cleanup.
TargetProperties.td had a few settings listed as signed integral values,
but the Target.cpp methods reading those values were reading them as
unsigned. e.g. target.max-memory-read-size, some accesses of
target.max-children-count, still today, previously
target.max-string-summary-length.
After Jonas' change to use templates to read these values in
https://reviews.llvm.org/D149774, when the code tried to fetch these
values, we'd eventually end up calling OptionValue::GetAsUInt64 which
checks that the value is actually a UInt64 before returning it; finding
that it was an SInt64, it would drop the user setting and return the
default value. This manifested as a bug that target.max-memory-read-size
is never used for memory read.
target.max-children-count is less straightforward, where one read of
that setting was fetching it as an int64_t, the other as a uint64_t.
I suspect all of these settings were originally marked as SInt64 so a
user could do -1 for "infinite", getting it static_cast to a UINT64_MAX
value along the way. I can't find any documentation for this behavior,
but it seems like something Greg would have done. We've partially lost
that behavior already via
https://github.com/llvm/llvm-project/pull/72233 for
target.max-string-summary-length, and this further removes it.
We're still fetching UInt64's and returning them as uint32_t's but I'm
not overly pressed about someone setting a count/size limit over 4GB.
I added a simple API test for the memory read setting limit.
This introduces a `target.object-map` which allows us to remap module
locations, much in the same way as source mapping works today. This is
useful, for instance, when debugging coredumps, so we can replace some
of the locations where LLDB attempts to load shared libraries and
executables from, without having to setup an entire sysroot.
For the significant amount of call sites that want to create an
incontrovertible error, such a wrapper function creates a significant
readability improvement and lowers the cost of entry to add error
handling in more places.
