Summary:
This is the second patch to improve module loading in a series that started here (where I explain the motivation and solution): https://reviews.llvm.org/D62499
I need to read the aux vector to know where the r_debug map with the loaded libraries are.
The AuxVector class was made generic so it could be reused between the POSIX-DYLD plugin and NativeProcess*. The class itself ended up in the ProcessUtility plugin.
Reviewers: clayborg, xiaobai, labath, JDevlieghere
Reviewed By: clayborg, labath, JDevlieghere
Subscribers: emaste, JDevlieghere, mgorny, lldb-commits
Tags: #lldb
Differential Revision: https://reviews.llvm.org/D62500
llvm-svn: 363098
Summary:
This is the first of a few patches I have to improve the performance of dynamic module loading on Android.
In this first diff I'll describe the context of my main motivation and will then link to it in the other diffs to avoid repeating myself.
## Motivation
I have a few scenarios where opening a specific feature on an Android app takes around 40s when lldb is attached to it. The reason for that is because 40 modules are dynamicly loaded at that point in time and each one of them is taking ~1s.
## The problem
To learn about new modules we have a breakpoint on a linker function that is called twice whenever a module is loaded. One time just before it's loaded (so lldb can check which modules are loaded) and another right after it's loaded (so lldb can check again which ones are loaded and calculate the diference).
It's figuring out which modules are loaded that is taking quite some time. This is currently done by traversing the linked list of loaded shared libraries that the linker maintains in memory. Each item in the linked list requires its own `x` packet sent to the gdb server (this is android so the network also plays a part). In my scenario there are 400+ loaded libraries and even though we read 0x800 worth of bytes at a time we still make ~180 requests that end up taking 150-200ms.
We also do this twice, once before the module is loaded (state = eAdd) and another right after (state = eConsistent) which easly adds up to ~400ms per module.
## A solution
**Implement `xfer:libraries-svr4` in lldb-server:**
I noticed in the code that loads the new modules that it had support for the `xfer:libraries-svr4` packet (added ~4 years ago to support the ds2 debug server) but we didn't support it in lldb-server. This single packet returns an xml list of all the loaded modules by the process. The advantage is that there's no more need to make 180 requests to read the linked list. Additionally this new requests takes around 10ms.
**More efficient usage of the `xfer:libraries-svr4` packet in lldb:**
When `xfer:libraries-svr4` is available the Process class has a `LoadModules` function that requests this packet and then loads or unloads modules based on the current list of loaded modules by the process.
This is the function that is used by the DYLDRendezvous class to get the list of loaded modules before and after the module is loaded. However, this is really not needed since the LoadModules function already loaded or unloaded the modules accordingly. I changed this strategy to call LoadModules only once (after the process has loaded the module).
**Bugs**
I found a few issues in lldb while implementing this and have submitted independent patches for them.
I tried to devide this into multiple logical patches to make it easier to review and discuss.
## Tests
I wanted to put these set of diffs up before having all the tests up and running to start having them reviewed from a techical point of view. I'm also having some trouble making the tests running on linux so I need more time to make that happen.
# This diff
The `xfer` packages follow the same protocol, they are requested with `xfer:<object>:<read|write>:<annex>:<offset,length>` and a return that starts with `l` or `m` depending if the offset and length covers the entire data or not. Before implementing the `xfer:libraries-svr4` I refactored the `xfer:auxv` to generically handle xfer packets so we can easly add new ones.
The overall structure of the function ends up being:
* Parse the packet into its components: object, offset etc.
* Depending on the object do its own logic to generate the data.
* Return the data based on its size, the requested offset and length.
Reviewers: clayborg, xiaobai, labath
Reviewed By: labath
Subscribers: mgorny, krytarowski, lldb-commits
Tags: #lldb
Differential Revision: https://reviews.llvm.org/D62499
llvm-svn: 362982
Ensure that errors are passed through correctly when performing
register read/write operations. Currently, any ptrace() errors are
silently discarded and LLDB behaves as if operation was successful.
Differential Revision: https://reviews.llvm.org/D63054
llvm-svn: 362946
Summary:
My main goal here is to make lldb-server work with Android Studio.
This is currently not the case because lldb-server is started in platform mode listening on a domain socket. When Android Studio connects to it lldb-server crashes because even though it's listening on a domain socket as soon as it gets a connection it asserts that it's a TCP connection, which will obviously fails for any non-tcp connection.
To do this I came up with a new method called GetConnectURI() in Socket that returns the URI needed to connect to the connected portion of the socket.
Reviewers: labath, clayborg, xiaobai
Reviewed By: labath
Subscribers: mgorny, jfb, lldb-commits
Tags: #lldb
Differential Revision: https://reviews.llvm.org/D62089
llvm-svn: 362173
Summary:
Previous patch (r360409) introduced the "symbol file unwind plan"
concept, but that plan wasn't used for unwinding yet. With this patch,
we start to consider the new plan as a possible strategy for both
synchronous and asynchronous unwinding. I also add a test that asserts
that unwinding via breakpad STACK CFI info works end-to-end.
Reviewers: jasonmolenda, clayborg
Subscribers: lldb-commits, amccarth, markmentovai
Differential Revision: https://reviews.llvm.org/D61853
llvm-svn: 361618
Summary:
On Windows `lldb::thread_result_t` resolves to `typedef unsigned thread_result_t;` and on other platforms it resolves to `typedef void *thread_result_t;`.
Therefore one cannot use `nullptr` when returning from a function that returns `thread_result_t`.
I've made this change because a windows build bot fails with these errors:
```
E:\build_slave\lldb-x64-windows-ninja\llvm\tools\lldb\source\Core\Communication.cpp(362): error C2440: 'return': cannot convert from 'nullptr' to 'lldb::thread_result_t'
E:\build_slave\lldb-x64-windows-ninja\llvm\tools\lldb\source\Core\Communication.cpp(362): note: A native nullptr can only be converted to bool or, using reinterpret_cast, to an integral type
```
and
```
E:\build_slave\lldb-x64-windows-ninja\llvm\tools\lldb\source\Core\Debugger.cpp(1619): error C2440: 'return': cannot convert from 'nullptr' to 'lldb::thread_result_t'
E:\build_slave\lldb-x64-windows-ninja\llvm\tools\lldb\source\Core\Debugger.cpp(1619): note: A native nullptr can only be converted to bool or, using reinterpret_cast, to an integral type
E:\build_slave\lldb-x64-windows-ninja\llvm\tools\lldb\source\Core\Debugger.cpp(1664): error C2440: 'return': cannot convert from 'nullptr' to 'lldb::thread_result_t'
E:\build_slave\lldb-x64-windows-ninja\llvm\tools\lldb\source\Core\Debugger.cpp(1664): note: A native nullptr can only be converted to bool or, using reinterpret_cast, to an integral type
```
This is the failing build: http://lab.llvm.org:8011/builders/lldb-x64-windows-ninja/builds/5035/steps/build/logs/stdio
Reviewers: JDevlieghere, teemperor, jankratochvil, labath, clayborg, RKSimon, courbet, jhenderson
Reviewed By: labath, clayborg
Subscribers: labath, lldb-commits
Tags: #lldb
Differential Revision: https://reviews.llvm.org/D62305
llvm-svn: 361503
Summary:
NFC = [[ https://llvm.org/docs/Lexicon.html#nfc | Non functional change ]]
This commit is the result of modernizing the LLDB codebase by using
`nullptr` instread of `0` or `NULL`. See
https://clang.llvm.org/extra/clang-tidy/checks/modernize-use-nullptr.html
for more information.
This is the command I ran and I to fix and format the code base:
```
run-clang-tidy.py \
-header-filter='.*' \
-checks='-*,modernize-use-nullptr' \
-fix ~/dev/llvm-project/lldb/.* \
-format \
-style LLVM \
-p ~/llvm-builds/debug-ninja-gcc
```
NOTE: There were also changes to `llvm/utils/unittest` but I did not
include them because I felt that maybe this library shall be updated in
isolation somehow.
NOTE: I know this is a rather large commit but it is a nobrainer in most
parts.
Reviewers: martong, espindola, shafik, #lldb, JDevlieghere
Reviewed By: JDevlieghere
Subscribers: arsenm, jvesely, nhaehnle, hiraditya, JDevlieghere, teemperor, rnkovacs, emaste, kubamracek, nemanjai, ki.stfu, javed.absar, arichardson, kbarton, jrtc27, MaskRay, atanasyan, dexonsmith, arphaman, jfb, jsji, jdoerfert, lldb-commits, llvm-commits
Tags: #lldb, #llvm
Differential Revision: https://reviews.llvm.org/D61847
llvm-svn: 361484
While here, update some ppc64le specific check to isPPC64(), if it
applies to big-endian as well, in the hope that it will ease the support
of big-endian if people are interested in this area. The big-endian
variant is used by at least FreeBSD, Gentoo Linux, Adélie Linux, and
Void Linux.
llvm-svn: 360868
This can cause us to return paths to files on the local filesystem even
if we don't end up using that file (for instance because the file is not
a real module).
llvm-svn: 360432
Summary:
This behavior is specified in the Section 6.4.2.3 (Register Rule
instructions) of the DWARF4 spec. We were not doing that, which meant
that any register rule which was relying on the cfa value being there
was not evaluated correctly (it was aborted due to "out of bounds"
access).
I'm not sure how come this wasn't noticed before, but I guess this has
something to do with the fact that dwarf unwind expressions are not used
very often, and when they are, the situation is so complicated that the
CFA is of no use. I noticed this when I started emitting dwarf
expressions for the unwind information present in breakpad symbol files.
Reviewers: jasonmolenda, clayborg
Subscribers: aprantl, lldb-commits
Differential Revision: https://reviews.llvm.org/D61018
llvm-svn: 360158
Summary:
According to [C128] "Virtual functions should specify exactly one
of `virtual`, `override`, or `final`", I've added override where a
virtual function is overriden but the explicit `override` keyword
was missing. Whenever both `virtual` and `override` were specified,
I removed `virtual`. As C.128 puts it:
> [...] writing more than one of these three is both redundant and
> a potential source of errors.
I anticipate a discussion about whether or not to add `override` to
destructors but I went for it because of an example in [ISOCPP1000].
Let me repeat the comment for you here:
Consider this code:
```
struct Base {
virtual ~Base(){}
};
struct SubClass : Base {
~SubClass() {
std::cout << "It works!\n";
}
};
int main() {
std::unique_ptr<Base> ptr = std::make_unique<SubClass>();
}
```
If for some odd reason somebody removes the `virtual` keyword from the
`Base` struct, the code will no longer print `It works!`. So adding
`override` to destructors actively protects us from accidentally
breaking our code at runtime.
[C128]: https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#c128-virtual-functions-should-specify-exactly-one-of-virtual-override-or-final
[ISOCPP1000]: https://github.com/isocpp/CppCoreGuidelines/issues/1000#issuecomment-476951555
Reviewers: teemperor, JDevlieghere, davide, shafik
Reviewed By: teemperor
Subscribers: kwk, arphaman, kadircet, lldb-commits
Tags: #lldb
Differential Revision: https://reviews.llvm.org/D61440
llvm-svn: 359868
logging messages that are written the same, making it difficult to
know for certain which code path was taken based on a logfile. Add
some words to make each unique.
Right now the ordering for finding a FullUnwindPlan (ignoring
fallback unwind plan logic) is
1. If this is a _sigtramp like function, try eh_frame which is
hand written on darwin systems to account for finding the
saved register context correctly.
2. Ask the DynamicLoader if eh_frame should be preferred for
this frame. Some binaries on the system may have hand-written
eh_frame and the DynamicLoader is the source for this. (primarily
this is for hand-written assembly in the objc runtime, and we tell
lldb to trust that for functions in libobjc.dylib.)
3. if 0th frame, use GetUnwindPlanAtNonCallSite plan.
4. GetUnwindPlanAtCallSite {for 0th or any other}
5. GetUnwindPlanAtNonCallSite {now for non-0th frames, only if not from a compiler? hm.}
6. GetUnwindPlanArchitectureDefaultAtFunctionEntry if we're on the first instruction
7. Architectural default unwind plan ABI::CreateDefaultUnwindPlan
I'm moving #6 -- DefaultAtFunctionEntry -- up to between #3 and #4,
where we're already doing things specific to the zeroth frame. If
we're on the zeroth frame and the GetUnwindPlanAtNonCallSite plan
has failed for some reason, and we're on the first instruction, we
should definitely use DefaultAtFunctionEntry instead of any other
unwind plan. If we're trying to step out of some rando function
on the system that we couldn't assembly instruction inspect, this
is sufficient for us to step out of it.
llvm-svn: 359847
Address an ambiguity between lldb_private::Thread and
llvm::minidump::Thread. Follow-up to llvm r359762 (which introduced the
second type).
llvm-svn: 359765
Fix bugs in piod_len return value processing in ReadMemory()
and WriteMemory() methods. In particular, add support for piod_len == 0
indicating EOF, and fix summing bytes_read/bytes_written when PT_IO does
partial reads/writes.
The EOF condition could happen if LLDB attempts to read past
vm.maxaddress, e.g. as a result of RBP containing large (invalid) value.
Previously, the 0 return caused the function to retry reading via PT_IO
indefinitely, effectively deadlooping lldb-server.
Partial reads probably did not occur in practice, yet they would cause
ReadMemory() to return incorrect bytes_read and/or overwrite previously
read data.
WriteMemory() suffered from analoguous problems.
Differential Revision: https://reviews.llvm.org/D61310
llvm-svn: 359572
Summary:
Dump more information about "access violation" and "in page error" exceptions to
description. Description now contains data about read/write violation type and
actual address as described at
https://docs.microsoft.com/en-us/windows/desktop/api/winnt/ns-winnt-_exception_record
Reviewers: asmith, stella.stamenova
Reviewed By: stella.stamenova
Subscribers: teemperor, amccarth, abidh, lldb-commits, aleksandr.urakov
Tags: #lldb
Differential Revision: https://reviews.llvm.org/D60519
llvm-svn: 359420
This is part two of the change started in r359330. This patch moves the
ownership of the script interpreter from the command interpreter into
the debugger. I would've preferred to remove the lazy initialization,
however the fact that the scripting language is set after the debugger
is created makes that tricky. So for now this does exactly the same
thing as when it was under the command interpreter. The result is that
this patch is fully NFC.
Differential revision: https://reviews.llvm.org/D61211
llvm-svn: 359354
Summary:
This is needed for gcc/cstdlib++ 5.4.0, where __get_cpuid_count is not
defined in cpuid.h.
Reviewers: labath
Reviewed By: labath
Subscribers: lldb-commits
Tags: #lldb
Differential Revision: https://reviews.llvm.org/D61036
llvm-svn: 359120
Summary:
This argument was added back in 2010 (r118882) to support the ability to unwind
from functions whose eh_frame entry does not cover the entire range of
the function.
However, due to the caching happening in FuncUnwinders, this solution is
very fragile. FuncUnwinders will cache the plan it got from eh_frame
regardless of the value of the current_offset, so our ability to unwind
from a given function depended what was the value of "current_offset" the
first time that this function was called.
Furthermore, since the "image show-unwind" command did not know what's
the right offset to pass, this created an unfortunate situation where
"image show-unwind" would show no valid plans for a function, even
though they were available and being used.
In this patch I implement the feature slightly differently. Instead of
giving just a base address to the eh_frame unwinder, I give it the
entire range we are interested in. Then, I change the unwinder to return
the first plan that covers (even partially) that range. This way even a
partial plan will be returned, regardless of the address in the function
where we are stopped at.
This solution is still not 100% correct, as it will not handle a
function which is covered by two independent fde entries. However, I
don't expect anybody will write this kind of functions, and this wasn't
handled by the previous implementation either. If this is ever needed in
the future. The eh_frame unwinder can be extended to return "composite"
unwind plans created by merging sevelar fde entries.
I also create a test which triggers this scenario. As doing this is
virtually impossible without hand-written assembly, the test only works
on x86 linux.
Reviewers: jasonmolenda, clayborg
Subscribers: lldb-commits
Differential Revision: https://reviews.llvm.org/D60829
llvm-svn: 358964
This was updated in r356703 to use llvm::sys::RetryAfterSignal, which
comes from llvm/Support/Errno.h. The header wasn't added, so it fails if
you compile for arm64/aarch64.
llvm-svn: 358530
Fix mistake that mapped mm* registers into the space for xmm* registers,
rather than the one shared with st* registers. In other words,
'register read mmN' now correctly shows the mmN register rather than
part of xmmN.
Includes a minimal lit regression test.
Differential Revision: https://reviews.llvm.org/D60325
llvm-svn: 358178
Summary:
D59433 added code to swap bytes UUIDs coming from minidump files, but
only enabled it for apple platforms. Based on my research, I believe
this is the correct thing to do for windows as well, as the natural way
of printing U(G)UIDs on this platforms is to print the first three
components as (4 or 2)-byte integers printed in natural (big-endian)
order. This makes the UUID string coming out of lldb match the strings
produced by other windows tools.
The decision to byte-swap the age field is somewhat arbitrary, because
the age field is usually printed separately from the file GUID (and
often in decimal). However, for our purposes (telling whether two files
are identical), including it in the UUID is correct, and printing it in
big-endian makes it easier to recognize the age value.
This also makes the UUIDs generated here (almost) match up with the
UUIDs computed for breakpad symbol files
(BreakpadRecords.cpp:parseModuleId), which already implemented the
byte-swapping. The "almost" is here because ObjectFileBreakpad does not
swap the age field, but I'll fix that in a follow-up.
There is no UUID support in ObjectFileCOFF at the moment, but ideally
the algorithms used here and in ObjectFileCOFF should be in sync so that
object file matching works correctly.
Reviewers: clayborg, amccarth, markmentovai, asmith
Subscribers: lldb-commits
Differential Revision: https://reviews.llvm.org/D60501
llvm-svn: 358169
A lot of comments in LLDB are surrounded by an ASCII line to delimit the
begging and end of the comment.
Its use is not really consistent across the code base, sometimes the
lines are longer, sometimes they are shorter and sometimes they are
omitted. Furthermore, it looks kind of weird with the 80 column limit,
where the comment actually extends past the line, but not by much.
Furthermore, when /// is used for Doxygen comments, it looks
particularly odd. And when // is used, it incorrectly gives the
impression that it's actually a Doxygen comment.
I assume these lines were added to improve distinguishing between
comments and code. However, given that todays editors and IDEs do a
great job at highlighting comments, I think it's worth to drop this for
the sake of consistency. The alternative is fixing all the
inconsistencies, which would create a lot more churn.
Differential revision: https://reviews.llvm.org/D60508
llvm-svn: 358135
In this patch, I just remove the structure definitions for the
ModuleList stream and the associated parsing code. The rest of the code
is converted to work with the definitions in llvm. NFC.
llvm-svn: 358070
Add a flag to control whether the ModulesDidLoad notification is
called when a module is added. If the notifications are disabled,
the caller must call ModulesDidLoad after adding all the new modules,
but postponing this notification until they're all batched up can
allow for better efficiency than notifying one-by-one.
Change the name of the ModuleList notifier functions that a subclass
can implement to start with 'Notify' to make it clear what they are.
Add a NotifyModulesRemoved.
Add header documentation for the changed/updated methods.
Added defaulted-value 'notify' argument to ModuleList Append,
AppendIfNeeded, and Remove because callers working with a local
ModuleList don't have an obvious idea of what notify means in this
context. When the ModuleList is a part of the Target class, the
notify behavior matters.
DynamicLoaderDarwin has been updated so that libraries being
added/removed are correctly batched up before notifications are
sent. Added the TestModuleLoadedNotifys.py test to run on
Darwin to test this.
<rdar://problem/48293064>
Differential Revision: https://reviews.llvm.org/D60172
llvm-svn: 357955
This is a follow-up to r357829 (https://reviews.llvm.org/D60340) to
see whether increasing the packet timeout for non-asan builds could
also positively affect the stability of non-asan bots.
llvm-svn: 357954
I also update the tests for SystemInfo parsing to use the yaml2minidump
capabilities in llvm instead of relying on checked-in binaries.
llvm-svn: 357896
Since these timeouts guard against catastrophic error in debugserver,
I also increased all of them to the maximum value among them.
The motivation for this test was the observation that an asanified
LLDB would often exhibit seemingly random test failures that could be
traced back to debugserver packets getting out of sync. With this path
applied I can no longer reproduce the one particular failure mode that
I was investigating.
rdar://problem/49441261
Differential Revision: https://reviews.llvm.org/D60340
llvm-svn: 357829
This patch removes the lower layers of the minidump parsing code from
the MinidumpParser class, and replaces it with the minidump parser in
llvm.
Not all functionality is already avaiable in the llvm class, but it is
enough for us to be able to stop enumerating streams manually, and rely
on the minidump directory parsing code from the llvm class.
This also removes some checked-in binaries which were used to test error
handling in the parser, as the error handling is now done (and tested)
in llvm. Instead I just add one test that ensures we correctly propagate
the errors reported by the llvm parser. The input for this test can be
written in yaml instead of a checked-in binary.
llvm-svn: 357748
Allow partial UUID matching in Minidump core file plug-in
Breakpad had bugs in earlier versions where it would take a 20 byte ELF build ID and put it into the minidump file as a 16 byte PDB70 UUID with an age of zero. This would make it impossible to do postmortem debugging with one of these older minidump files.
This fix allows partial matching of UUIDs. To do this we first try and match with the full UUID value, and then fall back to removing the original directory path from the module specification and we remove the UUID requirement, and then manually do the matching ourselves. This allows scripts to find symbols files using a symbol server, place them all in a directory, use the "setting set target.exec-search-paths" setting to specify the directory, and then load the core file. The Target::GetSharedModule() can then find the correct file without doing any other matching and load it.
Tests were added to cover a partial UUID match where the breakpad file has a 16 byte UUID and the actual file on disk has a 20 byte UUID, both where the first 16 bytes match, and don't match.
Differential Revision: https://reviews.llvm.org/D60001
llvm-svn: 357603
See discussion in https://reviews.llvm.org/D60001.
Revert Clean up windows build bot.
This reverts r357504 (git commit 380c2420ec)
Revert Fix buildbot where paths were not matching up.
This reverts r357491 (git commit 5050586860)
Revert Allow partial UUID matching in Minidump core file plug-in
This reverts r357482 (git commit 838bba9c34)
llvm-svn: 357534
Breakpad had bugs in earlier versions where it would take a 20 byte ELF build ID and put it into the minidump file as a 16 byte PDB70 UUID with an age of zero. This would make it impossible to do postmortem debugging with one of these older minidump files.
This fix allows partial matching of UUIDs. To do this we first try and match with the full UUID value, and then fall back to removing the original directory path from the module specification and we remove the UUID requirement, and then manually do the matching ourselves. This allows scripts to find symbols files using a symbol server, place them all in a directory, use the "setting set target.exec-search-paths" setting to specify the directory, and then load the core file. The Target::GetSharedModule() can then find the correct file without doing any other matching and load it.
Tests were added to cover a partial UUID match where the breakpad file has a 16 byte UUID and the actual file on disk has a 20 byte UUID, both where the first 16 bytes match, and don't match.
Differential Revision: https://reviews.llvm.org/D60001
llvm-svn: 357482
Include support for NetBSD core dumps from evbarm/aarch64 system,
and matching test cases for them.
Based on earlier work by Kamil Rytarowski.
Differential Revision: https://reviews.llvm.org/D60034
llvm-svn: 357399
Antonio Afonso