.. from the guts of GDBRemoteCommunication to ~top level.
This is motivated by #131519 and by the fact that's impossible to guess
whether the author of a symlink intended it to be a "convenience
shortcut" -- meaning it should be resolved before looking for related
files; or an "implementation detail" -- meaning the related files should
be located near the symlink itself.
This debate is particularly ridiculous when it comes to lldb-server
running in platform mode, because it also functions as a debug server,
so what we really just need to do is to pass /proc/self/exe in a
platform-independent manner.
Moving the location logic higher up achieves that as lldb-platform (on
non-macos) can pass `HostInfo::GetProgramFileSpec`, while liblldb can
use the existing complex logic (which only worked on liblldb anyway as
lldb-platform doesn't have a lldb_private::Platform instance).
Another benefit of this patch is a reduction in dependency from
GDBRemoteCommunication to the rest of liblldb (achieved by avoiding the
Platform dependency).
The function was extremely messy in that it, depending on the set of
arguments, it could either modify the Connection object in `this` or
not. It had a lot of arguments, with each call site passing a different
combination of null values. This PR:
- packs "url" and "comm_fd" arguments into a variant as they are
mutually exclusive
- removes the (surprising) "null url *and* null comm_fd" code path which
is not used as of https://github.com/llvm/llvm-project/pull/145017
- marks the function as `static` to make it clear it (now) does not
operate on the `this` object.
Depends on #145017
Originally added for reproducers, it is now only used for test code.
While we could make it a test helper, I think that after #145015 it is
simple enough to not be needed.
Also squeeze in a change to make ConnectionFileDescriptor accept a
unique_ptr<Socket>.
- use early exits where possible
- avoid the listen thread by using Socket APIs which allow separate
"listen" and "accept" steps
- use formatv-like log statements
There "should" be no functional changes from this patch.
I *think* this was the reason behind the failures in
2fd860c1f559c0b0be66cc000e38270a04d0a1a3: the clang include tool showed
the Config.h headers as unused, and because the macro was referenced
through an `#ifdef`, its removal didn't cause build failures. Switching
to `#cmakedefine01` + `#if` should make sure this does not happen again.
According to D48977, the `#ifndef`+`#cmakedefine` patterns is due to
some files redefining the macro themselves. I no longer see any such
files in the source tree (there also were no files like that in the
source tree at the revision mentioned, but the macro *was* defined in
the hand-maintained XCode project we had at the time).
This reverts commit 68ab45f0533f3bbfc1c96bddd53de7e769180219, reapplying
2fd860c1f559c0b0be66cc000e38270a04d0a1a3. The only change is keeping
"lldb/Host/Config.h", which I believe was the cause of the failures.
Move ReadPacketWithOutputSupport() from GDBRemoteCommunication
to GDBRemoteClientBase. This function is client-specific and moving
it there simplifies followup patches that split communication into
separate thread.
Sponsored by: The FreeBSD Foundation
Differential Revision: https://reviews.llvm.org/D135028
Move the broadcasting support from GDBRemoteCommunication
to GDBRemoteClientBase since this is where it is actually used. Remove
GDBRemoteCommunication and subclass constructor arguments left over
after Communication cleanup.
Sponsored by: The FreeBSD Foundation
Differential Revision: https://reviews.llvm.org/D133427
Split the read thread support from Communication into a dedicated
ThreadedCommunication subclass. The read thread support is used only
by a subset of Communication consumers, and it adds a lot of complexity
to the base class. Furthermore, having a dedicated subclass makes it
clear whether a particular consumer needs to account for the possibility
of read thread being running or not.
The modules currently calling `StartReadThread()` are updated to use
`ThreadedCommunication`. The remaining modules use the simplified
`Communication` class.
`SBCommunication` is changed to use `ThreadedCommunication` in order
to avoid changing the public API.
`CommunicationKDP` is updated in order to (hopefully) compile with
the new code. However, I do not have a Darwin box to test it, so I've
limited the changes to the bare minimum.
`GDBRemoteCommunication` is updated to become a `Broadcaster` directly.
Since it does not inherit from `ThreadedCommunication`, its event
support no longer collides with the one used for read thread and can
be implemented cleanly. The support for
`eBroadcastBitReadThreadDidExit` is removed from the code -- since
the read thread was not used, this event was never reported.
Sponsored by: The FreeBSD Foundation
Differential Revision: https://reviews.llvm.org/D133251
Implement the support for %Stop asynchronous notification packet format
in LLGS. This does not implement full support for non-stop mode for
threaded programs -- process plugins continue stopping all threads
on every event. However, it will be used to implement asynchronous
events in multiprocess debugging.
The non-stop protocol is enabled using QNonStop packet. When it is
enabled, the server uses notification protocol instead of regular stop
replies. Since all threads are always stopped, notifications are always
generated for all active threads and copied into stop notification
queue.
If the queue was empty, the initial asynchronous %Stop notification
is sent to the client immediately. The client needs to (eventually)
acknowledge the notification by sending the vStopped packet, in which
case it is popped from the queue and the stop reason for the next thread
is reported. This continues until notification queue is empty again,
in which case an OK reply is sent.
Asychronous notifications are also used for vAttach results and program
exits. The `?` packet uses a hybrid approach -- it returns the first
stop reason synchronously, and exposes the stop reasons for remaining
threads via vStopped queue.
The change includes a test case for a program generating a segfault
on 3 threads. The server is expected to generate a stop notification
for the segfaulting thread, along with the notifications for the other
running threads (with "no stop reason"). This verifies that the stop
reasons are correctly reported for all threads, and that notification
queue works.
Sponsored by: The FreeBSD Foundation
Differential Revision: https://reviews.llvm.org/D125575
Accept a function object instead of a raw pointer. This avoids a bunch
of boilerplate typically needed to pass arguments to the thread
functions.
Differential Revision: https://reviews.llvm.org/D120321
Remove the port predicate from Socket and ConnectionFileDescriptor,
and move it to gdb-remote. It is specifically relevant to the threading
used inside gdb-remote and with the new port callback API, we can
reliably move it there. While at it, switch from the custom Predicate
to std::promise/std::future.
Differential Revision: https://reviews.llvm.org/D112357
GDB uses normalized errno values for vFile errors. Implement
the translation between them and system errno values in the gdb-remote
plugin.
Differential Revision: https://reviews.llvm.org/D108148
Implement a fallback to getting the file size via vFile:stat packet
when the remote server does not implement vFile:size. This makes it
possible to query file sizes from remote gdbserver.
Note that unlike vFile:size, the fallback will not work if the server is
unable to open the file.
While at it, add a few tests for the 'platform get-size' command.
Differential Revision: https://reviews.llvm.org/D107780
The GDB replay server sanity-checks that every packet it receives
matches what it expects from the serialized packet log. This mechanism
tripped for TestReproducerAttach.py on Linux, because one of the packets
(jModulesInfo) uses run-length encoding. The replay server was comparing
the expanded incoming packet with the unexpanded packet in the log. As a
result, it claimed to have received an unexpected packet, which caused
the test to fail.
This patch addresses that issue by expanding the run-length encoding
before comparing the packets.
Differential revision: https://reviews.llvm.org/D76163
LLDB has a few different styles of header guards and they're not very
consistent because things get moved around or copy/pasted. This patch
unifies the header guards across LLDB and converts everything to match
LLVM's style.
Differential revision: https://reviews.llvm.org/D74743
On multiple retry this issue won't duplicate - will revisit with author if
duplication works again.
This reverts commit c9e0b354e2749ce7ab553974692cb35c8651a869.
This was causing a crash in opt+assert builds on linux and a follow-up
message was posted.
This reverts commit e81268d03e73aef4f9c7bd8ece8ad02f5b017dcf
When running the test suite with always capture on, a handful of tests
are failing because they have multiple targets and therefore multiple
GDB remote connections. The current reproducer infrastructure is capable
of dealing with that.
This patch reworks the GDB remote provider to support multiple GDB
remote connections, similar to how the reproducers support shadowing
multiple command interpreter inputs. The provider now keeps a list of
packet recorders which deal with a single GDB remote connection. During
replay we rely on the order of creation to match the number of packets
to the GDB remote connection.
Differential revision: https://reviews.llvm.org/D71105
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
Summary:
The field `m_decompression_scratch_type` is only used when `HAVE_LIBCOMPRESSION` is
defined, which caused a warning which I fixed in rLLDB350675 by just marking the variable as always used.
This patch fixes this in a better way by only defining the variable (and the related `m_decompression_scratch`
variable) when `HAVE_LIBCOMPRESSION` is defined. This also required changing the way we handle
`HAVE_LIBCOMPRESSION` works, as this was previously always defined on macOS within the source file
but not in the header. Now it's always defined from within our config header when CMake defines it or when
we are on macOS.
The field initialization was moved to the header to prevent that we have `#ifdef` within our initializer list.
Reviewers: #lldb, jasonmolenda, sgraenitz, labath
Reviewed By: labath
Subscribers: labath, beanz, mgorny, lldb-commits, dblaikie
Differential Revision: https://reviews.llvm.org/D57011
llvm-svn: 352175
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
systems. It has been available in the OS over over three years
now. If lldb doesn't link against -lcompression, it should be an
error.
Allocate a scratch buffer for libcompression to use when decoding
packets, instead of it having to allocate & free one on every call.
Fix a typeo with the size of the buffer that compression_decode_buffer()
is expanding into.
<rdar://problem/41601084>
llvm-svn: 349563
Summary:
These are general purpose "utility" classes, whose functionality is not
debugger-specific in any way. As such, I believe they belong in the
Utility module.
This doesn't break any particular dependency (yet), but it reduces the
number of Core dependencies across the board.
Reviewers: zturner, jingham, teemperor, clayborg
Subscribers: mgorny, lldb-commits
Differential Revision: https://reviews.llvm.org/D55361
llvm-svn: 349157
This patch removes the comments grouping header includes. They were
added after running IWYU over the LLDB codebase. However they add little
value, are often outdates and burdensome to maintain.
llvm-svn: 346626
Summary:
This class was initially in Host because its implementation used to be
very OS-specific. However, with C++11, it has become a very simple
std::condition_variable wrapper, with no host-specific code.
It is also a general purpose utility class, so it makes sense for it to
live in a place where it can be used by everyone.
This has no effect on the layering right now, but it enables me to later
move the Listener+Broadcaster+Event combo to a lower layer, which is
important, as these are used in a lot of places (notably for launching a
process in Host code).
Reviewers: jingham, zturner, teemperor
Reviewed By: zturner
Subscribers: xiaobai, mgorny, lldb-commits
Differential Revision: https://reviews.llvm.org/D50384
llvm-svn: 341089
Summary:
The Args class is used in plenty of places besides the command
interpreter (e.g., anything requiring an argc+argv combo, such as when
launching a process), so it needs to be in a lower layer. Now that the
class has no external dependencies, it can be moved down to the Utility
module.
This removes the last (direct) dependency from the Host module to
Interpreter, so I remove the Interpreter module from Host's dependency
list.
Reviewers: zturner, jingham, davide
Subscribers: mgorny, lldb-commits
Differential Revision: https://reviews.llvm.org/D45480
llvm-svn: 330200
While trying to use this header I noticed that it is not in the include
folder. Move it to there and update all #includes to reference that file
correctly.
llvm-svn: 327996
Summary:
Gdb servers like openocd may send many $O reply packets for the client to output during a qRcmd command sequence. Currently, lldb interprets the first O packet as an unexpected response. Besides generating no output, this causes lldb to get out of sync with future commands because it continues reading O packets from the first command as response to subsequent commands.
This patch handles any O packets during an qRcmd, treating the first non-O packet as the true response.
Preliminary discussion at http://lists.llvm.org/pipermail/lldb-dev/2018-January/013078.html
Reviewers: clayborg
Reviewed By: clayborg
Subscribers: labath, lldb-commits
Differential Revision: https://reviews.llvm.org/D41745
Patch by Owen Shaw <llvm@owenpshaw.net>
llvm-svn: 322190
Summary:
These tests used to log the error message and return plain bool mainly
because at the time they we written, we did not have a nice way to
assert on llvm::Error values. That is no longer true, so replace this
pattern with a more idiomatic approach.
As a part of this patch, I also move the formatting of
GDBRemoteCommunication::PacketResult values out of the test code, as
that can be useful elsewhere.
Reviewers: zturner, eugene
Subscribers: mgorny, lldb-commits
Differential Revision: https://reviews.llvm.org/D39790
llvm-svn: 317795
This renames the LLDB error class to Status, as discussed
on the lldb-dev mailing list.
A change of this magnitude cannot easily be done without
find and replace, but that has potential to catch unwanted
occurrences of common strings such as "Error". Every effort
was made to find all the obvious things such as the word "Error"
appearing in a string, etc, but it's possible there are still
some lingering occurences left around. Hopefully nothing too
serious.
llvm-svn: 302872
This replaces the raw integer timeout parameters in the class with their
chrono-based equivalents. To achieve this, I have moved the Timeout class to a
more generic place and added a quick unit test for it.
llvm-svn: 287920
the chrono library there uses long long as the underlying chrono type, but
defines int64_t as long (or the other way around, I am not sure). In any case,
this caused the implicit conversion to not trigger. This should address that.
Also fix up the relevant unit test.
llvm-svn: 287867
Summary:
This replaces the usage of raw integers with duration classes in the gdb-remote
packet management functions. The values are still converted back to integers once
they go into the generic Communication class -- that I am leaving to a separate
change.
The changes are mostly straight-forward (*), the only tricky part was
representation of infinite timeouts.
Currently, we use UINT32_MAX to denote infinite timeout. This is not well suited
for duration classes, as they tend to do arithmetic on the values, and the
identity of the MAX value can easily get lost (e.g.
microseconds(seconds(UINT32_MAX)).count() != UINT32_MAX). We cannot use zero to
represent infinity (as Listener classes do) because we already use it to do
non-blocking polling reads. For this reason, I chose to have an explicit value
for infinity.
The way I achieved that is via llvm::Optional, and I think it reads quite
natural. Passing llvm::None as "timeout" means "no timeout", while passing zero
means "poll". The only tricky part is this breaks implicit conversions (seconds
are implicitly convertible to microseconds, but Optional<seconds> cannot be
easily converted into Optional<microseconds>). For this reason I added a special
class Timeout, inheriting from Optional, and enabling the necessary conversions
one would normally expect.
(*) The other tricky part was GDBRemoteCommunication::PopPacketFromQueue, which
was needlessly complicated. I've simplified it, but that one is only used in
non-stop mode, and so is untested.
Reviewers: clayborg, zturner, jingham
Subscribers: lldb-commits
Differential Revision: https://reviews.llvm.org/D26971
llvm-svn: 287864
Summary:
Most of the changes are very straight-forward, the only tricky part was the
"packet speed-test" function, which is very time-heavy. As the function was
completely untested, I added a quick unit smoke test for it.
Reviewers: clayborg, zturner
Subscribers: lldb-commits
Differential Revision: https://reviews.llvm.org/D25391
llvm-svn: 285602
*** to conform to clang-format’s LLVM style. This kind of mass change has
*** two obvious implications:
Firstly, merging this particular commit into a downstream fork may be a huge
effort. Alternatively, it may be worth merging all changes up to this commit,
performing the same reformatting operation locally, and then discarding the
merge for this particular commit. The commands used to accomplish this
reformatting were as follows (with current working directory as the root of
the repository):
find . \( -iname "*.c" -or -iname "*.cpp" -or -iname "*.h" -or -iname "*.mm" \) -exec clang-format -i {} +
find . -iname "*.py" -exec autopep8 --in-place --aggressive --aggressive {} + ;
The version of clang-format used was 3.9.0, and autopep8 was 1.2.4.
Secondly, “blame” style tools will generally point to this commit instead of
a meaningful prior commit. There are alternatives available that will attempt
to look through this change and find the appropriate prior commit. YMMV.
llvm-svn: 280751
This started as an effort to change StringExtractor to store a
StringRef internally instead of a std::string. I got that working
locally with just 1 test failure which I was unable to figure out the
cause of. But it was also a massive changelist due to a trickle
down effect of changes.
So I'm starting over, using what I learned from the first time to
tackle smaller, more isolated changes hopefully leading up to
a full conversion by the end.
At first the changes (such as in this CL) will seem mostly
a matter of preference and pointless otherwise. However, there
are some places in my larger CL where using StringRef turned 20+
lines of code into 2, drastically simplifying logic. Hopefully
once these go in they will illustrate some of the benefits of
thinking in terms of StringRef.
llvm-svn: 279917
Despite its comment, the function is only used in the Client class, and its presence was merely
complicating mock implementation in unit tests.
llvm-svn: 278785
This change opens a socket pair and passes the second socket pair file descriptor down to the debugserver binary using a new option: "--fd=N" where N is the file descriptor. This file descriptor gets passed via posix_spawn() so that there is no need to do any bind/listen or bind/accept calls and eliminates the hanshake unix socket that is used to pass the result of the actual port that ends up being used so it can save time on launch as well as being faster.
This is currently only enabled on __APPLE__ builds. Other OSs should try modifying the #define from ProcessGDBRemote.cpp but the first person will need to port the --fd option over to lldb-server. Any OSs that enable USE_SOCKETPAIR_FOR_LOCAL_CONNECTION in their native builds can use the socket pair stuff. The #define is Apple only right now, but looks like:
#if defined (__APPLE__)
#define USE_SOCKETPAIR_FOR_LOCAL_CONNECTION 1
#endif
<rdar://problem/27814880>
llvm-svn: 278524
Resumbitting the commit after fixing the following problems:
- broken unit tests on windows: incorrect gtest usage on my part (TEST vs. TEST_F)
- the new code did not correctly handle the case where we went to interrupt the process, but it
stopped due to a different reason - the interrupt request would remain queued and would
interfere with the following "continue". I also added a unit test for this case.
This reapplies r277156 and r277139.
llvm-svn: 278118
