* [PATCH v2 1/2] gdb/remote: iterate on pspace inferiors in remote_new_objfile
2022-04-30 3:21 [PATCH v2 0/2] Fix regressions caused by prune_threads patch Simon Marchi
@ 2022-04-30 3:21 ` Simon Marchi
2022-05-04 12:04 ` Pedro Alves
2022-04-30 3:21 ` [PATCH v2 2/2] gdb/remote: send qSymbol to all inferiors on startup Simon Marchi
1 sibling, 1 reply; 7+ messages in thread
From: Simon Marchi @ 2022-04-30 3:21 UTC (permalink / raw)
To: gdb-patches
Commit 152a17495663 ("gdb: prune inferiors at end of
fetch_inferior_event, fix intermittent failure of
gdb.threads/fork-plus-threads.exp") broke some tests with the
native-gdbserver board, such as:
(gdb) PASS: gdb.base/step-over-syscall.exp: detach-on-fork=off: follow-fork=child: break cond on target : vfork: break marker
continue^M
Continuing.^M
terminate called after throwing an instance of 'gdb_exception_error'^M
I can manually reproduce the issue by running (just the commands that
the test does as a one liner):
$ ./gdb -q --data-directory=data-directory \
testsuite/outputs/gdb.base/step-over-syscall/step-over-vfork \
-ex "tar rem | ../gdbserver/gdbserver - testsuite/outputs/gdb.base/step-over-syscall/step-over-vfork" \
-ex "b main" \
-ex c \
-ex "d 1" \
-ex "set displaced-stepping off" \
-ex "b *0x7ffff7d7ac5a if main == 0" \
-ex "set detach-on-fork off" \
-ex "set follow-fork-mode child" \
-ex c \
-ex "inferior 1" \
-ex "b marker" \
-ex c
... where 0x7ffff7d7ac5a is the exact address of the vfork syscall
(which can be found by looking at gdb.log).
The important part of the above is that a vfork syscall creates inferior
2, then inferior 2 executes until exit, then we switch back to inferior
1 and try to resume it.
The uncaught exception happens here:
#4 0x00005596969d81a9 in error (fmt=0x559692da9e40 "Cannot execute this command while the target is running.\nUse the \"interrupt\" command to stop the target\nand then try again.")
at /home/simark/src/binutils-gdb/gdbsupport/errors.cc:43
#5 0x0000559695af6f66 in remote_target::putpkt_binary (this=0x617000038080, buf=0x559692da4380 "qSymbol::", cnt=9) at /home/simark/src/binutils-gdb/gdb/remote.c:9560
#6 0x0000559695af6aaf in remote_target::putpkt (this=0x617000038080, buf=0x559692da4380 "qSymbol::") at /home/simark/src/binutils-gdb/gdb/remote.c:9518
#7 0x0000559695ab50dc in remote_target::remote_check_symbols (this=0x617000038080) at /home/simark/src/binutils-gdb/gdb/remote.c:5141
#8 0x0000559695b3cccf in remote_new_objfile (objfile=0x0) at /home/simark/src/binutils-gdb/gdb/remote.c:14600
#9 0x0000559693bc52a9 in std::__invoke_impl<void, void (*&)(objfile*), objfile*> (__f=@0x61b0000167f8: 0x559695b3cb1d <remote_new_objfile(objfile*)>) at /usr/include/c++/11.2.0/bits/invoke.h:61
#10 0x0000559693bb2848 in std::__invoke_r<void, void (*&)(objfile*), objfile*> (__fn=@0x61b0000167f8: 0x559695b3cb1d <remote_new_objfile(objfile*)>) at /usr/include/c++/11.2.0/bits/invoke.h:111
#11 0x0000559693b8dddf in std::_Function_handler<void (objfile*), void (*)(objfile*)>::_M_invoke(std::_Any_data const&, objfile*&&) (__functor=..., __args#0=@0x7ffe0bae0590: 0x0) at /usr/include/c++/11.2.0/bits/std_function.h:291
#12 0x00005596956374b2 in std::function<void (objfile*)>::operator()(objfile*) const (this=0x61b0000167f8, __args#0=0x0) at /usr/include/c++/11.2.0/bits/std_function.h:560
#13 0x0000559695633c64 in gdb::observers::observable<objfile*>::notify (this=0x55969ef5c480 <gdb::observers::new_objfile>, args#0=0x0) at /home/simark/src/binutils-gdb/gdb/../gdbsupport/observable.h:150
#14 0x0000559695df6cc2 in clear_symtab_users (add_flags=...) at /home/simark/src/binutils-gdb/gdb/symfile.c:2873
#15 0x000055969574c263 in program_space::~program_space (this=0x6120000c8a40, __in_chrg=<optimized out>) at /home/simark/src/binutils-gdb/gdb/progspace.c:154
#16 0x0000559694fc086b in delete_inferior (inf=0x61700003bf80) at /home/simark/src/binutils-gdb/gdb/inferior.c:205
#17 0x0000559694fc341f in prune_inferiors () at /home/simark/src/binutils-gdb/gdb/inferior.c:390
#18 0x0000559695017ada in fetch_inferior_event () at /home/simark/src/binutils-gdb/gdb/infrun.c:4293
#19 0x0000559694f629e6 in inferior_event_handler (event_type=INF_REG_EVENT) at /home/simark/src/binutils-gdb/gdb/inf-loop.c:41
#20 0x0000559695b3b0e3 in remote_async_serial_handler (scb=0x6250001ef100, context=0x6170000380a8) at /home/simark/src/binutils-gdb/gdb/remote.c:14466
#21 0x0000559695c59eb7 in run_async_handler_and_reschedule (scb=0x6250001ef100) at /home/simark/src/binutils-gdb/gdb/ser-base.c:138
#22 0x0000559695c5a42a in fd_event (error=0, context=0x6250001ef100) at /home/simark/src/binutils-gdb/gdb/ser-base.c:189
#23 0x00005596969d9ebf in handle_file_event (file_ptr=0x60700005af40, ready_mask=1) at /home/simark/src/binutils-gdb/gdbsupport/event-loop.cc:574
#24 0x00005596969da7fa in gdb_wait_for_event (block=0) at /home/simark/src/binutils-gdb/gdbsupport/event-loop.cc:700
#25 0x00005596969d8539 in gdb_do_one_event () at /home/simark/src/binutils-gdb/gdbsupport/event-loop.cc:212
If I enable "set debug infrun" just before the last continue, we see:
(gdb) continue
Continuing.
[infrun] clear_proceed_status_thread: 965604.965604.0
[infrun] proceed: enter
[infrun] proceed: addr=0xffffffffffffffff, signal=GDB_SIGNAL_DEFAULT
[infrun] scoped_disable_commit_resumed: reason=proceeding
[infrun] start_step_over: enter
[infrun] start_step_over: stealing global queue of threads to step, length = 0
[infrun] operator(): step-over queue now empty
[infrun] start_step_over: exit
[infrun] resume_1: step=0, signal=GDB_SIGNAL_0, trap_expected=0, current thread [965604.965604.0] at 0x7ffff7d7ac5c
[infrun] do_target_resume: resume_ptid=965604.0.0, step=0, sig=GDB_SIGNAL_0
[infrun] prepare_to_wait: prepare_to_wait
[infrun] reset: reason=proceeding
[infrun] maybe_set_commit_resumed_all_targets: enabling commit-resumed for target remote
[infrun] maybe_call_commit_resumed_all_targets: calling commit_resumed for target remote
[infrun] proceed: exit
[infrun] fetch_inferior_event: enter
[infrun] scoped_disable_commit_resumed: reason=handling event
[infrun] do_target_wait: Found 2 inferiors, starting at #1
[infrun] random_pending_event_thread: None found.
[infrun] print_target_wait_results: target_wait (-1.0.0 [process -1], status) =
[infrun] print_target_wait_results: 965604.965604.0 [Thread 965604.965604],
[infrun] print_target_wait_results: status->kind = VFORK_DONE
[infrun] handle_inferior_event: status->kind = VFORK_DONE
[infrun] context_switch: Switching context from 0.0.0 to 965604.965604.0
[infrun] handle_vfork_done: not waiting for a vfork-done event
[infrun] start_step_over: enter
[infrun] start_step_over: stealing global queue of threads to step, length = 0
[infrun] operator(): step-over queue now empty
[infrun] start_step_over: exit
[infrun] resume_1: step=0, signal=GDB_SIGNAL_0, trap_expected=0, current thread [965604.965604.0] at 0x7ffff7d7ac5c
[infrun] do_target_resume: resume_ptid=965604.0.0, step=0, sig=GDB_SIGNAL_0
[infrun] prepare_to_wait: prepare_to_wait
[infrun] reset: reason=handling event
[infrun] maybe_set_commit_resumed_all_targets: enabling commit-resumed for target remote
[infrun] maybe_call_commit_resumed_all_targets: calling commit_resumed for target remote
terminate called after throwing an instance of 'gdb_exception_error'
What happens is:
- After doing the "continue" on inferior 1, the remote target gives us
a VFORK_DONE event. The core ignores it and resumes inferior 1.
- Since prune_inferiors is now called after each handled event, in
fetch_inferior_event, it is called after we handled that VFORK_DONE
event and resumed inferior 1.
- Inferior 2 is pruned, which (see backtrace above) causes its program
space to be deleted, which clears the symtabs for that program space,
which calls the new_objfile observable and remote_new_objfile
observer (with a nullptr objfile, to indicate that the previously
loaded symbols have been discarded), which calls
remote_check_symbols.
remote_check_symbols is the function that sends the qSymbol packet, to
let the remote side ask for symbol addresses. The problem is that the
remote target is working in all-stop / sync mode and is currently
resumed. It has sent a vCont packet to resume the target and is waiting
for a stop reply. It can't send any packets in the mean time. That
causes the exception to be thrown.
This wasn't a problem before, when prune_inferiors was called in
normal_stop, because it was always called at a time the target was not
resumed.
An important observation here is that the new_objfile observable is
invoked for a change in inferior 2's program space (inferior 2's program
space is the current program space). Inferior 2 isn't bound to any
process on the remote side (it has exited, that's why it's being
pruned). It doesn't make sense to try to send a qSymbol packet for a
process that doesn't exist on the remote side. remote_check_symbols
actually attempts to avoid that:
/* The remote side has no concept of inferiors that aren't running
yet, it only knows about running processes. If we're connected
but our current inferior is not running, we should not invite the
remote target to request symbol lookups related to its
(unrelated) current process. */
if (!target_has_execution ())
return;
The problem here is that while inferior 2's program space is the current
program space, inferior 1 is the current inferior. So the check above
passes, since inferior has execution. We therefore try to send a
qSymbol packet for inferior 1 in reaction to a change in inferior 2's
program space, that's wrong.
This exposes a conceptual flaw in remote_new_objfile. The "new_objfile"
event concerns a specific program space, which can concern multiple
inferiors, as inferiors can share a program space. We shouldn't
consider the current inferior at all, but instead all inferiors bound to
the affected program space. Especially since the current inferior can
be unrelated to the current program space at that point.
To be clear, we are in this state because ~program_space sets itself as
the current program space, but there is no more inferior having that
program space to switch to, inferior 2 has already been unlinked.
To fix this, make remote_new_objfile iterate on all inferiors bound to
the affected program space. Remove the target_has_execution check from
remote_check_symbols, replace it with an assert. All callers must
ensure that the current inferior has execution before calling it.
Change-Id: Ica643145bcc03115248290fd310cadab8ec8371c
---
gdb/remote.c | 87 +++++++++++++++++++++++++++++++++++-----------------
1 file changed, 59 insertions(+), 28 deletions(-)
diff --git a/gdb/remote.c b/gdb/remote.c
index 6914947c1723..fde6df3f84d5 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -1016,13 +1016,20 @@ struct stop_reply : public notif_event
int core;
};
+/* Return TARGET as a remote_target if it is one, else nullptr. */
+
+static remote_target *
+as_remote_target (process_stratum_target *target)
+{
+ return dynamic_cast<remote_target *> (target);
+}
+
/* See remote.h. */
bool
is_remote_target (process_stratum_target *target)
{
- remote_target *rt = dynamic_cast<remote_target *> (target);
- return rt != nullptr;
+ return as_remote_target (target) != nullptr;
}
/* Per-program-space data key. */
@@ -5116,13 +5123,10 @@ remote_target::remote_check_symbols ()
char *tmp;
int end;
- /* The remote side has no concept of inferiors that aren't running
- yet, it only knows about running processes. If we're connected
- but our current inferior is not running, we should not invite the
- remote target to request symbol lookups related to its
- (unrelated) current process. */
- if (!target_has_execution ())
- return;
+ /* It doesn't make sense to send a qSymbol packet for an inferior that
+ doesn't have execution, because the remote side doesn't know about
+ inferiors without execution. */
+ gdb_assert (target_has_execution ());
if (packet_support (PACKET_qSymbol) == PACKET_DISABLE)
return;
@@ -14575,28 +14579,55 @@ show_remote_cmd (const char *args, int from_tty)
static void
remote_new_objfile (struct objfile *objfile)
{
- remote_target *remote = get_current_remote_target ();
+ /* The objfile change happened in that program space. */
+ program_space *pspace = current_program_space;
- /* First, check whether the current inferior's process target is a remote
- target. */
- if (remote == nullptr)
- return;
+ /* The affected program space is possibly shared by multiple inferiors.
+ Consider sending a qSymbol packet for each of the inferiors using that
+ program space. */
+ for (inferior *inf : all_inferiors ())
+ {
+ if (inf->pspace != pspace)
+ continue;
- /* When we are attaching or handling a fork child and the shared library
- subsystem reads the list of loaded libraries, we receive new objfile
- events in between each found library. The libraries are read in an
- undefined order, so if we gave the remote side a chance to look up
- symbols between each objfile, we might give it an inconsistent picture
- of the inferior. It could appear that a library A appears loaded but
- a library B does not, even though library A requires library B. That
- would present a state that couldn't normally exist in the inferior.
-
- So, skip these events, we'll give the remote a chance to look up symbols
- once all the loaded libraries and their symbols are known to GDB. */
- if (current_inferior ()->in_initial_library_scan)
- return;
+ /* Check whether the inferior's process target is a remote target. */
+ remote_target *remote = as_remote_target (inf->process_target ());
+ if (remote == nullptr)
+ continue;
+
+ /* When we are attaching or handling a fork child and the shared library
+ subsystem reads the list of loaded libraries, we receive new objfile
+ events in between each found library. The libraries are read in an
+ undefined order, so if we gave the remote side a chance to look up
+ symbols between each objfile, we might give it an inconsistent picture
+ of the inferior. It could appear that a library A appears loaded but
+ a library B does not, even though library A requires library B. That
+ would present a state that couldn't normally exist in the inferior.
+
+ So, skip these events, we'll give the remote a chance to look up
+ symbols once all the loaded libraries and their symbols are known to
+ GDB. */
+ if (inf->in_initial_library_scan)
+ continue;
+
+ if (!remote->has_execution (inf))
+ continue;
+
+ /* Need to switch to a specific thread, because remote_check_symbols will
+ set the general thread using INFERIOR_PTID.
- remote->remote_check_symbols ();
+ It's possible to have inferiors with no thread here, because we are
+ called very early in the connection process, while the inferior is
+ being set up, before threads are added. Just skip it, start_remote_1
+ also calls remote_check_symbols when it's done setting things up. */
+ thread_info *thread = any_thread_of_inferior (inf);
+ if (thread != nullptr)
+ {
+ scoped_restore_current_thread restore_thread;
+ switch_to_thread (thread);
+ remote->remote_check_symbols ();
+ }
+ }
}
/* Pull all the tracepoints defined on the target and create local
--
2.36.0
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH v2 2/2] gdb/remote: send qSymbol to all inferiors on startup
2022-04-30 3:21 [PATCH v2 0/2] Fix regressions caused by prune_threads patch Simon Marchi
2022-04-30 3:21 ` [PATCH v2 1/2] gdb/remote: iterate on pspace inferiors in remote_new_objfile Simon Marchi
@ 2022-04-30 3:21 ` Simon Marchi
2022-05-04 12:03 ` Pedro Alves
1 sibling, 1 reply; 7+ messages in thread
From: Simon Marchi @ 2022-04-30 3:21 UTC (permalink / raw)
To: gdb-patches; +Cc: Simon Marchi
From: Simon Marchi <simon.marchi@efficios.com>
start_remote_1 calls remote_check_symbols after things are set up to
give the remote side a chance to look up symbols. One call to
remote_check_symbols sets the "general thread", if needed, and sends one
qSymbol packet. In theory, a remote target could have more than one
process on initial connection, and this would send a qSymbol for only
one of these processes.
Change it to iterate on all the target's inferiors and send a qSymbol
packet for each one.
I was able to kind of test this by changing gdbserver to spawn two processes on
startup:
diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index 33c42714e72..9b682e9f85f 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -3939,6 +3939,7 @@ captured_main (int argc, char *argv[])
/* Wait till we are at first instruction in program. */
target_create_inferior (program_path.get (), program_args);
+ target_create_inferior (program_path.get (), program_args);
/* We are now (hopefully) stopped at the first instruction of
the target process. This assumes that the target process was
And I was able to see qSymbol being sent for each inferior:
[remote] Sending packet: $Hgp828dc.828dc#1f
[remote] Packet received: OK
[remote] Sending packet: $qSymbol::#5b
[remote] Packet received: qSymbol:6764625f6167656e745f6764625f74705f686561705f627566666572
[remote] Sending packet: $qSymbol::6764625f6167656e745f6764625f74705f686561705f627566666572#1e
[remote] Packet received: qSymbol:6e70746c5f76657273696f6e
[remote] Sending packet: $qSymbol::6e70746c5f76657273696f6e#4d
[remote] Packet received: OK
[remote] Sending packet: $Hgp828dd.828dd#21
[remote] Packet received: OK
[remote] Sending packet: $qSymbol::#5b
[remote] Packet received: qSymbol:6764625f6167656e745f6764625f74705f686561705f627566666572
[remote] Sending packet: $qSymbol::6764625f6167656e745f6764625f74705f686561705f627566666572#1e
[remote] Packet received: qSymbol:6e70746c5f76657273696f6e
[remote] Sending packet: $qSymbol::6e70746c5f76657273696f6e#4d
[remote] Packet received: OK
Note that there would probably be more work to be done to fully support
this scenario, more things that need to be done for each discovered
inferior instead of just for one.
Change-Id: I21c4ecf6367391e2e389b560f0b4bd906cf6472f
---
gdb/remote.c | 20 ++++++++++++++++----
1 file changed, 16 insertions(+), 4 deletions(-)
diff --git a/gdb/remote.c b/gdb/remote.c
index fde6df3f84d5..b82140dc3d09 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -5021,12 +5021,24 @@ remote_target::start_remote_1 (int from_tty, int extended_p)
target_async (1);
}
- /* If we connected to a live target, do some additional setup. */
- if (target_has_execution ())
+ /* Give the target a chance to look up symbols. */
+ for (inferior *inf : all_inferiors (this))
{
+ /* The inferiors that exist at this point were created from what
+ was found already running on the remote side, so we know they
+ have execution. */
+ gdb_assert (this->has_execution (inf));
+
/* No use without a symbol-file. */
- if (current_program_space->symfile_object_file)
- remote_check_symbols ();
+ if (inf->pspace->symfile_object_file == nullptr)
+ continue;
+
+ /* Need to switch to a specific thread, because remote_check_symbols
+ uses INFERIOR_PTID to set the general thread. */
+ scoped_restore_current_thread restore_thread;
+ thread_info *thread = any_thread_of_inferior (inf);
+ switch_to_thread (thread);
+ this->remote_check_symbols ();
}
/* Possibly the target has been engaged in a trace run started
--
2.36.0
^ permalink raw reply [flat|nested] 7+ messages in thread