# Lldb-server crash
We have seen stacks like the following in lldb-server core dumps:
```
[
"__GI___pthread_kill at pthread_kill.c:46",
"__GI_raise at raise.c:26",
"__GI_abort at abort.c:100",
"__assert_fail_base at assert.c:92",
"__GI___assert_fail at assert.c:101",
"lldb_private::NativeProcessProtocol::RemoveSoftwareBreakpoint(unsigned long) at /redacted/lldb-server:0"
]
```
# Hypothesis of root cause
In `NativeProcessProtocol::RemoveSoftwareBreakpoint()`
([code](19b2dd9d79/lldb/source/Host/common/NativeProcessProtocol.cpp (L359-L423))),
a `ref_count` is asserted and reduced. If it becomes zero, the code
first go through a series of memory reads and writes to remove the
breakpoint trap opcode and to restore the original process code, then,
if everything goes fine, removes the entry from the map
`m_software_breakpoints` at the end of the function.
However, if any of the validations for the above reads and writes goes
wrong, the code returns an error early, skipping the removal of the
entry. This leaves the entry behind with a `ref_count` of zero.
The next call to `NativeProcessProtocol::RemoveSoftwareBreakpoint()` for
the same breakpoint[*] would violate the assertion about `ref_count > 0`
([here](19b2dd9d79/lldb/source/Host/common/NativeProcessProtocol.cpp (L365))),
which would cause a crash.
[*] We haven't found a *regular* way to repro such a next call in lldb
or lldb-dap. This is because both of them remove the breakpoint from
their internal list when they get any response from the lldb-server (OK
or error). Asking the client to delete the breakpoint a second time
doesn't trigger the client to send the `$z` gdb packet to lldb-server.
We are able to trigger the crash by sending the `$z` packet directly,
see "Manual test" below.
# Fix
Lift the removal of the map entry to be immediately after the decrement
of `ref_count`, before the early returns. This ensures that the asserted
case will never happen. The validation errors can still happen, and
whether they happen or not, the breakpoint has been removed from the
perspective of the lldb-server (same as that of lldb and lldb-dap).
# Manual test & unit test
See PR.
Summary:
A *.cpp file header in LLDB (and in LLDB) should like this:
```
//===-- TestUtilities.cpp -------------------------------------------------===//
```
However in LLDB most of our source files have arbitrary changes to this format and
these changes are spreading through LLDB as folks usually just use the existing
source files as templates for their new files (most notably the unnecessary
editor language indicator `-*- C++ -*-` is spreading and in every review
someone is pointing out that this is wrong, resulting in people pointing out that this
is done in the same way in other files).
This patch removes most of these inconsistencies including the editor language indicators,
all the different missing/additional '-' characters, files that center the file name, missing
trailing `===//` (mostly caused by clang-format breaking the line).
Reviewers: aprantl, espindola, jfb, shafik, JDevlieghere
Reviewed By: JDevlieghere
Subscribers: dexonsmith, wuzish, emaste, sdardis, nemanjai, kbarton, MaskRay, atanasyan, arphaman, jfb, abidh, jsji, JDevlieghere, usaxena95, lldb-commits
Tags: #lldb
Differential Revision: https://reviews.llvm.org/D73258
This reverts commit a7335393f50246b59db450dc6005f7c8f29e73a6.
It seems this is breaking a bunch of tests (https://reviews.llvm.org/D62503#1549874) so reverting until I find the time to repro and fix.
llvm-svn: 364355
Summary:
This is the fifth patch to improve module loading in a series that started here (where I explain the motivation and solution): D62499
Reading strings with ReadMemory is really slow when reading the path of the shared library. This is because we don't know the length of the path so use PATH_MAX (4096) and these strings are actually super close to the boundary of an unreadable page. So even though we use process_vm_readv it will usually fail because the read size spans to the unreadable page and we then default to read the string word by word with ptrace.
This new function is very similar to another ReadCStringFromMemory that already exists in lldb that makes sure it never reads cross page boundaries and checks if we already read the entire string by finding '\0'.
I was able to reduce the GetLoadedSharedLibraries call from 30ms to 4ms (or something of that order).
Reviewers: clayborg, xiaobai, labath
Reviewed By: labath
Subscribers: emaste, lldb-commits
Tags: #lldb
Differential Revision: https://reviews.llvm.org/D62503
llvm-svn: 363750
Summary:
This is the third patch to improve module loading in a series that started here (where I explain the motivation and solution): D62499
Add functions to read the r_debug location to know where the linked list of loaded libraries are so I can generate the `xfer:libraries-svr4` packet.
I'm also using this function to implement `GetSharedLibraryInfoAddress` that was "not implemented" for linux.
Most of this code was inspired by the current ds2 implementation here: https://github.com/facebook/ds2/blob/master/Sources/Target/POSIX/ELFProcess.cpp.
Reviewers: clayborg, xiaobai, labath
Reviewed By: clayborg, labath
Subscribers: emaste, krytarowski, mgorny, lldb-commits
Tags: #lldb
Differential Revision: https://reviews.llvm.org/D62501
llvm-svn: 363458
to reflect the new license.
We understand that people may be surprised that we're moving the header
entirely to discuss the new license. We checked this carefully with the
Foundation's lawyer and we believe this is the correct approach.
Essentially, all code in the project is now made available by the LLVM
project under our new license, so you will see that the license headers
include that license only. Some of our contributors have contributed
code under our old license, and accordingly, we have retained a copy of
our old license notice in the top-level files in each project and
repository.
llvm-svn: 351636
The warning comes from the fact that the MOCK_METHOD macros don't use the
override keyword internally. This makes us not use it in the manually overriden
methods either, to be consistent.
llvm-svn: 350209
The assertion fired (with a debug visual studio STL) because we tried to
dereference the end of a vector (although it was only to take its
address again and form an end iterator). Rewrite this logic to avoid the
questionable code.
llvm-svn: 350091
NativeProcessProtocol::ReadMemoryWithoutTrap had a bug, where it failed
to properly remove inserted breakpoint opcodes if the memory read
partially overlapped the trap opcode. This could not happen on x86
because it has a one-byte breakpoint instruction, but it could happen on
arm, which has a 4-byte breakpoint instruction (in arm mode).
Since triggerring this condition would only be possible on an arm
machine (and even then it would be a bit tricky). I test this using a
NativeProcessProtocol unit test.
llvm-svn: 343076
Summary:
NativeProcessProtocol is an abstract class, but it still contains a
significant amount of code. Some of that code is tested via tests of
specific derived classes, but these tests don't run everywhere, as they
are OS and arch-specific. They are also relatively high-level, which
means some functionalities (particularly the failure cases) are
hard/impossible to test.
In this approach, I replace the abstract methods with mocks, which
allows me to inject failures into the lowest levels of breakpoint
setting code and test the class behavior in this situation.
Reviewers: zturner, teemperor
Subscribers: mgorny, lldb-commits
Differential Revision: https://reviews.llvm.org/D52152
llvm-svn: 342875
