Seemingly, #96256 removed the only call to
Platform::GetCachedExecutable, which broke the resolution of executable
modules in the remote debugging mode
(https://github.com/llvm/llvm-project/issues/97410).
This commit fixes that.
In RegisterContextUnwind::SavedLocationForRegister we have special logic
for retrieving the Return Address register when it has the caller's
return address in it. An example would be the lr register on AArch64.
This register is never retrieved from a newer stack frame because it is
necessarly overwritten by a normal ABI function call. We allow frame 0
to provide its lr value to get the caller's return address, if it has
not been overwritten/saved to stack yet.
When a function is interrupted asynchronously by a POSIX signal
(sigtramp), or a fault handler more generally, the sigtramp/fault
handler has the entire register context available. In this situation, if
the fault handler is frame 0, the function that was async interrupted is
frame 1 and frame 2's return address may still be stored in lr. We need
to get the lr value for frame 1 from the fault handler in frame 0, to
get the return address for frame 2.
Without this fix, a frameless function that faults in a firmware
environment (that's where we've seen this issue most commonly) hasn't
spilled lr to stack, so we need to retrieve it from the fault handler's
full-register-context to find the caller of the frameless function that
faulted.
It's an unsurprising fix, all of the work was finding exactly where in
RegisterContextUnwind we were only allowing RA register use for frame 0,
when it should have been frame 0 or above a fault handler function.
rdar://127518945
PT_LOAD and PT_TLS segments are top level sections in the ObjectFileELF
section list. The two segments can often have the same program header
p_vaddr and p_paddr values and this can cause section load list issues
in LLDB if we load the PT_TLS segments. What happens is the
SectionLoadList::m_addr_to_sect, when a library is loaded, will first
map one of the sections named "PT_LOAD[0]" with the load address that
matches the p_vaddr entry from the program header. Then the "PT_TLS[0]"
would come along and try to load this section at the same address. This
would cause the "PT_LOAD[0]" section to be unloaded as the
SectionLoadList::m_addr_to_sect would replace the value for the matching
p_vaddr with the last section to be seen. The sizes of the PT_TLS and
PT_LOAD that have the same p_vaddr value don't need to have the same
byte size, so this could cause lookups to fail for an addresses in the
"PT_LOAD[0]" section or any of its children if the offset is greater
than the offset size of the PT_TLS segment. It could also cause us to
incorrectly attribute addresses from the "PT_LOAD[0]" to the "PT_TLS[0]"
segment when doing lookups for offset that are less than the size of the
PT_TLS segment.
This fix stops us from loading PT_TLS segments in the section load lists
and will prevent the bugs that resulted from this. No addresses the the
DWARF refer to TLS data with a "file address" in any way. They all have
TLS DWARF location expressions to locate these variables. We also don't
have any support for having actual thread specific sections and having
those sections resolve to something different for each thread, so there
currently is no point in loading thread specific sections. Both the
ObjectFileMachO and ObjectFileCOFF both ignore thread specific sections
at the moment, so this brings the ObjectFileELF to parity with those
plug-ins.
I added a test into an existing test to verify that things work as
expected.
Prior to this fix with a real binary, the output of "target dump
section-load-list" would look like this for the old LLDB:
```
// (lldb) target dump section-load-list
// addr = 0x0000000000000000, section = 0x55d46ab8c510: 0xfffffffffffffffd container [0x0000000000000000-0x0000000000000628) r-- 0x00000000 0x00000628 0x00000000 a.out.PT_LOAD[0]
// addr = 0x0000000000001000, section = 0x55d46ab8b0c0: 0xfffffffffffffffc container [0x0000000000001000-0x0000000000001185) r-x 0x00001000 0x00000185 0x00000000 a.out.PT_LOAD[1]
// addr = 0x0000000000002000, section = 0x55d46ac040f0: 0xfffffffffffffffb container [0x0000000000002000-0x00000000000020cc) r-- 0x00002000 0x000000cc 0x00000000 a.out.PT_LOAD[2]
// addr = 0x0000000000003db0, section = 0x55d46ab7cef0: 0xfffffffffffffff6 container [0x0000000000003db0-0x0000000000003db4) r-- 0x00002db0 0x00000000 0x00000000 a.out.PT_TLS[0]
```
And this for the fixed LLDB:
```
// (lldb) target dump section-load-list
// addr = 0x0000000000000000, section = 0x105f0a9a8: 0xfffffffffffffffd container [0x0000000000000000-0x0000000000000628) r-- 0x00000000 0x00000628 0x00000000 a.out.PT_LOAD[0]
// addr = 0x0000000000001000, section = 0x105f0adb8: 0xfffffffffffffffc container [0x0000000000001000-0x0000000000001185) r-x 0x00001000 0x00000185 0x00000000 a.out.PT_LOAD[1]
// addr = 0x0000000000002000, section = 0x105f0af48: 0xfffffffffffffffb container [0x0000000000002000-0x00000000000020cc) r-- 0x00002000 0x000000cc 0x00000000 a.out.PT_LOAD[2]
// addr = 0x0000000000003db0, section = 0x105f0b078: 0xfffffffffffffffa container [0x0000000000003db0-0x0000000000004028) rw- 0x00002db0 0x00000274 0x00000000 a.out.PT_LOAD[3]
```
We can see that previously the "PT_LOAD[3]" segment would be removed
from the section load list, and after the fix it remains and there is on
PT_TLS in the loaded sections.
ThreadList uses the Process mutex to guard its state. This means its not
possible to safely modify its process member, as the member is required
to lock the mutex.
Fortunately for us, we never actually need to change the process member
(we always just juggle different kinds of thread lists belonging to the
same process).
This patch replaces the process member assignment (which is technically
a race even when it assigns the same value) with an assertion.
Since all this means that the class can never change its process member
value (and it also must be non-null at all times), I've also changed the
member type to a reference.
Correct the method used to truncate the source_file string when
substring is a prefix. The previous method used substr, which was
changed to resize for clarity and efficiency.
Caught by cppcheck -
lldb/source/Plugins/Platform/Android/PlatformAndroid.cpp:290:19:
performance: Ineffective call of function 'substr' because a prefix of
the string is assigned to itself. Use resize() or pop_back() instead.
[uselessCallsSubstr]
Source code -
source_file = source_file.substr(0, pos);
Fix#91211
---------
Co-authored-by: Shivam Gupta <shivma98.tkg@gmail.com>
…lly registered languages
First of all, this is done to support exceptions for the Mojo language,
but it's done in a way that will benefit any other plugin language.
1. I added a new lldb-dap CLI argument (not DAP field) called
`pre-init-commands`. These commands are executed before DAP
initialization. The other `init-commands` are executed after DAP
initialization. It's worth mentioning that the debug adapter returns to
VSCode the list of supported exception breakpoints during DAP
initialization, which means that I need to register the Mojo plugin
before that initialization step, hence the need for `pre-init-commands`.
In general, language plugins should be registered in that step, as they
affect the capabilities of the debugger.
2. I added a set of APIs for lldb-dap to query information of each
language related to exception breakpoints. E.g. whether a language
supports throw or catch breakpoints, how the throw keyword is called in
each particular language, etc.
3. I'm realizing that the Swift support for exception breakpoints in
lldb-dap should have been implemented in this way, instead of hardcoding
it.
This is all the tests and fixes I've had percolating since my first
attempt at this in January. After 6 months of trying, I've given up on
adding the ability to test DWP files in LLDB API tests. I've left both
the tests (disabled) and the changes to Makefile.rules in place, in the
hopes that someone who can configure the build bots will be able to
enable the tests once a non-borked dwp tool is widely available.
Other than disabling the DWP tests, this continues to be the same diff
that I've tried to land and
[not](https://github.com/llvm/llvm-project/pull/90622)
[revert](https://github.com/llvm/llvm-project/pull/87676)
[five](https://github.com/llvm/llvm-project/pull/86812)
[times](https://github.com/llvm/llvm-project/pull/85693)
[before](https://github.com/llvm/llvm-project/pull/96802). There are a
couple of fixes that the testing exposed, and I've abandoned the DWP
tests because I want to get those fixes finally upstreamed, as without
them DebugInfoD is less useful.
While looking at a TSAN report (patch coming soon) in the ThreadList
class, I noticed that this code would be simpler if it did not use the
ThreadList class.
This improves the handling of `$` (dollar) characters in summary strings in the
following ways:
1. When a `$` is not followed by an open paren (`{`), it should be treated as a literal
character and preserved in the output. Previously, the dollar would be consumed by the
parser and not shown in the output.
2. When a `$` is the last character of a format string, this change eliminates the
infinite loop lldb would enter into.
rdar://131392446
Use POSIX-style path separators when checking for libcpp library path.
This is necessary to run API tests from 'std-module' group compiled on
Windows host for Linux target.
Reverts llvm/llvm-project#96802
Attempt #5 fails. It's been 6 months. I despise Makefile.rules and have
no ability to even *detect* these failures without _landing_ a diff. In
the mean time, we have no testing for DWP files at all (and a regression
that was introduced, that I fix with this diff) so I'm going to just
remove some of the tests and try to land it again, but with less testing
I guess.
This is the same diff I've put up at many times before. I've been trying
to add some brand new functionality to the LLDB test infrastucture
(create split-dwarf files!), and we all know that no good deed goes
unpunished. The last attempt was reverted because it didn't work on the
Fuchsia build.
There are no code differences between this and
[the](https://github.com/llvm/llvm-project/pull/90622)
[previous](https://github.com/llvm/llvm-project/pull/87676)
[four](https://github.com/llvm/llvm-project/pull/86812)
[diffs](https://github.com/llvm/llvm-project/pull/85693) landed &
reverted (due to testing infra failures). The only change in this one is
the way `dwp` is being identified in `Makefile.rules`.
Thanks to @petrhosek for helping me figure out how the fuchsia builders
are configured. I now prefer to use llvm-dwp and fall back to gnu's dwp
if the former isn't found. Hopefully this will work everywhere it needs
to.
Currently, LLDB prints out a rather unhelpful error message when passed
a file that it doesn't recognize as an executable.
> error: '/path/to/file' doesn't contain any 'host' platform
> architectures: arm64, armv7, armv7f, armv7k, armv7s, armv7m, armv7em,
> armv6m, armv6, armv5, armv4, arm, thumbv7, thumbv7k, thumbv7s,
> thumbv7f, thumbv7m, thumbv7em, thumbv6m, thumbv6, thumbv5, thumbv4t,
> thumb, x86_64, x86_64, arm64, arm64e
I did a quick search internally and found at least 24 instances of users
being confused by this. This patch improves the error message when it
doesn't recognize the file as an executable, but keeps the existing
error message otherwise, i.e. when it's an object file we understand,
but the current platform doesn't support.
Depends on:
* https://github.com/llvm/llvm-project/pull/97544
* https://github.com/llvm/llvm-project/pull/97549
* https://github.com/llvm/llvm-project/pull/97551
This patch tries to simplify the way in which the `std::map` formatter
goes from the root `__tree` pointer to a specific key/value pair.
Previously we would:
1. synthesize a structure that mimicked what `__iter_pointer` looked
like in memory
2. call `GetChildCompilerTypeAtIndex` on it to find the byte offset at
which the pair was located in the synthesized structure
3. finally, use that offset through a call to
`GetSyntheticChildAtOffset` to retrieve the key/value pair
Not only was this logic hard to follow, and encoded the libc++ layout in
non-obvious ways, it was also fragile to alignment miscalculations
(https://github.com/llvm/llvm-project/pull/97443); this would break once
the new layout of std::map landed as part of
https://github.com/https://github.com/llvm/llvm-project/issues/93069.
Instead, this patch simply casts the `__iter_pointer` to the
`__node_pointer` and uses a straightforward
`GetChildMemberWithName("__value_")` to get to the key/value we care
about. This allows us to get rid of some support infrastructure/class
state.
Ideally we would fix the underlying alignment issue, but this unblocks
the libc++ refactor in the interim, while also benefitting the formatter
in terms of readability (in my opinion).
Depends on https://github.com/llvm/llvm-project/pull/97687
Similar to https://github.com/llvm/llvm-project/pull/97579, this patch
simplifies the way in which we retrieve the key/value pair of a
`std::map` (in this case of the `std::map::iterator`).
We do this for the same reason: not only was the old logic hard to
follow, and encoded the libc++ layout in non-obvious ways, it was also
fragile to alignment miscalculations
(https://github.com/llvm/llvm-project/pull/97443); this would break once
the new layout of std::map landed as part of
https://github.com/llvm/llvm-project/issues/93069.
Instead, this patch simply casts the `__iter_pointer` to the
`__node_pointer` and uses a straightforward
`GetChildMemberWithName("__value_")` to get to the key/value we care
about.
We can eventually re-use the core-part of the `std::map` and
`std::map::iterator` formatters. But it will be an easier to change to
review once both simplifications landed.
Depends on https://github.com/llvm/llvm-project/pull/97752
This patch changes the way we retrieve the key/value pair in the
`std::unordered_map::iterator` formatter (similar to how we are changing
it for `std::map::iterator` in
https://github.com/llvm/llvm-project/pull/97713, the motivations being
the same).
The old logic was not very easy to follow, and encoded the libc++ layout
in non-obvious ways. But mainly it was also fragile to alignment
miscalculations (https://github.com/llvm/llvm-project/pull/97443); this
would break once the new layout of `std::unordered_map` landed as part
of https://github.com/llvm/llvm-project/issues/93069.
Instead, this patch simply casts the `__hash_iterator` to a
`__node_pointer` (which is what libc++ does too) and uses a
straightforward `GetChildMemberWithName("__value_")` to get to the
key/value we care about.
The `std::unordered_map` already does it this way, so we align the
iterator counterpart to do the same. We can eventually re-use the
core-part of the `std::unordered_map` and `std::unordered_map::iterator`
formatters. But it will be an easier to change to review once both
simplifications landed.
Similar to how we moved the `std::map::iterator` formatter in
https://github.com/llvm/llvm-project/pull/97687, do the same for
`std::unordered_map::iterator`.
Again the `unordered_map` and `unordered_map::iterator` formatters try
to do very similar things: retrieve data out of the map. The iterator
formatter does this in a fragile way (similar to how `std::map` does it,
see https://github.com/llvm/llvm-project/pull/97579). Thus we will be
refactoring the `std::unordered_map::iterator` in upcoming patches.
Having it in `LibCxxUnorderedMap` will allow us to re-use some of the
logic (and we won't have to repeat some of the clarification comments).
We currently supported the layout from pre-2016 (before the layout
change in
[14caaddd3f08e798dcd9ac0ddfc](https://github.com/llvm/llvm-project/commit/14caaddd3f08e798dcd9ac0ddfc)).
We have another upcoming layout change in `__tree` and `map` (as part of
https://github.com/llvm/llvm-project/issues/93069) which will likely
require rewriting parts of this formatter. Removing the support for the
pre-2016 layout will make those changes more straightforward to
review/maintain.
Being backward compatible would be great but we have no tests that
actually verify that the old layout still works (and our oldest matrix
bot tests clang-15). If anyone feels strongly about keeping this layout,
we could possibly factor out that logic and keep it around.
As a minor follow up for https://github.com/llvm/llvm-project/pull/97675, I'm renaming `SupportsExceptionBreakpoints` to `SupportsExceptionBreakpointsOnThrow` and adding a `SupportsExceptionBreakpointsOnCatch` to have a bit of more granularity.
The two formatters follow very similar techniques to retrieve data out
of the map. We're changing this for `std::map` in
https://github.com/llvm/llvm-project/pull/97579 and plan to change it in
the same way for the iterator formatter. Having them in the same place
will allow us to re-use some of the logic (and we won't have to repeat
some of the clarification comments).
CommandObjectBreakpoint has a harcoded list of languages for which
exception breakpoints can be enabled. I'm making this a bit more generic
so that my Mojo plugin can get this feature.
Basically, I'm adding a new overridable method
`Language::SupportsExceptionBreakpoints` that can be used by language
plugins to determine whether they support this feature or not. This
method is used in addition to the hardcoded list and, as an example, I'm
using it for the ObjC language support.
Another route is simply to avoid doing the check that it's being done
right now and simply try to the create the exception breakpoint for
whatever language that is not in the hardcoded list. I'm happy to do
that if the reviewers think it's a good idea.
As a note, the other possible place for adding this
`SupportsExceptionBreakpoints` method is in `LanguageRuntime`. However,
accessing it requires having a process, which is not always the case
when invoking the `breakpoint set -E` command. The process might not be
alive yet, so `Language` is a good second place for this.
And as a final note, I don't want to make this
`SupportsExceptionBreakpoints` complicated. I'm keeping it as simple as
possible because it can easily evolve as it's not part of the public
API.
This reverts commit 927def49728371d746476e79a6570cd93a4d335c.
I've refactored the tests so that we're explicit about whether the
enum is signed or not. Which means we use the proper types
throughout.
This patch factors all the logic for advancing the `MapIterator` out of
`GetChildAtIndex`. This, in my opinion, helps readability, and will be
useful for upcoming cleanups in this area.
While here, some drive-by changes:
* added a couple of clarification comments
* fixed a variable name typo
* turned the `return lldb::ValueObjectSP()` into `return nullptr`
* added an assertion to make sure we keep the iterator cache in a valid
state
This was just a typo, none of the external execution control functions
should discard other plans. In particular, it means if you stop in a
hand-called function and step an instruction, the function call thread
plan gets unshipped, popping all the function call frames.
I also added a test that asserts the correct behavior. I tested all the
stepping operations even though only StepInstruction was wrong.
Among other things, returning an empty string as the repeat command
disables auto-repeat, which can be useful for state-changing commands.
There's one remaining refinement to this setup, which is that for parsed
script commands, it should be possible to change an option value, or add
a new option value that wasn't originally specified, then ask lldb "make
this back into a command string". That would make doing fancy things
with repeat commands easier.
That capability isn't present in the lldb_private side either, however.
So that's for a next iteration.
I haven't added this to the docs on adding commands yet. I wanted to
make sure this was an acceptable approach before I spend the time to do
that.
Enums like this one are treated as bitfield like enums: enum FlagsLike
{B=2, C=4};
lldb recognises them as collections of flags, so you can have "B | C".
If there's any values not covered that's printed as hex "B | C | 0x1".
What happened if the value was 0 was we would not match any of the
enumerators, then the remainder check requires that the remainder is
non-zero. So lldb would print nothing at all.
Which I assume is a bug because knowing that no flags are set is useful,
just as much as knowing that some unkown bit was set to make it
non-zero.
Fixes#97514
Given this example:
```
enum E {};
int main()
{
E x = E(0);
E y = E(1);
E z = E(2);
return 0;
}
```
lldb used to print nothing for `x`, but `0x1` for `y` and `0x2` for `z`.
At first this seemed like the 0 case needed fixing but the real issue
here is that en enum with no enumerators was being detected as a
"bitfield like enum".
Which is an enum where all enumerators are a single bit value, or the
sum of previous single bit values.
For these we do not print anything for a value of 0, as we assume it
must be the remainder after we've printed the other bits that were set
(I think this is also unfortunate, but I'm not addressing that here).
Clearly an enum with no enumerators cannot be being used as a bitfield,
so check that up front and print it as if it's a normal enum where we
didn't match any of the enumerators. This means you now get:
```
(lldb) p x
(E) 0
(lldb) p y
(E) 1
(lldb) p z
(E) 2
```
Which is a change to decimal from hex, but I think it's overall more
consistent. Printing hex here was never a concious decision.
While testing register fields I found that if you put the max value into
a bitfield with an underlying type that is an unsigned enum, lldb would
not print the enum name.
This is because the code to match values to names wasn't checking
whether the enum's type was signed, it just assumed it was.
So for example a 2 bit field with value 3 got signed extended to -1,
which didn't match the enumerator value of 3. So lldb just printed the
number instead of the name.
For a value of 1, the top bit was 0 so the sign extend became a zero
extend, and lldb did print the name of the enumerator.
I added a new test because I needed to use C++ to get typed enums. It
checks min, max and an in between value for signed and unsigned enums
applied to a bitfield.
Redundant since:
```
commit be3be28b5d5c97de1c26bf069e0b82043d938f30
Author: Enrico Granata <egranata@apple.com>
Date: Mon Oct 3 23:33:00 2016 +0000
Changes to the std::multimap formatter to make it work against trunk libc++
Fixes rdar://28237486
llvm-svn: 283160
```
Non-recursive mutexes encourage better locking discipline and avoid bugs
like #96750, where one can unexpectedly re-enter the critical section on
the same thread, and interrupt a presumed-indivisible operation.
In this case, the only needed fix was to remove locking from some
BroadcastManager functions, which were only called from the Listener
class (and the listener already locked those mutexes to preserve lock
ordering).
While doing that, I noticed we don't have unit tests for these
functions, so I added one.
This enables XML output for enums and adds enums for 2 fields on AArch64
Linux:
* mte_ctrl.tcf, which controls how tag faults are delivered.
* fpcr.rmode, which sets the rounding mode for floating point
operations.
The other one we could do is cpsr.btype, but it is not clear what would
be useful here so I'm not including it in this change.
It's not necessary because the broadcasters hold a weak_ptr (*) to it,
and will delete the weak_ptr next time they try to lock it. Doing this
prevents recursion in RemoveListener, where the function can end up
holding the only shared_ptr to a listener, and its destruction can
trigger another call to RemoveListener -- which will mess up the state
of the first instance.
This is the same bug that we've have fixed in
https://reviews.llvm.org/D23406, but it was effectively undone in
https://reviews.llvm.org/D157556. With the addition of a primary
listener, a fix like D23406 becomes unwieldy (and it has already shown
itself to be fragile), which is why this patch attempts a different
approach.
Like in 2016, I don't know a good way to unit test this bug, since it
depends on precise timing, but the thing I like about this approach is
that it enables us to change the broadcaster mutex into a non-recursive
one. While that doesn't prevent the bug from happening again, it will
make it much easier to spot in the future, as the code will hang with a
smoking gun (instead of crashing a little while later). I'm going to
attempt that in a separate patch to minimize disruption.
(*) Technically a broadcaster holds the *primary* listener as a
shared_ptr, but that's still ok as it means that listener will not get
destroyed until it is explicitly removed.
ObjectFileMachO::SetLoadAddress sets the address of each segment in a
binary in a Target, but it ignores segments that are not loaded in the
virtual address space. It was marking segments that were purely BSS --
having no content in the file, but in zero-initialized memory when
running in the virtual address space -- as not-loadable, unless they
were named "DATA". This works pretty well for typical userland binaries,
but in less Darwin environments, there may be BSS segments with other
names, that ARE loadable.
I looked at the origin of SectionIsLoadable's check for this, and it was
a cleanup by Greg in 2018 where we had three different implementations
of the idea in ObjectFileMachO and one of them skipped zero-file-size
segments (BSS), which made it into the centralized SectionIsLoadable
method.
Also add some logging to the DynamicLoader log channel when loading a
binary - it's the first place I look when debugging segment address
setting bugs, and it wasn't emitting anything.
rdar://129870649
This patch introduces a new top-level `scripting` command with an `run`
sub-command, that basically replaces the `script` raw command.
To avoid breaking the `script` command usages, this patch also adds an
`script` alias to the `scripting run` sub-command.
The reason behind this change is to have a top-level command that will
cover scripting related subcommands.
Signed-off-by: Med Ismail Bennani <ismail@bennani.ma>
This is an improved attempt to improve the semantics of SupportFile
equivalence, taking into account the feedback from #95606.
Pavel's comment about the lack of a concise name because the concept
isn't trivial made me realize that I don't want to abstract this concept
away behind a helper function. Instead, I opted for a rather verbose
enum that forces the caller to consider exactly what kind of comparison
is appropriate for every call.
This patch allows expressions to reference entities in anonymous
namespaces. Previously this would have resulted in:
```
(lldb) expr foo::FooAnonymousVar
error: <user expression 0>:1:6: no member named 'FooAnonymousVar' in namespace 'foo'
1 | foo::FooAnonymousVar
| ~~~~~^
```
We already allow such lookups through inline namespaces, and for the
purposes of lookup, anonymous namespaces shouldn't behave any different.
Fixes https://github.com/llvm/llvm-project/issues/96963.
