By inspection, I noticed that the previous patch went too far, here:
@@ -7705,7 +7713,8 @@ remote_target::discard_pending_stop_replies (struct inferior *inf)
if (rs->remote_desc == NULL)
return;
- reply = (struct stop_reply *) rns->pending_event[notif_client_stop.id];
+ stop_reply_up reply
+ = as_stop_reply_up (std::move (rns->pending_event[notif_client_stop.id]));
/* Discard the in-flight notification. */
if (reply != NULL && reply->ptid.pid () == inf->pid)
That is always moving the stop reply from pending_event, when we only
really want to peek into it. The code further below that even says:
/* Discard the in-flight notification. */
if (reply != NULL && reply->ptid.pid () == inf->pid)
{
/* Leave the notification pending, since the server expects that
we acknowledge it with vStopped. But clear its contents, so
that later on when we acknowledge it, we also discard it. */
This commit reverts that hunk back, adjusted to use unique_ptr::get().
Change-Id: Ifc809d1a8225150a4656889f056d51267100ee24
We already use unique_ptr with notif_event and stop_reply in some
places around the remote target, but not fully. There are several
code paths that still use raw pointers. This commit makes all of the
ownership of these objects tracked by unique pointers, making the
lifetime flow much more obvious, IMHO.
I notice that it fixes a leak -- in remote_notif_stop_ack, We weren't
destroying the stop_reply object if it was of TARGET_WAITKIND_IGNORE
kind.
Approved-By: Tom Tromey <tom@tromey.com>
Change-Id: Id81daf39653d8792c8795b2a145772176bfae77c
struct cached_reg_t owns its data buffer, but currently that is
managed manually. Convert it to use a unique_xmalloc_ptr.
Approved-By: Tom Tromey <tom@tromey.com>
Change-Id: I05a107098b717299e76de76aaba00d7fbaeac77b
This changes gdb to use the C++17 [[fallthrough]] attribute rather
than special comments.
This was mostly done by script, but I neglected a few spellings and so
also fixed it up by hand.
I suspect this fixes the bug mentioned below, by switching to a
standard approach that, presumably, clang supports.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=23159
Approved-By: John Baldwin <jhb@FreeBSD.org>
Approved-By: Luis Machado <luis.machado@arm.com>
Approved-By: Pedro Alves <pedro@palves.net>
This changes serial_send_break and serial_write to throw exceptions
rather than attempt to set errno and return an error indicator. This
lets us correctly report failures on Windows.
Both functions had to be converted in a single patch because one
implementation of send_break works via write.
This also introduces remote_serial_send_break to handle error checking
when attempting to send a break. This was previously ignored.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30770
remote.c assumes that a failure to open the serial connection will set
errno. This is somewhat true, because the Windows code tries to set
errno appropriately -- but only somewhat, because it isn't clear that
the "pex" code sets it, and the tcp code seems to do the wrong thing.
It seems better to simply have the serial open functions throw on
error.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30770
remote.c has this code:
if (serial_setbaudrate (rs->remote_desc, baud_rate))
{
/* The requested speed could not be set. Error out to
top level after closing remote_desc. Take care to
set remote_desc to NULL to avoid closing remote_desc
more than once. */
serial_close (rs->remote_desc);
rs->remote_desc = NULL;
perror_with_name (name);
The perror here cannot be correct, because if serial_setbaudrate did
set errno, it may be obscured by serial_close.
This patch changes serial_setbaudrate to throw an exception instead.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30770
Since GDB now requires C++17, we don't need the internally maintained
gdb::optional implementation. This patch does the following replacing:
- gdb::optional -> std::optional
- gdb::in_place -> std::in_place
- #include "gdbsupport/gdb_optional.h" -> #include <optional>
This change has mostly been done automatically. One exception is
gdbsupport/thread-pool.* which did not use the gdb:: prefix as it
already lives in the gdb namespace.
Change-Id: I19a92fa03e89637bab136c72e34fd351524f65e9
Approved-By: Tom Tromey <tom@tromey.com>
Approved-By: Pedro Alves <pedro@palves.net>
While looking at the regcache code, I noticed that the address space
(passed to regcache when constructing it, and available through
regcache::aspace) wasn't relevant for the regcache itself. Callers of
regcache::aspace use that method because it appears to be a convenient
way of getting the address space for a thread, if you already have the
regcache. But there is always another way to get the address space, as
the callers pretty much always know which thread they are dealing with.
The regcache code itself doesn't use the address space.
This patch removes anything related to address_space from the regcache
code, and updates callers to get it from the thread in context. This
removes a bit of unnecessary complexity from the regcache code.
The current get_thread_arch_regcache function gets an address_space for
the given thread using the target_thread_address_space function (which
calls the target_ops::thread_address_space method). This suggest that
there might have been the intention of supporting per-thread address
spaces. But digging through the history, I did not find any such case.
Maybe this method was just added because we needed a way to get an
address space from a ptid (because constructing a regcache required an
address space), and this seemed like the right way to do it, I don't
know.
The only implementations of thread_address_space and
process_stratum_target::thread_address_space and
linux_nat_target::thread_address_space, which essentially just return
the inferior's address space. And thread_address_space is only used in
the current get_thread_arch_regcache, which gets removed. So, I think
that the thread_address_space target method can be removed, and we can
assume that it's fine to use the inferior's address space everywhere.
Callers of regcache::aspace are updated to get the address space from
the relevant inferior, either using some context they already know
about, or in last resort using the current global context.
So, to summarize:
- remove everything in regcache related to address spaces
- in particular, remove get_thread_arch_regcache, and rename
get_thread_arch_aspace_regcache to get_thread_arch_regcache
- remove target_ops::thread_address_space, and
target_thread_address_space
- adjust all users of regcache::aspace to get the address space another
way
Change-Id: I04fd41b22c83fe486522af7851c75bcfb31c88c7
When stepping over a breakpoint with displaced stepping, GDB needs to
be informed if the stepped thread exits, otherwise the displaced
stepping buffer that was allocated to that thread leaks, and this can
result in deadlock, with other threads waiting for their turn to
displaced step, but their turn never comes.
Similarly, when stepping over a breakpoint in line, GDB also needs to
be informed if the stepped thread exits, so that is can clear the step
over state and re-resume threads.
This commit makes it possible for GDB to ask the target to report
thread exit events for a given thread, using the new "thread options"
mechanism introduced by a previous patch.
This only adds the core bits. Following patches in the series will
teach the Linux backends (native & gdbserver) to handle the
GDB_THREAD_OPTION_EXIT option, and then a later patch will make use of
these thread exit events to clean up displaced stepping and inline
stepping state properly.
Reviewed-By: Andrew Burgess <aburgess@redhat.com>
Change-Id: I96b719fdf7fee94709e98bb3a90751d8134f3a38
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=27338
Currently, GDB does not understand the THREAD_EXITED stop reply in
remote all-stop mode. There's no good reason for this, it just
happened that THREAD_EXITED was only ever reported in non-stop mode so
far. This patch teaches GDB to parse that event in all-stop RSP too.
There is no need to add a qSupported feature for this, because the
server won't send a THREAD_EXITED event unless GDB explicitly asks for
it, with QThreadEvents, or with the GDB_THREAD_OPTION_EXIT
QThreadOptions option added in the next patch.
Change-Id: Ide5d12391adf432779fe4c79526801c4a5630966
A previous patch taught GDB about a new TARGET_WAITKIND_THREAD_CLONED
event kind, and made the Linux target report clone events.
A following patch will teach Linux GDBserver to do the same thing.
However, for remote debugging, it wouldn't be ideal for GDBserver to
report every clone event to GDB, when GDB only cares about such events
in some specific situations. Reporting clone events all the time
would be potentially chatty. We don't enable thread create/exit
events all the time for the same reason. Instead we have the
QThreadEvents packet. QThreadEvents is target-wide, though.
This patch makes GDB instead explicitly request that the target
reports clone events or not, on a per-thread basis.
In order to be able to do that with GDBserver, we need a new remote
protocol feature. Since a following patch will want to enable thread
exit events on per-thread basis too, the packet introduced here is
more generic than just for clone events. It lets you enable/disable a
set of options at once, modelled on Linux ptrace's PTRACE_SETOPTIONS.
IOW, this commit introduces a new QThreadOptions packet, that lets you
specify a set of per-thread event options you want to enable. The
packet accepts a list of options/thread-id pairs, similarly to vCont,
processed left to right, with the options field being a number
interpreted as a bit mask of options. The only option defined in this
commit is GDB_THREAD_OPTION_CLONE (0x1), which ask the remote target
to report clone events. Another patch later in the series will
introduce another option.
For example, this packet sets option "1" (clone events) on thread
p1000.2345:
QThreadOptions;1:p1000.2345
and this clears options for all threads of process 1000, and then sets
option "1" (clone events) on thread p1000.2345:
QThreadOptions;0:p1000.-1;1:p1000.2345
This clears options of all threads of all processes:
QThreadOptions;0
The target reports the set of supported options by including
"QThreadOptions=<supported options>" in its qSupported response.
infrun is then tweaked to enable GDB_THREAD_OPTION_CLONE when stepping
over a breakpoint.
Unlike PTRACE_SETOPTIONS, fork/vfork/clone children do NOT inherit
their parent's thread options. This is so that GDB can send e.g.,
"QThreadOptions;0;1:TID" without worrying about threads it doesn't
know about yet.
Documentation for this new remote protocol feature is included in a
documentation patch later in the series.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=19675
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=27830
Reviewed-By: Andrew Burgess <aburgess@redhat.com>
Change-Id: Ie41e5093b2573f14cf6ac41b0b5804eba75be37e
The previous patch taught GDB about a new
TARGET_WAITKIND_THREAD_CLONED event kind, and made the Linux target
report clone events.
A following patch will teach Linux GDBserver to do the same thing.
But before we get there, we need to teach the remote protocol about
TARGET_WAITKIND_THREAD_CLONED. That's what this patch does. Clone is
very similar to vfork and fork, and the new stop reply is likewise
handled similarly. The stub reports "T05clone:...".
GDBserver core is taught to handle TARGET_WAITKIND_THREAD_CLONED and
forward it to GDB in this patch, but no backend actually emits it yet.
That will be done in a following patch.
Documentation for this new remote protocol feature is included in a
documentation patch later in the series.
Reviewed-By: Andrew Burgess <aburgess@redhat.com>
Change-Id: If271f20320d864f074d8ac0d531cc1a323da847f
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=19675
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=27830
Overview
========
Consider the following situation, GDB is in non-stop mode, the main
thread is running while a second thread is stopped. The user has the
second thread selected as the current thread and asks GDB to detach.
At the exact moment of detach the main thread exits.
This situation currently causes crashes, assertion failures, and
unexpected errors to be reported from GDB for both native and remote
targets.
This commit addresses this situation for native and remote targets.
There are a number of different fixes, but all are required in order
to get this functionality working correct for native and remote
targets.
Native Linux Target
===================
For the native Linux target, detaching is handled in the function
linux_nat_target::detach. In here we call stop_wait_callback for each
thread, and it is this callback that will spot that the main thread
has exited.
GDB then detaches from everything except the main thread by calling
detach_callback.
After this the first problem is this assert:
/* Only the initial process should be left right now. */
gdb_assert (num_lwps (pid) == 1);
The num_lwps call will return 0 as the main thread has exited and all
of the other threads have now been detached. I fix this by changing
the assert to allow for 0 or 1 lwps at this point. As the 0 case can
only happen in non-stop mode, the assert becomes:
gdb_assert (num_lwps (pid) == 1
|| (target_is_non_stop_p () && num_lwps (pid) == 0));
The next problem is that we do:
main_lwp = find_lwp_pid (ptid_t (pid));
and then proceed assuming that main_lwp is not nullptr. In the case
that the main thread has exited though, main_lwp will be nullptr.
However, we only need main_lwp so that GDB can detach from the
thread. If the main thread has exited, and GDB has already detached
from every other thread, then GDB has finished detaching, GDB can skip
the calls that try to detach from the main thread, and then tell the
user that the detach was a success.
For Remote Targets
==================
On remote targets there are two problems.
First is that when the exit occurs during the early phase of the
detach, we see the stop notification arrive while GDB is removing the
breakpoints ahead of the detach. The 'set debug remote on' trace
looks like this:
[remote] Sending packet: $z0,7f1648fe0241,1#35
[remote] Notification received: Stop:W0;process:2a0ac8
# At this point an unpatched gdbserver segfaults, and the connection
# is broken. A patched gdbserver continues as below...
[remote] Packet received: E01
[remote] Sending packet: $z0,7f1648ff00a8,1#68
[remote] Packet received: E01
[remote] Sending packet: $z0,7f1648ff132f,1#6b
[remote] Packet received: E01
[remote] Sending packet: $D;2a0ac8#3e
[remote] Packet received: E01
I was originally running into Segmentation Faults, from within
gdbserver/mem-break.cc, in the function find_gdb_breakpoint. This
function calls current_process() and then dereferences the result to
find the breakpoint list.
However, in our case, the current process has already exited, and so
the current_process() call returns nullptr. At the point of failure,
the gdbserver backtrace looks like this:
#0 0x00000000004190e4 in find_gdb_breakpoint (z_type=48 '0', addr=4198762, kind=1) at ../../src/gdbserver/mem-break.cc:982
#1 0x000000000041930d in delete_gdb_breakpoint (z_type=48 '0', addr=4198762, kind=1) at ../../src/gdbserver/mem-break.cc:1093
#2 0x000000000042d8db in process_serial_event () at ../../src/gdbserver/server.cc:4372
#3 0x000000000042dcab in handle_serial_event (err=0, client_data=0x0) at ../../src/gdbserver/server.cc:4498
...
The problem is that, as a result non-stop being on, the process
exiting is only reported back to GDB after the request to remove a
breakpoint has been sent. Clearly gdbserver can't actually remove
this breakpoint -- the process has already exited -- so I think the
best solution is for gdbserver just to report an error, which is what
I've done.
The second problem I ran into was on the gdb side, as the process has
already exited, but GDB has not yet acknowledged the exit event, the
detach -- the 'D' packet in the above trace -- fails. This was being
reported to the user with a 'Can't detach process' error. As the test
actually calls detach from Python code, this error was then becoming a
Python exception.
Though clearly the detach has returned an error, and so, maybe, having
GDB throw an error would be fine, I think in this case, there's a good
argument that the remote error can be ignored -- if GDB tries to
detach and gets back an error, and if there's a pending exit event for
the pid we tried to detach, then just ignore the error and pretend the
detach worked fine.
We could possibly check for a pending exit event before sending the
detach packet, however, I believe that it might be possible (in
non-stop mode) for the stop notification to arrive after the detach is
sent, but before gdbserver has started processing the detach. In this
case we would still need to check for pending stop events after seeing
the detach fail, so I figure there's no point having two checks -- we
just send the detach request, and if it fails, check to see if the
process has already exited.
Testing
=======
In order to test this issue I needed to ensure that the exit event
arrives at the same time as the detach call. The window of
opportunity for getting the exit to arrive is so small I've never
managed to trigger this in real use -- I originally spotted this issue
while working on another patch, which did manage to trigger this
issue.
However, if we trigger both the exit and the detach from a single
Python function then we never return to GDB's event loop, as such GDB
never processes the exit event, and so the first time GDB gets a
chance to see the exit is during the detach call. And so that is the
approach I've taken for testing this patch.
Tested-By: Kevin Buettner <kevinb@redhat.com>
Approved-By: Kevin Buettner <kevinb@redhat.com>
Remove this typedef. I think that hiding the real type (std::vector)
behind a typedef just hinders readability.
Change-Id: I80949da3392f60a2826c56c268e0ec6f503ad79f
Approved-By: Pedro Alves <pedro@palves.net>
Reviewed-By: Reviewed-By: Lancelot Six <lancelot.six@amd.com>
As reported in bug 30630 [1], we hit a case where the remote target's
async flag is marked while the target is not configured (yet) to work
async. This should not happen. It is caught thanks to this assert in
remote_target::wait:
/* Start by clearing the flag that asks for our wait method to be called,
we'll mark it again at the end if needed. If the target is not in
async mode then the async token should not be marked. */
if (target_is_async_p ())
rs->clear_async_event_handler ();
else
gdb_assert (!rs->async_event_handler_marked ());
This is helpful, but I think that we could have caught the problem earlier than
that, at the moment we marked the handler. Catching problems earlier
makes them easier to debug.
[1] https://sourceware.org/bugzilla/show_bug.cgi?id=30630
Change-Id: I7e229c74b04da82bef6a817d5a676be5cf52e833
Approved-By: Andrew Burgess <aburgess@redhat.com>
A subsequent patch will want to know if the remote is async within a
remote_state method. Add a helper method for that, and for "can async"
as well, for symmetry.
Change-Id: Id0f648ee4896736479fa942f5453eeeb0e5d4352
Approved-By: Andrew Burgess <aburgess@redhat.com>
Make remote_async_inferior_event_token private (rename to
m_async_event_handler_token) and add methods for the various operations
we do on it. This will help by:
- allowing to break on those methods when debugging
- allowing to add assertions in the methods
Change-Id: Ia3b8a2bc48ad4849dbbe83442c3f83920f03334d
Approved-By: Andrew Burgess <aburgess@redhat.com>
This function is just a wrapper around the current inferior's gdbarch.
I find that having that wrapper just obscures where the arch is coming
from, and that it's often used as "I don't know which arch to use so
I'll use this magical target_gdbarch function that gets me an arch" when
the arch should in fact come from something in the context (a thread,
objfile, symbol, etc). I think that removing it and inlining
`current_inferior ()->arch ()` everywhere will make it a bit clearer
where that arch comes from and will trigger people into reflecting
whether this is the right place to get the arch or not.
Change-Id: I79f14b4e4934c88f91ca3a3155f5fc3ea2fadf6b
Reviewed-By: John Baldwin <jhb@FreeBSD.org>
Approved-By: Andrew Burgess <aburgess@redhat.com>
Make the inferior's gdbarch field private, and add getters and setters.
This helped me by allowing putting breakpoints on set_arch to know when
the inferior's arch was set. A subsequent patch in this series also
adds more things in set_arch.
Change-Id: I0005bd1ef4cd6b612af501201cec44e457998eec
Reviewed-By: John Baldwin <jhb@FreeBSD.org>
Approved-By: Andrew Burgess <aburgess@redhat.com>
The new_objfile observer is currently used to indicate both when a new
objfile is added to program space (when passed non-nullptr) and when all
objfiles of a program space were just removed (when passed nullptr).
I think this is confusing (and Andrew apparently thinks so too [1]).
Add a new "all_objfiles_removed" observer to remove the second role from
"new_objfile".
Some existing users of new_objfile do nothing if the passed objfile is
nullptr. For them, we can simply drop the nullptr check. For others,
add a new all_objfiles_removed callback, and refactor things a bit to
keep the existing behavior as much as possible.
Some callbacks relied on current_program_space, and following
the refactoring now use either objfile->pspace or the pspace passed to
all_objfiles_removed. I think this should be relatively safe, and in
general a step in the right direction.
On the notify side, I found only one call site to change from
new_objfile to all_objfiles_removed, in clear_symtab_users. It is not
entirely clear to me that this is entirely correct. clear_symtab_users
appears to be called in spots that don't remove all objfiles
(functions finish_new_objfile, remove_symbol_file_command, reread_symbols,
do_module_cleanups). But I think that this patch at least makes the
current code clearer.
[1] a0a031bce0
Change-Id: Icb648f72862e056267f30f44dd439bd4ec766f13
Approved-By: Tom Tromey <tom@tromey.com>
This replaces some casts to 'tracepoint *' with checked_static_cast.
Some functions are changed to accept a 'tracepoint *' now, for better
type safety.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
Following the example of private_thread_info and private_inferior, turn
struct btrace_target_info into a small class hierarchy.
Also merge btrace_tinfo_bts with btrace_tinfo_pt and inline into
linux_btrace_target_info.
Fixes PR gdb/30751.
This patch changes remote.c so that the getpkt 'forever' parameter now
defaults to 'false' and fixes up all the callers.
Reviewed-by: John Baldwin <jhb@FreeBSD.org>
getpkt and getpkt_or_notif_sane are just wrappers for
getpkt_or_notif_sane_1. This patch adds the is_notif parameter to
getpkt, with a suitable default, and removes the wrappers.
Reviewed-by: John Baldwin <jhb@FreeBSD.org>
For getpkt_or_notif_sane_1, expecting_notif is redundant, because it
always reflects whether the is_notif parameter is non-NULL. This
patch removes the redundant parameter.
Reviewed-by: John Baldwin <jhb@FreeBSD.org>
In remote_target::thread_info_to_thread_handle we return a copy:
...
gdb::byte_vector
remote_target::thread_info_to_thread_handle (struct thread_info *tp)
{
remote_thread_info *priv = get_remote_thread_info (tp);
return priv->thread_handle;
}
...
Fix this by returning a gdb::array_view instead:
...
gdb::array_view<const gdb_byte>
remote_target::thread_info_to_thread_handle (struct thread_info *tp)
...
Tested on x86_64-linux.
This fixes the build when building with -std=c++20.
Approved-By: Pedro Alves <pedro@palves.net>
When building gdb with -std=c++20 I run into:
...
gdb/ada-lang.c:10713:16: error: implicit capture of ‘this’ via ‘[=]’ is \
deprecated in C++20 [-Werror=deprecated]
10713 | auto do_op = [=] (LONGEST x, LONGEST y)
| ^
gdb/ada-lang.c:10713:16: note: add explicit ‘this’ or ‘*this’ capture
...
Fix this by using "[this]".
Likewise in two more spots.
Tested on x86_64-linux.
When building gdb with clang 15 and -std=c++20, I run into:
...
gdbsupport/common-exceptions.h:203:32: error: arithmetic between different \
enumeration types ('const enum return_reason' and 'const enum errors') is \
deprecated [-Werror,-Wdeprecated-enum-enum-conversion]
size_t result = exc.reason + exc.error;
~~~~~~~~~~ ^ ~~~~~~~~~
...
Fix this by using to_underlying.
Likewise in a few other places.
Tested on x86_64-linux.
A local gdb test failed with this error message:
Remote communication error. Target disconnected.: Arg list too long.
The ".:" seemed weird to me. This patch removes the ".".
Reviewed-by: John Baldwin <jhb@FreeBSD.org>
While working on another patch I triggered a segfault from within the
function remote_target::discard_pending_stop_replies. Turns out this
was caused by a cut&paste error introduced in this commit:
commit df5ad10200
Date: Wed Dec 1 09:40:03 2021 -0500
gdb, gdbserver: detach fork child when detaching from fork parent
This commit adds a remote_debug_printf call that was copied from
earlier in the function, however, the new call wasn't updated to use
the appropriate local variable. The local variable that it is using
might be nullptr, in which case we trigger undefined behaviour, and
could crash, which is what I was seeing.
Fixed by updating to use the correct local variable.
This patch changes the remote.c readahead_cache to use
gdb::byte_vector. This simplifies the code by removing manual memory
management.
Reviewed-by: John Baldwin <jhb@FreeBSD.org>
Instead of having the interpreter code registering observers for the
signal_received observable, add a "signal_received" virtual method to
struct interp. Add a interps_notify_signal_received function that loops
over all UIs and calls the signal_received method on the interpreter.
Finally, add a notify_signal_received function that calls
interps_notify_signal_received and then notifies the observers. Replace
all existing notifications to the signal_received observers with calls
to notify_signal_received.
Before this patch, the CLI and MI code both register a signal_received
observer. These observer go over all UIs, and, for those that have a
interpreter of the right kind, print the stop notifiation.
After this patch, we have just one "loop over all UIs", inside
interps_notify_signal_received. Since the interp::on_signal_received
method gets called once for each interpreter, the implementations only
need to deal with the current interpreter (the "this" pointer).
The motivation for this patch comes from a future patch, that makes the
amdgpu code register an observer to print a warning after the CLI's
signal stop message. Since the amdgpu and the CLI code both use
observers, the order of the two messages is not stable, unless we define
the priority using the observer dependency system. However, the
approach of using virtual methods on the interpreters seems like a good
change anyway, I think it's more straightforward and simple to
understand than the current solution that uses observers. We are sure
that the amdgpu message gets printed after the CLI message, since
observers are notified after interpreters.
Keep the signal_received, even if nothing uses if, because we will be
using it in the upcoming amdgpu patch implementing the warning described
above.
Change-Id: I4d8614bb8f6e0717f4bfc2a59abded3702f23ac4
Remove the bp_location_pointer_iterator layer. Adjust all users of
breakpoint::locations to use references instead of pointers.
Change-Id: Iceed34f5e0f5790a9cf44736aa658be6d1ba1afa
Reviewed-By: Andrew Burgess <aburgess@redhat.com>
While working on a later patch in this series, which tightens up some
of our pattern matching when using gdb_test, I ran into some failures
caused by some warnings having a trailing newline character.
The warning function already adds a trailing newline, and it is my
understanding that we should not be adding a second by including a
newline at the end of any warning message.
The problem cases I found were in language.c and remote.c, in this
patch I fix the cases I hit, but I also checked all the other warning
calls in these two files and removed any additional trailing newlines
I found.
In remote.c the warning actually had a newline character in the middle
of the warning message (in addition to the trailing newline), which
I've removed. I don't think it's helpful to forcibly split a warning
as was done here -- in the middle of a sentence. Additionally, the
message isn't even that long (71 characters), so I think removing this
newline is an improvement.
None of the expected test result need updating with this commit,
currently the patterns in gdb_test will match one or more newline
sequences, so the tests are as happy with one newline (after this
commit) as they are with two newlines (before this commit). A later
commit will change gdb_test so that it is not so forgiving, and these
warnings would have caused some failures.
Reviewed-By: Tom Tromey <tom@tromey.com>
Make find_thread_ptid (the overload that takes a process_stratum_target)
a method of process_stratum_target.
Change-Id: Ib190a925a83c6b93e9c585dc7c6ab65efbdd8629
Reviewed-By: Tom Tromey <tom@tromey.com>
The workaround for the vCont packet is no longer required due to the
former commit "gdb: Make global feature array a per-remote target array".
The vCont packet is now checked once when the connection is started and
the supported vCont actions are set to the target's remote state
attribute.
This patch adds per-remote target variables for the configuration of
memory read- and write packet size. It is a further change to commit
"gdb: Make global feature array a per-remote target array" to apply the
fixme notes described in commit 5b6d1e4 "Multi-target support".
The former global variables for that configuration are still available
to allow the command line configuration for all future remote
connections. Similar to the command line configuration of the per-
remote target feature array, the commands
- set remotewritesize (deprecated)
- set remote memory-read-packet-size
- set remote memory-write-packet-size
will configure the current target (if available). If no target is
available, the default configuration for future remote connections is
adapted. The show command will display the current remote target's
packet size configuration. If no remote target is selected, the default
configuration for future connections will be shown.
It is required to adapt the test gdb.base/remote.exp which is failing
for --target_board=native-extended-gdbserver. With that board GDB
connects to gdbserver at gdb start time. Due to this patch two loggings
"The target may not be able to.." are shown if the command 'set remote
memory-write-packet-size fixed' is executed while a target is connected
for the current inferior. To fix this, the clean_restart command is
moved to a later time point of the test. It is sufficient to be
connected to the server when "runto_main" is executed. Now the
connection time is similar to a testrun with
--target_board=native-gdbserver.
To allow the user to distinguish between the packet-size configuration
for future remote connections and for the currently selected target, the
commands' loggings are adapted.
This patch applies the appropriate FIXME notes described in commit 5b6d1e4
"Multi-target support".
"You'll notice that remote.c includes some FIXME notes. These refer to
the fact that the global arrays that hold data for the remote packets
supported are still globals. For example, if we connect to two
different servers/stubs, then each might support different remote
protocol features. They might even be different architectures, like
e.g., one ARM baremetal stub, and a x86 gdbserver, to debug a
host/controller scenario as a single program. That isn't going to
work correctly today, because of said globals. I'm leaving fixing
that for another pass, since it does not appear to be trivial, and I'd
rather land the base work first. It's already useful to be able to
debug multiple instances of the same server (e.g., a distributed
cluster, where you have full control over the servers installed), so I
think as is it's already reasonable incremental progress."
Using this patch it is possible to configure per-remote targets'
feature packets.
Given the following setup for two gdbservers:
~~~~
gdbserver --multi :1234
gdbserver --disable-packet=vCont --multi :2345
~~~~
Before this patch configuring of range-stepping was not possible for one
of two connected remote targets with different support for the vCont
packet. As one of the targets supports vCont, it should be possible to
configure "set range-stepping". However, the output of GDB looks like:
(gdb) target extended-remote :1234
Remote debugging using :1234
(gdb) add-inferior -no-connection
[New inferior 2]
Added inferior 2
(gdb) inferior 2
[Switching to inferior 2 [<null>] (<noexec>)]
(gdb) target extended-remote :2345
Remote debugging using :2345
(gdb) set range-stepping on
warning: Range stepping is not supported by the current target
(gdb) inferior 1
[Switching to inferior 1 [<null>] (<noexec>)]
(gdb) set range-stepping on
warning: Range stepping is not supported by the current target
~~~~
Two warnings are shown. The warning for inferior 1 should not appear
as it is connected to a target supporting the vCont package.
~~~~
(gdb) target extended-remote :1234
Remote debugging using :1234
(gdb) add-inferior -no-connection
[New inferior 2]
Added inferior 2
(gdb) inferior 2
[Switching to inferior 2 [<null>] (<noexec>)]
(gdb) target extended-remote :2345
Remote debugging using :2345
(gdb) set range-stepping on
warning: Range stepping is not supported by the current target
(gdb) inferior 1
[Switching to inferior 1 [<null>] (<noexec>)]
(gdb) set range-stepping on
(gdb)
~~~~
Now only one warning is shown for inferior 2, which is connected to
a target not supporting vCont.
The per-remote target feature array is realized by a new class
remote_features, which stores the per-remote target array and
provides functions to determine supported features of the target.
A remote_target object now has a new member of that class.
Each time a new remote_target object is initialized, a new per-remote
target array is constructed based on the global remote_protocol_packets
array. The global array is initialized in the function _initialize_remote
and can be configured using the command line. Before this patch the
command line configuration affected current targets and future remote
targets (due to the global feature array used by all remote
targets). This behavior is different and the configuration applies as
follows:
- If a target is connected, the command line configuration affects the
current connection. All other existing remote targets are not
affected.
- If not connected, the command line configuration affects future
connections.
The show command displays the current remote target's configuration. If no
remote target is selected the default configuration for future
connections is shown.
If we have for instance the following setup with inferior 2 being
selected:
~~~~
(gdb) info inferiors
Num Description Connection Executable
1 <null> 1 (extended-remote :1234)
* 2 <null> 2 (extended-remote :2345)
~~~~
Before this patch, if we run 'set remote multiprocess-feature-packet', the
following configuration was set:
The feature array of all remote targets (in this setup the two connected
targets) and all future remote connections are affected.
After this patch, it will be configured as follows:
The feature array of target with port :2345 which is currently selected
will be configured. All other existing remote targets are not affected.
The show command 'show remote multiprocess-feature-packet' will display
the configuration of target with port :2345.
Due to this configuration change, it is required to adapt the test
"gdb/testsuite/gdb.multi/multi-target-info-inferiors.exp" to configure the
multiprocess-feature-packet before the connections are created.
To inform the gdb user about the new behaviour of the 'show remote
PACKET-NAME' commands and the new configuration impact for remote
targets using the 'set remote PACKET-NAME' commands the commands'
outputs are adapted. Due to this change it is required to adapt each
test using the set/show remote 'PACKET-NAME' commands.
