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.
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 addition of the StopCondition in the lldb_private layer meant that
clearing a breakpoint condition with:
sb_break.SetCondition(nullptr);
now crashes. Also, GetCondition for an empty condition used to return a
nullptr, but now it returns "".
This patch fixes that crash and makes the SB GetCondition always return
nullptr for an empty condition.
Enforce that only specific tests can link liblldb. All the other unit
tests statically link the private libraries. Linking both the static
libraries and liblldb results in duplicated symbols.
Fixes#162378
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
7f51a2a47d which introduced the
ToDwarfSourceLanguage function.
Added in 1902ffd9a4 but appears to be
leftover code from some older design.
I can't find any code that reads packetLog via the class itself, or
checks whether it is None.
No tests failed on AArch64 Linux after removing it.
This test, with a corefile created via yaml2macho-core plus an
ObjectFileJSON binary with symbol addresses and ranges, was failing
on some machines/CI because the wrong ABI was being picked.
The bytes of the functions were not included in the yaml or .json
binary. The unwind falls back to using the ABI plugin default
unwind plans. We have two armv7 ABIs - the Darwin ABI that always
uses r7 as the frame pointer, and the AAPCS ABI which uses r11 code.
In reality, armv7 code uses r11 in arm mode, r7 in thumb code. But
the ABI ArchDefaultUnwindPlan doesn't have any access to the Target's
ArchSpec or Process register state, to determine the correct processor
state (arm or thumb). And in fact, on Cortex-M targets, the
instructions are always thumb, so the arch default unwind plan
(hardcoded r11) is always wrong.
The corefile doesn't specify a vendor/os, only a cpu.
The object file json specifies the armv7m-apple-* triple, which will
select the correct ABI plugin, and the test runs.
In some cases, it looks like the Process ABI was fetched after
opening the corefile, but before the binary.json was loaded and
corrected the Target's ArchSpec. And we never re-evaluate the ABI
once it is set, in a Process. When we picked the AAPCS armv7 ABI,
we would try to use r11 as frame pointer, and the unwind would stop
after one stack frame.
I'm stepping around this problem by (1) adding the register bytes of
the prologues of every test function in the backtrace, and (2)
shortening the function ranges (in binary.json) to specify that the
functions are all just long enough for the prologue where execution
is stopped. The instruction emulation plugin will fail if it can't
get all of the bytes from the function instructions, so I hacked
the function sizes in the .json to cover the prologue plus one and
changed the addresses in the backtrace to fit within those ranges.
Compute the start time *before* registering the callback, rather than
after, to avoid the possibility of a small race.
The following scenario illustrates the problem.
1. The callback is registered with a 2 second timeout at t=0ms.
2. We compute the start time after registering the callback. For the
sake of argument, let's say it took 5ms to return from registering the
callback and computing the current time. Start=5ms.
3. The callback fires after exactly 2 seconds, or t=2000ms.
4. We compute the difference between start and now. If it took less than
5ms to compute, then we end up with a difference that's less than 2000ms
and the test fails. Let's say it took 3ms this time, then
2003ms-5ms=1998ms < 2000ms.
The actual values in the example above are arbitrary. All that matters
is that it took longer to compute the start time than the end time. My
theory is that this explains why this test is flaky when running under
ASan in CI (which has unpredictable timing).
rdar://160956999
### 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
```
Relands #149701 which was reverted in
185ae5cdc6
because it broke demangling of Itanium symbols on i386.
The last commit in this PR adds the fix for this (discussed in #160930).
On x86 environments, the prefix of `__cdecl` functions will now be
removed to match DWARF. I opened #161676 to discuss this for the other
calling conventions.
Relates to #161510
Fixes 6db44e52ce
(it's not fixing it, it's just making the error not be an unhandled
error)
When we fail to attach to a process we see if we can add more
information about why it happened:
```
if (status.GetError() == EPERM) {
// Depending on the value of ptrace_scope, we can return a different
// error that suggests how to fix it.
return AddPtraceScopeNote(status.ToError());
}
```
ToError creates a new error value and leaves the one in `status`
unchecked. `status`'s error is ok because it will be checked by Status'
destructor.
The problem happens in `AddPtraceScopeNote`. If we take certain return
paths, this new error, or the one we get when trying to find the ptrace
scope, may be unchecked on destruction when the function returns.
To fix this, in AddPtraceScopeNote, consume any errors that we are not
going to return. Anything returned will be checked by some caller.
Reproducing this failure mode is difficult but it can be faked by
calling AddPtraceScopeNote earlier. Which is what I did to prove the
concept of the problem.
Fixes#161510
Addresses the Linux parts of #138085
The situation we have to handle here is systems where Yama ptrace_scope
set to 1.
> 1 - restricted ptrace: a process must have a predefined relationship
> with the inferior it wants to call PTRACE_ATTACH on. By default,
> this relationship is that of only its descendants when the above
> classic criteria is also met. To change the relationship, an
> inferior can call prctl(PR_SET_PTRACER, debugger, ...) to declare
> an allowed debugger PID to call PTRACE_ATTACH on the inferior.
> Using PTRACE_TRACEME is unchanged.
(https://www.kernel.org/doc/Documentation/security/Yama.txt)
The inferior was addressing this by calling this at the start of main():
prctl(PR_SET_PTRACER, PR_SET_PTRACER_ANY, 0, 0, 0);
Which is ok if lldb-server tries to attach after that call has happened,
but there was nothing to synchronise this. So if the system was heavily
loaded, the inferior may be stalled, delaying the call, causing
lldb-server to fail to attach with EPERM (permission denied).
We were not using any mechanism to retry the attach or wait for some
signal from the inferior.
Except we do do this in other tests, even other lldb-server tests.
So I have adopted that mechanism to these tests:
* The inferior is launched with `syncfile:<path>` as its first argument.
* It creates this file at `<path>`, at a point where we know attaching
has been allowed.
* The test framework launches the inferior then waits for the file to
appear.
* This check is retried a few times, increasing the delay each time and
eventually giving up.
* Only once it has seen the file does it start lldb-server and tell it
to attach to the inferior.
I have tested this by insterting a `sleep()` call before the attach
enable call and running the test on a machine with ptrace_scope set to
1. I was able to increase the sleep to 6 seconds before tests failed
(when running just these tests, single threaded).
With OS scheduling, you could be stalled indefinitely, so we may have to
increase this timeout but this is easy to do with
wait_for_file_on_target.
The alternative is to have the test runner check ptrace_scope and only
enable these on systems where it's 0. Would be good to keep them running
if we can though.
The way I was setting the path to the yaml2macho-core tool for
API tests assumed that the llvm tool bin directory was the same
as the lldb tool bin directory. There are build configuration
styles where they are not. Set it the same way lldb-dap etc
are set to the lldb bin dir.
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 adds a new Binding helper class to allow mapping of incoming and
outgoing requests / events to specific handlers.
This should make it easier to create new protocol implementations and
allow us to create a relay in the lldb-mcp binary.
This reverts `5a80fb9177e3c831c9c574400a13d77393397f2a`. The original
change got reverted because of failing tests on macOS.
The issue was that I changed the scope of setting `type =
eSymbolTypeData` during the cleanup. This patch relands the original
patch but doesn't change the `else` branch to an `else if` branch.
Tested that macOS test-suite passes.
The `CompilerInstance::createSourceManager()` function currently accepts
the `FileManager` to be used. However, all clients call
`CompilerInstance::createFileManager()` prior to creating the
`SourceManager`, and it never makes sense to use a `FileManager` in the
`SourceManager` that's different from the rest of the compiler. Passing
the `FileManager` explicitly is redundant, error-prone, and deviates
from the style of other `CompilerInstance` initialization APIs.
This PR therefore removes the `FileManager` parameter from
`createSourceManager()` and also stops returning the `FileManager`
pointer from `createFileManager()`, since that was its primary use. Now,
`createSourceManager()` internally calls `getFileManager()` instead.
Skip tests that require `-gstructor-decl-linkage-names` on Clang versions that don't support it.
Don't pass `-gno-structor-decl-linkage-names` on Clang versions where it the flag didn't exist but it was the default behaviour of the compiler anyway.
Drive-by:
- We used to run `self.expect("Bar()")` which would always fail. So the `error=True` would be true even if we didn't pass the `-gno-structor-linkage-names`. So it wasn't testing the behaviour properly. This patch changes these to `self.expect("expr Bar()")`.
Skip tests that require `-gstructor-decl-linkage-names` on Clang
versions that don't support it.
Don't pass `-gno-structor-decl-linkage-names` on Clang versions where it
the flag didn't exist but it was the default behaviour of the compiler
anyway.
Failing with:
```
error: command failed with exit status: 1
executed command: 'c:\buildbot\as-builder-10\lldb-x86-64\build\bin\filecheck.exe' 'C:\buildbot\as-builder-10\lldb-x86-64\llvm-project\lldb\test\Shell\Expr\TestGlobalSymbolObjCConflict.c'
.---command stderr------------
| C:\buildbot\as-builder-10\lldb-x86-64\llvm-project\lldb\test\Shell\Expr\TestGlobalSymbolObjCConflict.c:30:11: error: CHECK: expected string not found in input
| // CHECK: (lldb) p OglobalVar
| ^
| <stdin>:1:1: note: scanning from here
| (lldb) command source -s 0 'C:/buildbot/as-builder-10/lldb-x86-64/build/tools/lldb\test\Shell\lit-lldb-init-quiet'
| ^
| <stdin>:4:1: note: possible intended match here
| (lldb) target create "C:\\buildbot\\as-builder-10\\lldb-x86-64\\build\\tools\\lldb\\test\\Shell\\Expr\\Output\\TestGlobalSymbolObjCConflict.c.tmp.out"
| ^
|
| Input file: <stdin>
| Check file: C:\buildbot\as-builder-10\lldb-x86-64\llvm-project\lldb\test\Shell\Expr\TestGlobalSymbolObjCConflict.c
|
| -dump-input=help explains the following input dump.
|
| Input was:
| <<<<<<
| 1: (lldb) command source -s 0 'C:/buildbot/as-builder-10/lldb-x86-64/build/tools/lldb\test\Shell\lit-lldb-init-quiet'
| check:30'0 X~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ error: no match found
| 2: Executing commands in 'C:\buildbot\as-builder-10\lldb-x86-64\build\tools\lldb\test\Shell\lit-lldb-init-quiet'.
| check:30'0 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| 3: (lldb) command source -C --silent-run true lit-lldb-init
| check:30'0 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| 4: (lldb) target create "C:\\buildbot\\as-builder-10\\lldb-x86-64\\build\\tools\\lldb\\test\\Shell\\Expr\\Output\\TestGlobalSymbolObjCConflict.c.tmp.out"
| check:30'0 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| check:30'1 ? possible intended match
| 5: Current executable set to 'C:\buildbot\as-builder-10\lldb-x86-64\build\tools\lldb\test\Shell\Expr\Output\TestGlobalSymbolObjCConflict.c.tmp.out' (x86_64).
| check:30'0 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| 6: (lldb) b 27
| check:30'0 ~~~~~~~~~~~~
| >>>>>>
`-----------------------------
error: command failed with exit status: 1
```
We probably need to use LLD here or something. But I don't have a Windows machine to test this on. So XFAILing for now.
This test failed during testing on the RISC-V target because we couldn't
strip the main label from the binary. main is dynamically linked when
the -fPIC flag is enabled. The RISC-V ABI requires that executables
support loading at arbitrary addresses to enable shared libraries and
secure loading (ASLR). In PIC mode, function addresses cannot be
hardcoded in the code. Instead, code is generated to load addresses from
the GOT/PLT tables, which are initialized by the dynamic loader. The
reference to main thus ends up in .dynsym and is dynamically bound. We
cannot strip main or any other dynamically linked functions because
these functions are referenced indirectly via dynamic linking tables
(.plt and .got). Removing these symbols would break the dynamic linking
mechanism needed to resolve function addresses at runtime, causing the
executable to fail to correctly call them.
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
On Darwin C-symbols are prefixed with a '_'. The LLDB Macho-O parses
handles Objective-C metadata symbols starting with '_OBJC' specially.
Previously global symbols starting with a '_O' prefix were lost because
of incorrectly scoped if-guards. This patch removes those checks.
There is more cleanup that can be done in this file because there's a
bunch of duplicated checks for these ObjC symbols. I decided to leave
that for an NFC follow-up.
Depends on https://github.com/llvm/llvm-project/pull/161520
rdar://158159242
We've been seen (very sporadic) lifetime issues around this area. We
noticed that `GetMangledName` has two accessors, one of which returns a
non-const reference. I audited all the callsites and no users of this
overload actually mutate the `ConstString` itself (which is a suspicious
thing to do anyway since it's just a wrapper around a `const char*`).
This patch removes the redundant overload.
rdar://161128180
