| Age | Commit message (Collapse) | Author |
|---|
|
In C++17, static constexpr members are implicitly inline, so they no
longer require an out-of-line definition.
Identified with readability-redundant-declaration.
|
|
This patch fixes and eliminates the possibility of SupportFileSP ever
being nullptr. The support file was originally treated like a value
type, but became a polymorphic type and therefore has to be stored and
passed around as a pointer.
To avoid having all the callers check the validity of the pointer, I
introduced the invariant that SupportFileSP is never null and always
default constructed. However, without enforcement at the type level,
that's fragile and indeed, we already identified two crashes where
someone accidentally broke that invariant.
This PR introduces a NonNullSharedPtr to prevent that. NonNullSharedPtr
is a smart pointer wrapper around std::shared_ptr that guarantees the
pointer is never null. If default-constructed, it creates a
default-constructed instance of the contained type. Note that I'm using
private inheritance because you shouldn't inherit from standard library
classes due to the lack of virtual destructor. So while the new
abstraction looks like a `std::shared_ptr`, it is in fact **not** a
shared pointer. Given that our destructor is trivial, we could use
public inheritance, but currently there's no need for it.
rdar://164989579
|
|
Another attempt at resolving the deadlock issue @GeorgeHuyubo discovered
(his previous
[attempt](https://github.com/llvm/llvm-project/pull/160225)).
This change can be summarized as the following:
* Plumb through a boolean flag to force no preload in
`GetOrCreateModules` all the way through to `LoadModuleAtAddress`.
* Parallelize `Module::PreloadSymbols` separately from
`Target::GetOrCreateModule` and its caller `LoadModuleAtAddress` (this
is what avoids the deadlock).
These changes roughly maintain the performance characteristics of the
previous implementation of parallel module loading. Testing on targets
with between 5000 and 14000 modules, I saw similar numbers as before,
often more than 10% faster in the new implementation across multiple
trials for these massive targets. I think it's because we have less lock
contention with this approach.
# The deadlock
See [bt.txt](https://github.com/user-attachments/files/22524471/bt.txt)
for a sample backtrace of LLDB when the deadlock occurs.
As @GeorgeHuyubo explains in his
[PR](https://github.com/llvm/llvm-project/pull/160225), the deadlock
occurs from an ABBA deadlock that happens when a thread context-switches
out of `Module::PreloadSymbols`, goes into `Target::GetOrCreateModule`
for another module, possibly entering this block:
```
if (!module_sp) {
// The platform is responsible for finding and caching an appropriate
// module in the shared module cache.
if (m_platform_sp) {
error = m_platform_sp->GetSharedModule(
module_spec, m_process_sp.get(), module_sp, &search_paths,
&old_modules, &did_create_module);
} else {
error = Status::FromErrorString("no platform is currently set");
}
}
```
`Module::PreloadSymbols` holds a module-level mutex, and then
`GetSharedModule` *attempts* to hold the mutex of the global shared
`ModuleList`. So, this thread holds the module mutex, and waits on the
global shared `ModuleList` mutex.
A competing thread may execute `Target::GetOrCreateModule`, enter the
same block as above, grabbing the global shared `ModuleList` mutex.
Then, in `ModuleList::GetSharedModule`, we eventually call
`ModuleList::FindModules` which eventually waits for the `Module` mutex
held by the first thread (via `Module::GetUUID`). Thus, we deadlock.
## Reproducing the deadlock
It might be worth noting that I've never been able to observe this
deadlock issue during live debugging (e.g. launching or attaching to
processes), however we were able to consistently reproduce this issue
with coredumps when using the following settings:
```
(lldb) settings set target.parallel-module-load true
(lldb) settings set target.preload-symbols true
(lldb) settings set symbols.load-on-demand false
(lldb) target create --core /some/core/file/here
# deadlock happens
```
## How this change avoids this deadlock
This change avoids concurrent executions of `Module::PreloadSymbols`
with `Target::GetOrCreateModule` by waiting until after the
`Target::GetOrCreateModule` executions to run `Module::PreloadSymbols`
in parallel. This avoids the ordering of holding a Module lock *then*
the ModuleList lock, as `Target::GetOrCreateModule` executions maintain
the ordering of the shared ModuleList lock first (from what I've read
and tested).
## Why not read-write lock?
Some feedback in https://github.com/llvm/llvm-project/pull/160225 was to
modify mutexes used in these components with read-write locks. This
might be a good idea overall, but I don't think it would *easily*
resolve this specific deadlock. `Module::PreloadSymbols` would probably
need a write lock to Module, so even if we had a read lock in
`Module::GetUUID` we would still contend. Maybe the `ModuleList` lock
could be a read lock that converts to a write lock if it chooses to
update the module, but it seems likely that some thread would try to
update the shared module list and then the write lock would contend
again.
Perhaps with deeper architectural changes, we could fix this issue?
# Other attempts
One downside of this approach (and the former approach of parallel
module loading) is that each DYLD would need to implement this pattern
themselves. With @clayborg's help, I looked at a few other approaches:
* In `Target::GetOrCreateModule`, backgrounding the
`Module::PreloadSymbols` call by adding it directly to the thread pool
via `Debugger::GetThreadPool().async()`. This required adding a lock to
`Module::SetLoadAddress` (probably should be one there already) since
`ObjectFileELF::SetLoadAddress` is not thread-safe (updates sections).
Unfortunately, during execution, this causes the preload symbols to run
synchronously with `Target::GetOrCreateModule`, preventing us from truly
parallelizing the execution.
* In `Module::PreloadSymbols`, backgrounding the `symtab` and `sym_file`
`PreloadSymbols` calls individually, but similar issues as the above.
* Passing a callback function like
https://github.com/swiftlang/llvm-project/pull/10746 instead of the
boolean I use in this change. It's functionally the same change IMO,
with some design tradeoffs:
* Pro: the caller doesn't need to explicitly call
`Module::PreloadSymbols` itself, and can instead call whatever function
is passed into the callback.
* Con: the caller needs to delay the execution of the callback such that
it occurs after the `GetOrCreateModule` logic, otherwise we run into the
same issue. I thought this would be trickier for the caller, requiring
some kinda condition variable or otherwise storing the calls to execute
afterwards.
# Test Plan:
```
ninja check-lldb
```
---------
Co-authored-by: Tom Yang <toyang@fb.com>
|
|
The new test fails on x86 and arm64 public macOS bots:
```
09:27:59 ======================================================================
09:27:59 FAIL: test_append_frames (TestScriptedFrameProvider.ScriptedFrameProviderTestCase)
09:27:59 Test that we can add frames after real stack.
09:27:59 ----------------------------------------------------------------------
09:27:59 Traceback (most recent call last):
09:27:59 File "/Users/ec2-user/jenkins/workspace/llvm.org/as-lldb-cmake/llvm-project/lldb/test/API/functionalities/scripted_frame_provider/TestScriptedFrameProvider.py", line 122, in test_append_frames
09:27:59 self.assertEqual(new_frame_count, original_frame_count + 1)
09:27:59 AssertionError: 5 != 6
09:27:59 Config=arm64-/Users/ec2-user/jenkins/workspace/llvm.org/as-lldb-cmake/lldb-build/bin/clang
09:27:59 ======================================================================
09:27:59 FAIL: test_applies_to_thread (TestScriptedFrameProvider.ScriptedFrameProviderTestCase)
09:27:59 Test that applies_to_thread filters which threads get the provider.
09:27:59 ----------------------------------------------------------------------
09:27:59 Traceback (most recent call last):
09:27:59 File "/Users/ec2-user/jenkins/workspace/llvm.org/as-lldb-cmake/llvm-project/lldb/test/API/functionalities/scripted_frame_provider/TestScriptedFrameProvider.py", line 218, in test_applies_to_thread
09:27:59 self.assertEqual(
09:27:59 AssertionError: 5 != 1 : Thread with ID 1 should have 1 synthetic frame
09:27:59 Config=arm64-/Users/ec2-user/jenkins/workspace/llvm.org/as-lldb-cmake/lldb-build/bin/clang
09:27:59 ======================================================================
09:27:59 FAIL: test_prepend_frames (TestScriptedFrameProvider.ScriptedFrameProviderTestCase)
09:27:59 Test that we can add frames before real stack.
09:27:59 ----------------------------------------------------------------------
09:27:59 Traceback (most recent call last):
09:27:59 File "/Users/ec2-user/jenkins/workspace/llvm.org/as-lldb-cmake/llvm-project/lldb/test/API/functionalities/scripted_frame_provider/TestScriptedFrameProvider.py", line 84, in test_prepend_frames
09:27:59 self.assertEqual(new_frame_count, original_frame_count + 2)
09:27:59 AssertionError: 5 != 7
09:27:59 Config=arm64-/Users/ec2-user/jenkins/workspace/llvm.org/as-lldb-cmake/lldb-build/bin/clang
09:27:59 ======================================================================
09:27:59 FAIL: test_remove_frame_provider_by_id (TestScriptedFrameProvider.ScriptedFrameProviderTestCase)
09:27:59 Test that RemoveScriptedFrameProvider removes a specific provider by ID.
09:27:59 ----------------------------------------------------------------------
09:27:59 Traceback (most recent call last):
09:27:59 File "/Users/ec2-user/jenkins/workspace/llvm.org/as-lldb-cmake/llvm-project/lldb/test/API/functionalities/scripted_frame_provider/TestScriptedFrameProvider.py", line 272, in test_remove_frame_provider_by_id
09:27:59 self.assertEqual(thread.GetNumFrames(), 3, "Should have 3 synthetic frames")
09:27:59 AssertionError: 5 != 3 : Should have 3 synthetic frames
09:27:59 Config=arm64-/Users/ec2-user/jenkins/workspace/llvm.org/as-lldb-cmake/lldb-build/bin/clang
09:27:59 ======================================================================
09:27:59 FAIL: test_replace_all_frames (TestScriptedFrameProvider.ScriptedFrameProviderTestCase)
09:27:59 Test that we can replace the entire stack.
09:27:59 ----------------------------------------------------------------------
09:27:59 Traceback (most recent call last):
09:27:59 File "/Users/ec2-user/jenkins/workspace/llvm.org/as-lldb-cmake/llvm-project/lldb/test/API/functionalities/scripted_frame_provider/TestScriptedFrameProvider.py", line 41, in test_replace_all_frames
09:27:59 self.assertEqual(thread.GetNumFrames(), 3, "Should have 3 synthetic frames")
09:27:59 AssertionError: 5 != 3 : Should have 3 synthetic frames
09:27:59 Config=arm64-/Users/ec2-user/jenkins/workspace/llvm.org/as-lldb-cmake/lldb-build/bin/clang
09:27:59 ======================================================================
09:27:59 FAIL: test_scripted_frame_objects (TestScriptedFrameProvider.ScriptedFrameProviderTestCase)
09:27:59 Test that provider can return ScriptedFrame objects.
09:27:59 ----------------------------------------------------------------------
09:27:59 Traceback (most recent call last):
09:27:59 File "/Users/ec2-user/jenkins/workspace/llvm.org/as-lldb-cmake/llvm-project/lldb/test/API/functionalities/scripted_frame_provider/TestScriptedFrameProvider.py", line 159, in test_scripted_frame_objects
09:27:59 self.assertEqual(frame0.GetFunctionName(), "custom_scripted_frame_0")
09:27:59 AssertionError: 'thread_func(int)' != 'custom_scripted_frame_0'
09:27:59 - thread_func(int)
09:27:59 + custom_scripted_frame_0
09:27:59
09:27:59 Config=arm64-/Users/ec2-user/jenkins/workspace/llvm.org/as-lldb-cmake/lldb-build/bin/clang
09:27:59 ----------------------------------------------------------------------
09:27:59 Ran 6 tests in 14.242s
09:27:59
09:27:59 FAILED (failures=6)
```
Reverts llvm/llvm-project#161870
|
|
This patch extends ScriptedFrame to work with real (non-scripted)
threads,
enabling frame providers to synthesize frames for native processes.
Previously, ScriptedFrame only worked within
ScriptedProcess/ScriptedThread
contexts. This patch decouples ScriptedFrame from ScriptedThread,
allowing
users to augment or replace stack frames in real debugging sessions for
use
cases like custom calling conventions, reconstructing corrupted frames
from
core files, or adding diagnostic frames.
Key changes:
- ScriptedFrame::Create() now accepts ThreadSP instead of requiring
ScriptedThread, extracting architecture from the target triple rather
than ScriptedProcess.arch
- Added SBTarget::RegisterScriptedFrameProvider() and
ClearScriptedFrameProvider() APIs, with Target storing a
SyntheticFrameProviderDescriptor template for new threads
- Added "target frame-provider register/clear" commands for CLI access
- Thread class gains LoadScriptedFrameProvider(),
ClearScriptedFrameProvider(),
and GetFrameProvider() methods for per-thread frame provider management
- New SyntheticStackFrameList overrides FetchFramesUpTo() to lazily
provide
frames from either the frame provider or the real stack
This enables practical use of the SyntheticFrameProvider infrastructure
in
real debugging workflows.
rdar://161834688
Signed-off-by: Med Ismail Bennani <ismail@bennani.ma>
Signed-off-by: Med Ismail Bennani <ismail@bennani.ma>
|
|
We're iterating over the stop hooks so if one of them changes the stop
hook list by deleting itself or another stop hook, that invalidates our
iterator.
I chose to fix this by making a copy of the stop hook list and iterating
over that. That's a cheap operation since this is just an array of
shared pointers. But more importantly doing it this way means that if on
a stop where one stop hook deletes another, the deleted hook will always
get a chance to run. If you iterated over the list itself, then whether
that to be deleted hook gets to run would be dependent on whether it was
before or after the deleting stop hook, which would be confusing.
|
|
(#166869)
We weren't setting `m_should_detach` when going through the
`DoConnectRemote` code path. This meant that when you would attaches to
a remote process with `gdb-remote <port>` and use Ctrl+D, it would kill
the process instead of detach from it.
rdar://156111423
|
|
Main executables were bypassing the locate module callback that shared
libraries use, preventing custom symbol file location logic from working
consistently.
This PR fix this by
* Adding target context to ModuleSpec
* Leveraging that context to use target search path and platform's
locate module callback in ModuleList::GetSharedModule
This ensures both main executables and shared libraries get the same
callback treatment for symbol file resolution.
---------
Co-authored-by: George Hu <hyubo@meta.com>
Co-authored-by: George Hu <georgehuyubo@gmail.com>
|
|
This patch introduces a new way to reconstruct the thread stackframe
list.
New `SyntheticFrameProvider` classes can lazy fetch a StackFrame at
index using a provided StackFrameList.
In can either be the real unwinder StackFrameList or we could also chain
SyntheticFrameProviders to each others.
This is the foundation work to implement ScriptedFrameProviders, which
will come in a follow-up patch.
Signed-off-by: Med Ismail Bennani <ismail@bennani.ma>
Signed-off-by: Med Ismail Bennani <ismail@bennani.ma>
|
|
This avoids unintentional comparisons between `SourceLanguage` and
`LanguageType`.
Also marks `operator bool` explicit so we don't implicitly convert to
bool.
|
|
(#166157)
https://github.com/llvm/llvm-project/pull/165996 is adding a Clang
dependency to Target because we're moving some of the functionality of
the VerboseTrapFrameRecognizer into libClang. To avoid adding this
dependency this patch moves VerboseTrapFrameRecognizer into the
CPPLanguageRuntime. Most of the frame recognizers already live in the
various runtime plugins.
An alternative discussed was to create a common `CLanguageRuntime` whose
currently sole responsibility was to register the
`VerboseTrapFrameRecognizer` and `AssertStackFrameRecognizer`. The main
issue I ran into here was frame recognizers aren't uniqued in the
target. Currently this only manifests when re-running a target, which
re-triggers all the recognizer registration (added a test with a FIXME
for this). If we had a common `CLanguageRuntime` that
`CPPLanguageRuntime` and `ObjCLanguageRuntime` inherited from, I didn't
find a great way to avoid registering the recognizer multiple times. We
can't just call_once on it because we do want the recognisers to be
re-registered for new targets in the same debugger session. If the
recognisers were stored in something like a UniqueVector in the Target,
then we wouldn't have that issue. But currently that's not the case, and
it would take a bit of refactoring to de-dupe the recognisers.
There may very well be solutions I haven't considered, but all the
things I've tried so far I wasn't very happy with. So in the end I just
moved this to the C++ runtime for now in order to unblock
https://github.com/llvm/llvm-project/pull/165996.
The C++ language runtime is always available (even for C targets) if the
C++ language plugin is available. Which it should also be unless someone
is using an LLDB with the C++ plugin compiled out. But at that point
numerous things wouldn't work when even debugging just C.
|
|
(#166001)
This adds a virtual method that allows `InstrumentationRuntime` sub
classes to match against all modules rather than just a library that
matches a particular regex. When the implementation returns true
`GetPatternForRuntimeLibrary()` is ignored and all modules are iterated
over. The default implementation returns false which was the previous
behavior which uses `GetPatternForRuntimeLibrary()` to only match a
particular runtime library.
The intended use case here is for implementing an
`InstrumentationRuntime` where the runtime library of interest can have
multiple implementations and whose name is not known ahead of time.
The concrete use case here is for a `InstrumentationRuntime` plugin for
implementations of the `-fbounds-safety` soft-trap runtime which can
have multiple different implementations and so the module containing the
runtime functions isn't known ahead of time. This plug-in will be
upstreamed as part of the process of upstreaming `-fbounds-safety`.
An alternative to this would be for the `GetPatternForRuntimeLibrary()`
function to return a regex that matches everything. While that
technically works this new API more clearly indicates in the intent. We
probably also save a little perf by not executing the regex match for
every loaded module but I have not measured this.
rdar://163230807
|
|
This commit aggregates the following changes:
1. Fix the format (i.e., indentation) when printing stop hooks via `target
stop-hook list`.
2. Add `IndentScope Stream::MakeIndentScope()` to make managing (and restoring!)
of the indentation level on `Stream` instances more ergonomic and less error
prone.
3. Simplify printing of stop hooks using the new `IndentScope`.
|
|
eliminate code duplication (#164739)
### Summary
This PR refactors breakpoint event notification in LLDB to centralize
and eliminate code duplication. It creates a unified method in the
`Target` class for sending breakpoint change events. The new methods
check if listeners exist before broadcasting events
### Test
<img width="1532" height="76" alt="Screenshot 2025-10-23 at 12 49 31 PM"
src="https://github.com/user-attachments/assets/6d6a6da6-9684-463c-aeeb-90663cdbd077"
/>
---------
Co-authored-by: Piyush Jaiswal <piyushjais@meta.com>
|
|
Introduce the concept of internal stop hooks.
These are similar to LLDB's internal breakpoints:
LLDB itself will add them and users of LLDB will
not be able to add or remove them.
This change adds the following 3
independently-useful concepts:
* Maintain a list of internal stop hooks that will be populated by LLDB
and cannot be added to or removed from by users. They are managed in a
separate list in `Target::m_internal_stop_hooks`.
* `StopHookKind:CodeBased` and `StopHookCoded` represent a stop hook
defined by a C++ code callback (instead of command line expressions or a
Python class).
* Stop hooks that do not print any output can now also suppress the
printing of their header and description when they are hit via
`StopHook::GetSuppressOutput`.
Combining these 3 concepts we can model "internal
stop hooks" which serve the same function as
LLDB's internal breakpoints: executing built-in,
LLDB-defined behavior, leveraging the existing
mechanism of stop hooks.
This change also simplifies
`Target::RunStopHooks`. We already have to
materialize a new list for combining internal and
user stop hooks. Filter and only add active hooks to this list to avoid
the need for "isActive?"
checks later on.
|
|
This commit introduces a base-class implementation for a method that
reads memory from multiple ranges at once. This implementation simply
calls the underlying `ReadMemoryFromInferior` method on each requested
range, intentionally bypassing the memory caching mechanism (though this
may be easily changed in the future).
`Process` implementations that can be perform this operation more
efficiently - e.g. with the MultiMemPacket described in [1] - are
expected to override this method.
As an example, this commit changes AppleObjCClassDescriptorV2 to use the
new API.
Note about the API
------------------
In the RFC, we discussed having the API return some kind of class
`ReadMemoryRangesResult`. However, while writing such a class, it became
clear that it was merely wrapping a vector, without providing anything
useful. For example, this class:
```
struct ReadMemoryRangesResult {
ReadMemoryRangesResult(
llvm::SmallVector<llvm::MutableArrayRef<uint8_t>> ranges)
: ranges(std::move(ranges)) {}
llvm::ArrayRef<llvm::MutableArrayRef<uint8_t>> getRanges() const {
return ranges;
}
private:
llvm::SmallVector<llvm::MutableArrayRef<uint8_t>> ranges;
};
```
As can be seen in the added test and in the added use-case
(AppleObjCClassDescriptorV2), users of this API will just iterate over
the vector of memory buffers. So they want a return type that can be
iterated over, and the vector seems more natural than creating a new
class and defining iterators for it.
Likewise, in the RFC, we discussed wrapping the result into an
`Expected`. Upon experimenting with the code, this feels like it limits
what the API is able to do as the base class implementation never needs
to fail the entire result, it's the individual reads that may fail and
this is expressed through a zero-length result. Any derived classes
overriding `ReadMemoryRanges` should also never produce a top level
failure: if they did, they can just fall back to the base class
implementation, which would produce a better result.
The choice of having the caller allocate a buffer and pass it to
`Process::ReadMemoryRanges` is done mostly to follow conventions already
done in the Process class.
[1]:
https://discourse.llvm.org/t/rfc-a-new-vectorized-memory-read-packet/
|
|
StopInfoBreakpoint keeps a BreakpointLocationCollection for all the
breakpoint locations at the BreakpointSite that was hit. It is also
lives through the time a given thread is stopped, so there are plenty of
opportunities for one of the owning breakpoints to get deleted.
But BreakpointLocations don't keep their owner Breakpoints alive, so if
the BreakpointLocationCollection can live past when some code gets a
chance to delete an owner breakpoint, and then you ask that location for
some breakpoint information, it will access freed memory.
This wasn't a problem before PR #158128 because the StopInfoBreakpoint
just kept the BreakpointSite that was hit, and when you asked it
questions, it relooked up that list. That was not great, however,
because if you hit breakpoints 5 & 6, deleted 5 and then asked which
breakpoints got hit, you would just get 6. For that and other reasons
that PR changed to storing a BreakpointLocationCollection of the
breakpoints that were hit. That's better from a UI perspective but
caused this potential problem.
I fix it by adding a variant of the BreakpointLocationCollection that
also holds onto a shared pointer to the Breakpoints that own the
locations that were hit, thus keeping them alive till the
StopInfoBreakpoint goes away.
This fixed the ASAN assertion. I also added a test that works harder to
cause trouble by deleting breakpoints during a stop.
|
|
Specially when dealing with different address spaces, CFAs could start
from addresses like 0. For instance, Nvidia GPUs have instructions to
read from local memory that use 0-based offsets and stack memory can be
referenced by these offsets rather than global addresses. Note that ABIs
could already specify what they consider to be valid CFA values but this
was never used in these parts of the unwinder code. For most ABIs, this
makes the validation more strict, as they already used to discard 0s and
then checked for alignment which would discard 1s. There a few
exceptions where 0s were possible and this makes it less strict, like
the RISCV and ARC ABIs.
@jasonmolenda Would you be the appropriate reviewer for this?
Also cc. @clayborg @walter-erquinigo
|
|
This patch adds the notion of "Facade" locations which can be reported
from a ScriptedResolver instead of the actual underlying breakpoint
location for the breakpoint. Also add a "was_hit" method to the scripted
resolver that allows the breakpoint to say which of these "Facade"
locations was hit, and "get_location_description" to provide a
description for the facade locations.
I apologize in advance for the size of the patch. Almost all of what's
here was necessary to (a) make the feature testable and (b) not break
any of the current behavior.
The motivation for this feature is given in the "Providing Facade
Locations" section that I added to the python-reference.rst so I won't
repeat it here.
rdar://152112327
|
|
`structured-data ommand` -> `structured-data command`
|
|
(#161982)
When a thread reaches a breakpoint at the return address set by
`ThreadPlanStepOut`, `ThreadPlanStepOut::ShouldStop()` calls
`ThreadPlanShouldStopHere::InvokeShouldStopHereCallback()`, and if it
returns `false`, `ThreadPlanShouldStopHere::QueueStepOutFromHerePlan()`
is called to queue a new plan to skip the corresponding range. Once the
new plan finishes, `ThreadPlanStepOut::ShouldStop()` should recheck the
stop condition; however, there is no code path in the method that sets
`done` to `true`. Before #126838, if `done` was `false`, the method checked
if a suitable frame had been reached. After the patch, the check is only
performed at a breakpoint; thus, the execution continues.
This patch causes `ThreadPlanStepOut::ShouldStop()` to recheck the stop
condition when `m_step_out_further_plan_sp` completes.
|
|
(#162050)
Depends on https://github.com/llvm/llvm-project/pull/162048
This makes sure we also include the version number in the description.
For `C++17`, this would, e.g., now return `"C++17"` instead of `"ISO
C++"`.
|
|
This fixes a potential use after free where
ModuleList::RemoveSharedModuleIfOrphaned ->
SharedModuleList::RemoveIfOrphaned -> SharedModuleList::RemoveFromMap
would potentially dereference a freed pointer. This fixes it by not
calling ModuleList::RemoveSharedModuleIfOrphaned at all if the pointer
was just freed.
|
|
The ToDwarfSourceLanguage function incorrectly excluded languages that
equal eLanguageTypeLastStandardLanguage. The comparison used `<` instead
of `<=`, causing the last standard language to fall through to the
default case and return std::nullopt.
This broke language plugins that use eLanguageTypeLastStandardLanguage
(currently Mojo at 0x0033) as their language code, preventing proper
DWARF language conversion and breaking REPL functionality.
The fix changes the comparison from `<` to `<=` to include the last
standard language in the automatic conversion to DWARF source language.
This is a regression from commit
7f51a2a47d2e706d04855b0e41690ebafa2b3238 which introduced the
ToDwarfSourceLanguage function.
|
|
### Summary
Add support for unique target ids per Target instance. This is needed
for upcoming changes to allow debugger instances to be shared across
separate DAP instances for child process debugging. We want the IDE to
be able to attach to existing targets in an already runny lldb-dap
session, and having a unique ID per target would make that easier.
Each Target instance will have its own unique id, and uses a
function-local counter in `TargetList::CreateTargetInternal` to assign
incremental unique ids.
### Tests
Added several unit tests to test basic functionality, uniqueness of
targets, and target deletion doesn't affect the uniqueness.
```
bin/lldb-dotest -p TestDebuggerAPI
```
|
|
The intention for this API is to be used when presenting language names
to the user, e.g., in expression evaluation diagnostics or LLDB errors.
Most uses of `GetNameForLanguageType` can be probably replaced with
`GetDisplayNameForLanguageType`, but that's out of scope of this PR.
This uses `llvm::dwarf::LanguageDescription` under the hood, so we would
lose the version numbers in the names. If we deem those to be important,
we could switch to an implementation that hardcodes a list of
user-friendly names with version numbers included.
The intention is to use it from
https://github.com/llvm/llvm-project/pull/161688
Depends on https://github.com/llvm/llvm-project/pull/161804
|
|
Currently we don't benefit from the user-friendly names that
`LanguageDescription` returns because we would always use
`Language::GetNameForLanguageType`. I'm not aware of a situation where
`GetDescription` should prefer the non-human readable form of the name
with. This patch removes the call to `GetNameForLanguageType`.
`LanguageDescription` already handles languages that it doesn't know
about. For those it would return `Unknown`. The LLDB language types
should all be available via DWARF. If there are languages that don't map
cleanly between `lldb::LanguageType` and `DW_LANG`, then we should add
explicit support for that in the `SourceLanguage::SourceLanguage`
constructor.
|
|
|
|
This patch adds a load core time, right now we don't have much insight
into the performance of load core, especially for large coredumps. To
start collecting information on this I've added some minor
instrumentation code to measure the two call sites of `LoadCore`.
I've also added a test to validate the new metric is output in
statistics dump
|
|
Currently, we always pass the "selected" execution context to the
statusline. When handling a process or thread event, we can be more
precise, and build an execution context from the event data. This PR
also adopts the new `StoppedExecutionContext` that was recently
introduced.
|
|
Based on testing on processors that use pointer metadata, and with all
the work done to delay calls to FixDataAddress, this is no longer
necessary.
Note that, with debugserver in particular, this is an NFC change: the
code path here is for frame zero, and debugserver will strip metadata
when reading fp from frame zero anyway.
|
|
The first call, in InitializeNonZerothFrame, is inside a logging branch.
For that, it's better to log the real value instead of the fixed one.
The second call, inside RegisterContextUnwind::ReadFrameAddress, is
computing an address which is then passed to
ReadRegisterValueFromMemory, which boils down to a Process::ReadMemory,
which fixes the address if it wants to. The current variable names are
misleading, making readers believe it is the cfa value itself that is
being passed to Fix*Address; that's not the case. This commit renames
the variable to make this abundantly clear.
|
|
[lldb] Track CFA pointer metadata in StackID
In this commit:
9c8e71644227 [lldb] Make StackID call Fix{Code,Data} pointers (#152796)
We made StackID keep track of the CFA without any pointer metadata in
it. This is necessary when comparing two StackIDs to determine which one
is "younger".
However, the CFA inside StackIDs is also used in other contexts through
the method StackID::GetCallFrameAddress. One notable case is
DWARFExpression: the computation of `DW_OP_call_frame_address` is done
using StackID. This feeds into many other places, e.g. expression
evaluation may require the address of a variable that is computed from
the CFA; to access the variable without faulting, we may need to
preserve the pointer metadata. As such, StackID must be able to provide
both versions of the CFA.
In the spirit of allowing consumers of pointers to decide what to do
with pointer metadata, this patch changes StackID to store both versions
of the cfa pointer. Two getter methods are provided, and all call sites
except DWARFExpression preserve their existing behavior (stripped
pointer). Other alternatives were considered:
* Just store the raw pointer. This would require changing the
comparisong operator `<` to also receive a Process, as the comparison
requires stripped pointers. It wasn't clear if all call-sites had a
non-null process, whereas we know we have a process when creating a
StackID.
* Store a weak pointer to the process inside the class, and then strip
metadata as needed. This would require a `weak_ptr::lock` in many
operations of LLDB, and it felt wasteful. It also prevents stripping
of the pointer if the process has gone away.
This patch also changes RegisterContextUnwind::ReadFrameAddress, which
is the method computing the CFA fed into StackID, to also preserve the
signature pointers.
|
|
This is something the StopInfo class manages, so it should be allowed to
compute this rather than having SBThread do so. This code just moves the
computation to methods in StopInfo. It is mostly NFC. The one change
that I actually had to adjust the tests for was a couple of tests that
were asking for the UnixSignal stop info data by asking for the data at
index 1. GetStopInfoDataCount returns 1 and we don't do 1 based indexing
so the test code was clearly wrong. But I don't think it makes sense to
perpetuate handing out the value regardless of what index you pass us.
|
|
When a processor faults/is interrupted/gets an exception, it will stop
running code and jump to an exception catcher routine. Most processors
will store the pc that was executing in a system register, and the
catcher functions have special instructions to retrieve that & possibly
other registers. It may then save those values to stack, and the author
can add .cfi directives to tell lldb's unwinder where to find those
saved values.
ARM Cortex-M (microcontroller) processors have a simpler mechanism where
a fixed set of registers are saved to the stack on an exception, and a
unique value is put in the link register to indicate to the caller that
this has taken place. No special handling needs to be written into the
exception catcher, unless it wants to inspect these preserved values.
And it is possible for a general stack walker to walk the stack with no
special knowledge about what the catch function does.
This patch adds an Architecture plugin method to allow an Architecture
to override/augment the UnwindPlan that lldb would use for a stack
frame, given the contents of the return address register. It resembles a
feature where the LanguageRuntime can replace/augment the unwind plan
for a function, but it is doing it at offset by one level. The
LanguageRuntime is looking at the local register context and/or symbol
name to decide if it will override the unwind rules. For the Cortex-M
exception unwinds, we need to modify THIS frame's unwind plan if the
CALLER's LR had a specific value. RegisterContextUnwind has to retrieve
the caller's LR value before it has completely decided on the UnwindPlan
it will use for THIS stack frame.
This does mean that we will need one additional read of stack memory
than we currently do when unwinding, on Armv7 Cortex-M targets. The
unwinder walks the stack lazily, as stack frames are requested, and so
now if you ask for 2 stack frames, we will read enough stack to walk 2
frames, plus we will read one extra word of memory, the spilled LR value
from the stack. In practice, with 512-byte memory cache reads, this is
unlikely to be a real performance hit.
This PR includes a test with a yaml corefile description and a JSON
ObjectFile, incorporating all of the necessary stack memory and symbol
names from a real debug session I worked on. The architectural default
unwind plans are used for all stack frames except the 0th because
there's no instructions for the functions, and no unwind info. I may
need to add an encoding of unwind fules to ObjectFileJSON in the future
as we create more test cases like this.
This PR depends on the yaml2macho-core utility from
https://github.com/llvm/llvm-project/pull/153911 to run its API test.
rdar://110663219
|
|
DWO file count functions" (#156980)
This relands changes in https://github.com/llvm/llvm-project/pull/155023
for adding a count of dwo errors and combine all the dwo related stats
into one struct.
The previous PR was reverted in
https://github.com/llvm/llvm-project/pull/156777 as the newly added unit
test `test_dwo_id_mismatch_error_stats` sometimes fails due to
inappropriate use of `glob.glob`.
This change modified the tests created in the former PR to collect and
modify the dwo files by there names instead of using index after
`glob.glob`. This will avoid the possible failure in these tests if the
order of dwo files changes.
[Original PR:
https://github.com/llvm/llvm-project/pull/155023](https://github.com/llvm/llvm-project/pull/155023)
## Testing
Ran unit tests
```
$ ./bin/llvm-lit /data/users/ziyiww/llvm-project/lldb/test/API/commands/statistics/basic/TestStats.py
./bin/llvm-lit /data/users/ziyiww/llvm-project/lldb/test/API/commands/statistics/basic/TestStats.py -v
-- Testing: 1 tests, 1 workers --
PASS: lldb-api :: commands/statistics/basic/TestStats.py (1 of 1)
Testing Time: 388.52s
Total Discovered Tests: 1
Passed: 1 (100.00%)
$ bin/lldb-dotest -p TestStats.py /data/users/ziyiww/llvm-project/lldb/test/API/commands/statistics/basic/
----------------------------------------------------------------------
Ran 27 tests in 386.302s
OK (skipped=3)
```
|
|
(#133079)
When hitting a sanitizer breakpoint, LLDB currently displays the frame
in the sanitizer dylib (which we usually don't have debug-info for),
which isn't very helpful to the user. A more helpful frame to display
would be the first frame not in the sanitizer library (we have a
[similar heuristic when we trap inside
libc++](https://github.com/llvm/llvm-project/pull/108825)). This patch
does just that, by implementing the `GetSuggestedStackFrameIndex` API
Depends on https://github.com/llvm/llvm-project/pull/133078
|
|
(#133078)
The motivation for this patch is that
`StopInfo::GetSuggestedStackFrameIndex` would not take effect for
`InstrumentationRuntimeStopInfo` (which we plan to implement in
https://github.com/llvm/llvm-project/pull/133079). This was happening
because the instrumentation runtime plugins would run utility
expressions as part of the stop that would set the
`m_selected_frame_idx`. This means `SelectMostRelevantFrame` was never
called, and we would not be able to report the selected frame via the
`StopInfo` object.
This patch makes sure we clear the `m_selected_frame_idx` to allow
`GetSuggestedStackFrameIndex` to take effect, regardless of what the
frame recognizers choose to do.
|
|
Some comments were "suffixed" to member variable declarations; these are
moved to before the variable.
Some constructors and operators were just defaulted and not necessary.
Some comments dividing the class into logical sections, like "//
constructors and destructors", were not applied everywhere. These were
removed. They are used in some parts of LLDB, but are the exception.
An include was not needed.
The operator != can be defined in terms of ==.
|
|
Avoids more Triple->string->Triple round trip. This
is a continuation of f137c3d592e96330e450a8fd63ef7e8877fc1908
|
|
DWO file count functions" (#156777)
Reverts llvm/llvm-project#155023
The PR tests passed, but it failed in the CI. Reverting to give time to
investigate.
|
|
This patch changes the way frames created from scripted affordances like
Scripted Threads are displayed. Currently, they're marked artificial
which is used usually for compiler generated frames.
This patch changes that behaviour by introducing a new synthetic
StackFrame kind and moves 'artificial' to be a distinct StackFrame
attribut.
On top of making these frames less confusing, this allows us to know
when a frame was created from a scripted affordance.
rdar://155949703
Signed-off-by: Med Ismail Bennani <ismail@bennani.ma>
|
|
count functions (#155023)
## Summary
A new `totalDwoErrorCount` counter is available in statistics when
calling `statistics dump` to track the number of DWO errors.
Additionally, this PR refactors the DWO file statistics by consolidating
the existing functionality for counting loaded and total DWO files
together with the number of DWO errors into a single function that
returns a new DWOStats struct.
1. A new struct, `DWOStats` is created to hold the number of loaded DWO
files, the total number of DWO files and the number of DWO errors.
2. Replaced the previous `GetDwoFileCounts` function for loaded and
total DWO file counts with a single `GetDwoStats()` function returning
the struct `DWOStats`. An override is implemented for `SymbolFileDWARF`
that computes the new DWO error count alongside existing counts in one
pass. If the status of a DWO CU is `Fail`, which means there is error
happened during the loading process, we increment the DWO error counter.
_Note: The newly created function `GetDwoStats` will only be called when
we try to get statistics. Other codepaths will not be affected._
3. In Statistics, we sum up the total number of DWO file loading errors.
This is done by getting `DWOStats` for each symbol file and adding up
the results for each module, then adding to the total count among all
modules.
4. In Statistics, we also updated call sites to use the new combined
function and struct for loaded and total DWO file counts. As it is
possible for one module to have several symbol files, the DWO file
counts in a module's stats are updated to be calculated by adding up the
counts from all symbol files.
## Expected Behavior
- When binaries are compiled with split-dwarf and separate DWO files,
`totalDwoLoadErrorCount` would be the number of dwo files with error
occurs during the loading process, 0 if no error occurs during a loading
process.
- When not using split-dwarf, we expect `totalDwoLoadErrorCount` to be 0
since there no DWO file loading errors would be caused.
- `totalLoadedDwoFileCount` and `totalDwoFileCount` should be correctly
calculated after refactoring and updating.
## Testing
### Manual Testing
We created some files to simulate the possible DWO errors manually and
observed the results generated by statistics dump.
For example, if we delete one of the DWO files generated after
compiling, we would get:
```
(lldb) statistics dump
{
...
"totalDwoLoadErrorCount": 1,
...
}
```
We also checked the time cost of `statistics dump` w/o the modification
to make sure no significant time cost increase imported.
### Unit test
Added two unit tests that build with new "dwo_error_foo.cpp" and
"dwo_error_main.cpp" files. For tests with flags -gsplit-dwarf, this
generates 2 DWO files.
In one of the tests, we delete both DWO files and check the result to
see if it reflects the number of DWO files with errors correctly. In
another test we update one of the files but loading the outdated .dwo
file of it, expecting it increments the error count by 1.
To run the test:
```
$ bin/lldb-dotest -p TestStats.py ~/local/llvm-project/lldb/test/API/commands/statistics/basic/ -G "dwo"
----------------------------------------------------------------------
Ran 27 tests in 2.680s
OK (skipped=21)
$ bin/lldb-dotest -p TestStats.py ~/local/llvm-project/lldb/test/API/commands/statistics/basic/
----------------------------------------------------------------------
Ran 27 tests in 370.131s
OK (skipped=3)
```
|
|
(follow-up to #147460) (#152887)
**Context**
Follow-up to
[#147460](https://github.com/llvm/llvm-project/pull/147460), which added
the ability to surface register-resident variable locations.
This PR moves the annotation logic out of `Instruction::Dump()` and into
`Disassembler::PrintInstructions()`, and adds lightweight state tracking
so we only print changes at range starts and when variables go out of
scope.
---
## What this does
While iterating the instructions for a function, we maintain a “live
variable map” keyed by `lldb::user_id_t` (the `Variable`’s ID) to
remember each variable’s last emitted location string. For each
instruction:
- **New (or newly visible) variable** → print `name = <location>` once
at the start of its DWARF location range, cache it.
- **Location changed** (e.g., DWARF range switched to a different
register/const) → print the updated mapping.
- **Out of scope** (was tracked previously but not found for the current
PC) → print `name = <undef>` and drop it.
This produces **concise, stateful annotations** that highlight variable
lifetime transitions without spamming every line.
---
## Why in `PrintInstructions()`?
- Keeps `Instruction` stateless and avoids changing the
`Instruction::Dump()` virtual API.
- Makes it straightforward to diff state across instructions (`prev →
current`) inside the single driver loop.
---
## How it works (high-level)
1. For the current PC, get in-scope variables via
`StackFrame::GetInScopeVariableList(/*get_parent=*/true)`.
2. For each `Variable`, query
`DWARFExpressionList::GetExpressionEntryAtAddress(func_load_addr,
current_pc)` (added in #144238).
3. If the entry exists, call `DumpLocation(..., eDescriptionLevelBrief,
abi)` to get a short, ABI-aware location string (e.g., `DW_OP_reg3 RBX →
RBX`).
4. Compare against the last emitted location in the live map:
- If not present → emit `name = <location>` and record it.
- If different → emit updated mapping and record it.
5. After processing current in-scope variables, compute the set
difference vs. the previous map and emit `name = <undef>` for any that
disappeared.
Internally:
- We respect file↔load address translation already provided by
`DWARFExpressionList`.
- We reuse the ABI to map LLVM register numbers to arch register names.
---
## Example output (x86_64, simplified)
```
-> 0x55c6f5f6a140 <+0>: cmpl $0x2, %edi ; argc = RDI, argv = RSI
0x55c6f5f6a143 <+3>: jl 0x55c6f5f6a176 ; <+54> at d_original_example.c:6:3
0x55c6f5f6a145 <+5>: pushq %r15
0x55c6f5f6a147 <+7>: pushq %r14
0x55c6f5f6a149 <+9>: pushq %rbx
0x55c6f5f6a14a <+10>: movq %rsi, %rbx
0x55c6f5f6a14d <+13>: movl %edi, %r14d
0x55c6f5f6a150 <+16>: movl $0x1, %r15d ; argc = R14
0x55c6f5f6a156 <+22>: nopw %cs:(%rax,%rax) ; i = R15, argv = RBX
0x55c6f5f6a160 <+32>: movq (%rbx,%r15,8), %rdi
0x55c6f5f6a164 <+36>: callq 0x55c6f5f6a030 ; symbol stub for: puts
0x55c6f5f6a169 <+41>: incq %r15
0x55c6f5f6a16c <+44>: cmpq %r15, %r14
0x55c6f5f6a16f <+47>: jne 0x55c6f5f6a160 ; <+32> at d_original_example.c:5:10
0x55c6f5f6a171 <+49>: popq %rbx ; i = <undef>
0x55c6f5f6a172 <+50>: popq %r14 ; argv = RSI
0x55c6f5f6a174 <+52>: popq %r15 ; argc = RDI
0x55c6f5f6a176 <+54>: xorl %eax, %eax
0x55c6f5f6a178 <+56>: retq
```
Only transitions are shown: the start of a location, changes, and
end-of-lifetime.
---
## Scope & limitations (by design)
- Handles **simple locations** first (registers, const-in-register cases
surfaced by `DumpLocation`).
- **Memory/composite locations** are out of scope for this PR.
- Annotations appear **only at range boundaries** (start/change/end) to
minimize noise.
- Output is **target-independent**; register names come from the target
ABI.
## Implementation notes
- All annotation printing now happens in
`Disassembler::PrintInstructions()`.
- Uses `std::unordered_map<lldb::user_id_t, std::string>` as the live
map.
- No persistent state across calls; the map is rebuilt while walking
instruction by instruction.
- **No changes** to the `Instruction` interface.
---
## Requested feedback
- Placement and wording of the `<undef>` marker.
- Whether we should optionally gate this behind a setting (currently
always on when disassembling with an `ExecutionContext`).
- Preference for immediate inclusion of tests vs. follow-up patch.
---
Thanks for reviewing! Happy to adjust behavior/format based on feedback.
---------
Co-authored-by: Jonas Devlieghere <jonas@devlieghere.com>
Co-authored-by: Adrian Prantl <adrian.prantl@gmail.com>
|
|
`cfa_reg_contents` is a local variable. Whatever value we assign there
right before the `return` statement will be lost anyway.
Co-authored-by: Matej Košík <matej.kosik@codasip.com>
|
|
The PR https://github.com/llvm/llvm-project/pull/152020 got merged by
accident. This commit addresses some follow up review comments.
|
|
Prior to this patch, SBFrame/SBThread methods exhibit racy behavior if
called while the process is running, because they do not lock the
`Process::RetRunLock` mutex. If they did, they would fail, correctly
identifying that the process is not running.
Some methods _attempt_ to protect against this with the pattern:
```
ExecutionContext exe_ctx(m_opaque_sp.get(), lock); // this is a different lock
Process *process = exe_ctx.GetProcessPtr();
if (process) {
Process::StopLocker stop_locker;
if (stop_locker.TryLock(&process->GetRunLock()))
.... do work ...
```
However, this is also racy: the constructor of `ExecutionContext` will
access the frame list, which is something that can only be done once the
process is stopped.
With this patch:
1. The constructor of `ExecutionContext` now expects a `ProcessRunLock`
as an argument. It attempts to lock the run lock, and only fills in
information about frames and threads if the lock can be acquired.
Callers of the constructor are expected to check the lock.
2. All uses of ExecutionContext are adjusted to conform to the above.
3. The SBThread.cpp-defined helper function ResumeNewPlan now expects a
locked ProcessRunLock as _proof_ that the execution is stopped. It will
unlock the mutex prior to resuming the process.
This commit exposes many opportunities for early-returns, but these
would increase the diff of this patch and distract from the important
changes, so we opt not to do it here.
|
|
In architectures where pointers may contain metadata, such as arm64e, it
is important to ignore those bits when comparing two different StackIDs,
as the metadata may not be the same even if the pointers are.
This patch is a step towards allowing consumers of pointers to decide
whether they want to keep or remove metadata, as opposed to discarding
metadata at the moment pointers are created. See
https://github.com/llvm/llvm-project/pull/150537.
This was tested running the LLDB test suite on arm64e.
|
|
Resolves #141955
- Adds data to breakpoints `Source` object, in order for assembly
breakpoints, which rely on a temporary `sourceReference` value, to be
able to resolve in future sessions like normal path+line breakpoints
- Adds optional `instructions_offset` parameter to `BreakpointResolver`
|
|
|
