* [PATCH v4 0/3] Reduce back and forth with target when threads have pending statuses
@ 2021-01-25 4:57 Simon Marchi
2021-01-25 4:57 ` [PATCH v4 1/3] gdb/testsuite: add test for run/attach while program is running Simon Marchi
` (3 more replies)
0 siblings, 4 replies; 8+ messages in thread
From: Simon Marchi @ 2021-01-25 4:57 UTC (permalink / raw)
To: gdb-patches
This is the v4 of patches 3 and 4 of this series:
https://sourceware.org/pipermail/gdb-patches/2021-January/174786.html
The "better handling of 'S' packets" part of the series was already
merged.
Patch 1 is new, it adds a test to cover a case that didn't seem already
covered, that is running or attaching while the inferior is running.
Note that this series was developped and tested on top of these other
series/patches, so they can be considered prerequisites (applied in this
order):
Fix detach + displaced-step regression + N bugs more
https://sourceware.org/pipermail/gdb-patches/2021-January/175040.html
Clear target async event handlers in wait method
https://sourceware.org/pipermail/gdb-patches/2020-November/173633.html
gdb: remove unneeded argument in check_multi_target_resumption
https://sourceware.org/pipermail/gdb-patches/2021-January/175057.html
Simon Marchi (3):
gdb/testsuite: add test for run/attach while program is running
gdb: move commit_resume to process_stratum_target
gdb: generalize commit_resume, avoid commit-resuming when threads have
pending statuses
gdb/infcmd.c | 490 +++++++++---------
gdb/infrun.c | 147 +++++-
gdb/infrun.h | 59 +++
gdb/mi/mi-main.c | 88 ++--
gdb/process-stratum-target.c | 14 +
gdb/process-stratum-target.h | 38 ++
gdb/record-full.c | 10 +-
gdb/remote.c | 123 +++--
gdb/target-delegates.c | 22 -
gdb/target.c | 30 +-
gdb/target.h | 20 -
.../gdb.base/run-attach-while-running.c | 69 +++
.../gdb.base/run-attach-while-running.exp | 131 +++++
13 files changed, 847 insertions(+), 394 deletions(-)
create mode 100644 gdb/testsuite/gdb.base/run-attach-while-running.c
create mode 100644 gdb/testsuite/gdb.base/run-attach-while-running.exp
--
2.30.0
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v4 1/3] gdb/testsuite: add test for run/attach while program is running
2021-01-25 4:57 [PATCH v4 0/3] Reduce back and forth with target when threads have pending statuses Simon Marchi
@ 2021-01-25 4:57 ` Simon Marchi
2021-02-06 18:22 ` Pedro Alves
2021-01-25 4:57 ` [PATCH v4 2/3] gdb: move commit_resume to process_stratum_target Simon Marchi
` (2 subsequent siblings)
3 siblings, 1 reply; 8+ messages in thread
From: Simon Marchi @ 2021-01-25 4:57 UTC (permalink / raw)
To: gdb-patches
A previous version of this patch series broke the use case of doing
"run" or "attach" while the program is running, but it wasn't caught by
the testsuite, which means it's not covered. Add a test for that.
gdb/testsuite/ChangeLog:
* gdb.base/run-attach-while-running.exp: New.
* gdb.base/run-attach-while-running.c: New.
Change-Id: I77f098ec0b28dc2d4575ea80e941f6a75273e431
---
.../gdb.base/run-attach-while-running.c | 69 +++++++++
.../gdb.base/run-attach-while-running.exp | 131 ++++++++++++++++++
2 files changed, 200 insertions(+)
create mode 100644 gdb/testsuite/gdb.base/run-attach-while-running.c
create mode 100644 gdb/testsuite/gdb.base/run-attach-while-running.exp
diff --git a/gdb/testsuite/gdb.base/run-attach-while-running.c b/gdb/testsuite/gdb.base/run-attach-while-running.c
new file mode 100644
index 000000000000..57bebbe6456e
--- /dev/null
+++ b/gdb/testsuite/gdb.base/run-attach-while-running.c
@@ -0,0 +1,69 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2021 Free Software Foundation, Inc.
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program. If not, see <http://www.gnu.org/licenses/>. */
+
+#include <unistd.h>
+#include <assert.h>
+
+#ifndef WITH_THREADS
+# error "WITH_THREADS must be defined."
+#endif
+
+#if WITH_THREADS
+# include <pthread.h>
+
+static pthread_barrier_t barrier;
+
+static void *
+thread_func (void *p)
+{
+ pthread_barrier_wait (&barrier);
+
+ for (int i = 0; i < 30; i++)
+ sleep (1);
+
+ return NULL;
+}
+
+#endif /* WITH_THREADS */
+
+static void
+all_started (void)
+{}
+
+int
+main (void)
+{
+ alarm (30);
+
+#if WITH_THREADS
+ int ret = pthread_barrier_init (&barrier, NULL, 2);
+ assert (ret == 0);
+
+ pthread_t thread;
+ ret = pthread_create (&thread, NULL, thread_func, NULL);
+ assert (ret == 0);
+
+ pthread_barrier_wait (&barrier);
+#endif /* WITH_THREADS */
+
+ all_started ();
+
+ for (int i = 0; i < 30; i++)
+ sleep (1);
+
+ return 0;
+}
diff --git a/gdb/testsuite/gdb.base/run-attach-while-running.exp b/gdb/testsuite/gdb.base/run-attach-while-running.exp
new file mode 100644
index 000000000000..385bbc410dbf
--- /dev/null
+++ b/gdb/testsuite/gdb.base/run-attach-while-running.exp
@@ -0,0 +1,131 @@
+# Copyright 2021 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+
+# Test doing a "run" or an "attach" while the program is running.
+#
+# We test a non-threaded and a threaded configuration, so that targets that
+# don't support threads get some testing, but we also test with threads when
+# possible in case that triggers some multi-thread-specific bugs.
+
+standard_testfile
+
+set binfile_threads ${binfile}-threads
+set binfile_nothreads ${binfile}-nothreads
+unset binfile
+
+# Valid parameter / axis values:
+#
+# - non-stop: "off" of "on"
+# - threaded: 0 or 1
+# - run-or-attach: "run" or "attach"
+
+proc_with_prefix test { non-stop threaded run-or-attach } {
+ if { ${run-or-attach} == "attach" && ![can_spawn_for_attach] } {
+ unsupported "attach not supported"
+ return
+ }
+
+ # Choose the right (threaded or not) binfile.
+ if { $threaded } {
+ set binfile $::binfile_threads
+ } else {
+ set binfile $::binfile_nothreads
+ }
+
+ save_vars ::GDBFLAGS {
+ set ::GDBFLAGS "$::GDBFLAGS -ex \"set non-stop ${non-stop}\""
+
+ # The test doesn't work when the remote target uses the synchronous
+ # remote protocol, because GDB can't kill the remote inferior while it
+ # is running, when we "run" or "attach" again. When aswering "yes" to
+ # the "Start it from the beginning?" question, we otherwise get:
+ #
+ # Cannot execute this command while the target is running. Use the
+ # "interrupt" command to stop the target and then try again.
+ #
+ # Interrupting the target would defeat the purpose of the test. So
+ # when non-stop is off and using the remote target, force the target
+ # to use the async / non-stop version of the protocol.
+ if { [target_info exists gdb_protocol] && ${non-stop} == "off" } {
+ set ::GDBFLAGS "$::GDBFLAGS -ex \"maint set target-non-stop on\""
+ }
+
+ clean_restart $binfile
+ }
+
+ if { ![runto_main] } {
+ untested "could not run to main"
+ return
+ }
+
+ gdb_breakpoint "all_started" "temporary"
+ gdb_continue_to_breakpoint "continue to all_started"
+
+ # If all-stop, everything stopped when we hit the all_started breakpoint,
+ # so resume execution in background. If running the non-threaded version,
+ # our only thread is stopped in any case, so resume as well. But if we are
+ # in non-stop with two threads, we have one running and one stopped, leave
+ # it like this, it makes an interesting test case.
+ if { ${non-stop} == "off" || !${threaded} } {
+ gdb_test "continue &" "Continuing."
+ }
+
+ gdb_test_no_output "set confirm off"
+
+ # Run again (or, connect to a new stub if using a stub), take advantage
+ # of the fact that runto_main leaves the breakpoint on main in place.
+ if { ${run-or-attach} == "run" } {
+ gdb_run_cmd
+ gdb_test "" "Breakpoint $::decimal, .*main.*" "hit main breakpoint after re-run"
+ } elseif { ${run-or-attach} == "attach" } {
+ set test_spawn_id [spawn_wait_for_attach $binfile]
+ set test_pid [spawn_id_get_pid $test_spawn_id]
+
+ gdb_test "attach $test_pid" "Attaching to program: .*" "attach to process"
+
+ gdb_exit
+ kill_wait_spawned_process $test_spawn_id
+ } else {
+ error "Invalid value for run-or-attach"
+ }
+}
+
+# Build and test with the non-threaded version.
+if { [build_executable "failed to prepare" ${binfile_nothreads} ${srcfile} \
+ {debug additional_flags=-DWITH_THREADS=0} ] } {
+ return
+}
+
+with_test_prefix {threaded=0} {
+ foreach_with_prefix run-or-attach {run attach} {
+ foreach_with_prefix non-stop {off on} {
+ test ${non-stop} 0 ${run-or-attach}
+ }
+ }
+}
+
+# Build and test with the threaded version.
+if { [build_executable "failed to prepare" ${binfile_threads} ${srcfile} \
+ {debug pthreads additional_flags=-DWITH_THREADS=1} ] } {
+ return
+}
+
+with_test_prefix {threaded=1} {
+ foreach_with_prefix run-or-attach {run attach} {
+ foreach_with_prefix non-stop {off on} {
+ test ${non-stop} 1 ${run-or-attach}
+ }
+ }
+}
--
2.30.0
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v4 2/3] gdb: move commit_resume to process_stratum_target
2021-01-25 4:57 [PATCH v4 0/3] Reduce back and forth with target when threads have pending statuses Simon Marchi
2021-01-25 4:57 ` [PATCH v4 1/3] gdb/testsuite: add test for run/attach while program is running Simon Marchi
@ 2021-01-25 4:57 ` Simon Marchi
2021-01-25 4:57 ` [PATCH v4 3/3] gdb: generalize commit_resume, avoid commit-resuming when threads have pending statuses Simon Marchi
2021-02-03 1:35 ` [PATCH v4 0/3] Reduce back and forth with target " Pedro Alves
3 siblings, 0 replies; 8+ messages in thread
From: Simon Marchi @ 2021-01-25 4:57 UTC (permalink / raw)
To: gdb-patches; +Cc: Simon Marchi
From: Simon Marchi <simon.marchi@efficios.com>
The following patch will change the commit_resume target method to
something stateful. Because it would be difficult to track a state
replicated in the various targets of a target stack, and since for the
foreseeable future, only process stratum targets are going to use this
concept, this patch makes the commit resume concept specific to process
stratum targets.
So, move the method to process_stratum_target, and move helper functions
to process-stratum-target.h.
gdb/ChangeLog:
* target.h (struct target_ops) <commit_resume>: New.
(target_commit_resume): Remove.
(make_scoped_defer_target_commit_resume): Remove.
* target.c (defer_target_commit_resume): Remove.
(target_commit_resume): Remove.
(make_scoped_defer_target_commit_resume): Remove.
* process-stratum-target.h (class process_stratum_target)
<commit_resume>: New.
(maybe_commit_resume_all_process_targets): New.
(make_scoped_defer_process_target_commit_resume): New.
* process-stratum-target.c (defer_process_target_commit_resume):
New.
(maybe_commit_resume_process_target): New.
(make_scoped_defer_process_target_commit_resume): New.
* infrun.c (do_target_resume): Adjust.
(commit_resume_all_targets): Rename into...
(maybe_commit_resume_all_process_targets): ... this, adjust.
(proceed): Adjust.
* record-full.c (record_full_wait_1): Adjust.
* target-delegates.c: Re-generate.
Change-Id: Ifc957817ac5b2303e22760ce3d14740b9598f02c
---
gdb/infrun.c | 28 +++++++++-------------------
gdb/process-stratum-target.c | 23 +++++++++++++++++++++++
gdb/process-stratum-target.h | 29 +++++++++++++++++++++++++++++
gdb/record-full.c | 8 ++++----
gdb/target-delegates.c | 22 ----------------------
gdb/target.c | 22 ----------------------
gdb/target.h | 20 --------------------
7 files changed, 65 insertions(+), 87 deletions(-)
diff --git a/gdb/infrun.c b/gdb/infrun.c
index e70e3214d7b3..ad6f276038fc 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -2172,7 +2172,7 @@ do_target_resume (ptid_t resume_ptid, bool step, enum gdb_signal sig)
target_resume (resume_ptid, step, sig);
- target_commit_resume ();
+ maybe_commit_resume_process_target (tp->inf->process_target ());
if (target_can_async_p ())
target_async (1);
@@ -2760,28 +2760,17 @@ schedlock_applies (struct thread_info *tp)
execution_direction)));
}
-/* Calls target_commit_resume on all targets. */
+/* Calls maybe_commit_resume_process_target on all process targets. */
static void
-commit_resume_all_targets ()
+maybe_commit_resume_all_process_targets ()
{
scoped_restore_current_thread restore_thread;
- /* Map between process_target and a representative inferior. This
- is to avoid committing a resume in the same target more than
- once. Resumptions must be idempotent, so this is an
- optimization. */
- std::unordered_map<process_stratum_target *, inferior *> conn_inf;
-
- for (inferior *inf : all_non_exited_inferiors ())
- if (inf->has_execution ())
- conn_inf[inf->process_target ()] = inf;
-
- for (const auto &ci : conn_inf)
+ for (process_stratum_target *target : all_non_exited_process_targets ())
{
- inferior *inf = ci.second;
- switch_to_inferior_no_thread (inf);
- target_commit_resume ();
+ switch_to_target_no_thread (target);
+ maybe_commit_resume_process_target (target);
}
}
@@ -3005,7 +2994,8 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal)
cur_thr->prev_pc = regcache_read_pc_protected (regcache);
{
- scoped_restore save_defer_tc = make_scoped_defer_target_commit_resume ();
+ scoped_restore save_defer_tc
+ = make_scoped_defer_process_target_commit_resume ();
started = start_step_over ();
@@ -3075,7 +3065,7 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal)
}
}
- commit_resume_all_targets ();
+ maybe_commit_resume_all_process_targets ();
finish_state.release ();
diff --git a/gdb/process-stratum-target.c b/gdb/process-stratum-target.c
index 719167803fff..ec127f524b74 100644
--- a/gdb/process-stratum-target.c
+++ b/gdb/process-stratum-target.c
@@ -108,3 +108,26 @@ switch_to_target_no_thread (process_stratum_target *target)
break;
}
}
+
+/* If true, `maybe_commit_resume_process_target` is a no-op. */
+
+static bool defer_process_target_commit_resume;
+
+/* See process-stratum-target.h. */
+
+void
+maybe_commit_resume_process_target (process_stratum_target *proc_target)
+{
+ if (defer_process_target_commit_resume)
+ return;
+
+ proc_target->commit_resume ();
+}
+
+/* See process-stratum-target.h. */
+
+scoped_restore_tmpl<bool>
+make_scoped_defer_process_target_commit_resume ()
+{
+ return make_scoped_restore (&defer_process_target_commit_resume, true);
+}
diff --git a/gdb/process-stratum-target.h b/gdb/process-stratum-target.h
index b513c26ffc2a..c8060c46be93 100644
--- a/gdb/process-stratum-target.h
+++ b/gdb/process-stratum-target.h
@@ -63,6 +63,20 @@ class process_stratum_target : public target_ops
bool has_registers () override;
bool has_execution (inferior *inf) override;
+ /* Commit a series of resumption requests previously prepared with
+ resume calls.
+
+ GDB always calls `commit_resume` on the process stratum target after
+ calling `resume` on a target stack. A process stratum target may thus use
+ this method in coordination with its `resume` method to batch resumption
+ requests. In that case, the target doesn't actually resume in its
+ `resume` implementation. Instead, it takes note of resumption intent in
+ `resume`, and defers the actual resumption `commit_resume`.
+
+ E.g., the remote target uses this to coalesce multiple resumption requests
+ in a single vCont packet. */
+ virtual void commit_resume () {}
+
/* True if any thread is, or may be executing. We need to track
this separately because until we fully sync the thread list, we
won't know whether the target is fully stopped, even if we see
@@ -92,4 +106,19 @@ extern std::set<process_stratum_target *> all_non_exited_process_targets ();
extern void switch_to_target_no_thread (process_stratum_target *target);
+/* Commit a series of resumption requests previously prepared with
+ target_resume calls.
+
+ This function is a no-op if commit resumes are deferred (see
+ `make_scoped_defer_process_target_commit_resume`). */
+
+extern void maybe_commit_resume_process_target
+ (process_stratum_target *target);
+
+/* Setup to defer `commit_resume` calls, and re-set to the previous status on
+ destruction. */
+
+extern scoped_restore_tmpl<bool>
+ make_scoped_defer_process_target_commit_resume ();
+
#endif /* !defined (PROCESS_STRATUM_TARGET_H) */
diff --git a/gdb/record-full.c b/gdb/record-full.c
index c3307e812e71..464f4a85c558 100644
--- a/gdb/record-full.c
+++ b/gdb/record-full.c
@@ -1243,11 +1243,11 @@ record_full_wait_1 (struct target_ops *ops,
break;
}
+ process_stratum_target *proc_target
+ = current_inferior ()->process_target ();
+
if (gdbarch_software_single_step_p (gdbarch))
{
- process_stratum_target *proc_target
- = current_inferior ()->process_target ();
-
/* Try to insert the software single step breakpoint.
If insert success, set step to 0. */
set_executing (proc_target, inferior_ptid, false);
@@ -1264,7 +1264,7 @@ record_full_wait_1 (struct target_ops *ops,
"issuing one more step in the "
"target beneath\n");
ops->beneath ()->resume (ptid, step, GDB_SIGNAL_0);
- ops->beneath ()->commit_resume ();
+ proc_target->commit_resume ();
continue;
}
}
diff --git a/gdb/target-delegates.c b/gdb/target-delegates.c
index 437b19b8581c..8b933fdf82eb 100644
--- a/gdb/target-delegates.c
+++ b/gdb/target-delegates.c
@@ -14,7 +14,6 @@ struct dummy_target : public target_ops
void detach (inferior *arg0, int arg1) override;
void disconnect (const char *arg0, int arg1) override;
void resume (ptid_t arg0, int arg1, enum gdb_signal arg2) override;
- void commit_resume () override;
ptid_t wait (ptid_t arg0, struct target_waitstatus *arg1, target_wait_flags arg2) override;
void fetch_registers (struct regcache *arg0, int arg1) override;
void store_registers (struct regcache *arg0, int arg1) override;
@@ -185,7 +184,6 @@ struct debug_target : public target_ops
void detach (inferior *arg0, int arg1) override;
void disconnect (const char *arg0, int arg1) override;
void resume (ptid_t arg0, int arg1, enum gdb_signal arg2) override;
- void commit_resume () override;
ptid_t wait (ptid_t arg0, struct target_waitstatus *arg1, target_wait_flags arg2) override;
void fetch_registers (struct regcache *arg0, int arg1) override;
void store_registers (struct regcache *arg0, int arg1) override;
@@ -440,26 +438,6 @@ debug_target::resume (ptid_t arg0, int arg1, enum gdb_signal arg2)
fputs_unfiltered (")\n", gdb_stdlog);
}
-void
-target_ops::commit_resume ()
-{
- this->beneath ()->commit_resume ();
-}
-
-void
-dummy_target::commit_resume ()
-{
-}
-
-void
-debug_target::commit_resume ()
-{
- fprintf_unfiltered (gdb_stdlog, "-> %s->commit_resume (...)\n", this->beneath ()->shortname ());
- this->beneath ()->commit_resume ();
- fprintf_unfiltered (gdb_stdlog, "<- %s->commit_resume (", this->beneath ()->shortname ());
- fputs_unfiltered (")\n", gdb_stdlog);
-}
-
ptid_t
target_ops::wait (ptid_t arg0, struct target_waitstatus *arg1, target_wait_flags arg2)
{
diff --git a/gdb/target.c b/gdb/target.c
index 9a8473d40e1a..58fd75687d0a 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -2053,28 +2053,6 @@ target_resume (ptid_t ptid, int step, enum gdb_signal signal)
clear_inline_frame_state (curr_target, ptid);
}
-/* If true, target_commit_resume is a nop. */
-static int defer_target_commit_resume;
-
-/* See target.h. */
-
-void
-target_commit_resume (void)
-{
- if (defer_target_commit_resume)
- return;
-
- current_top_target ()->commit_resume ();
-}
-
-/* See target.h. */
-
-scoped_restore_tmpl<int>
-make_scoped_defer_target_commit_resume ()
-{
- return make_scoped_restore (&defer_target_commit_resume, 1);
-}
-
void
target_pass_signals (gdb::array_view<const unsigned char> pass_signals)
{
diff --git a/gdb/target.h b/gdb/target.h
index 917476d16ab6..c92ecd0f70d7 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -478,8 +478,6 @@ struct target_ops
int TARGET_DEBUG_PRINTER (target_debug_print_step),
enum gdb_signal)
TARGET_DEFAULT_NORETURN (noprocess ());
- virtual void commit_resume ()
- TARGET_DEFAULT_IGNORE ();
/* See target_wait's description. Note that implementations of
this method must not assume that inferior_ptid on entry is
pointing at the thread or inferior that ends up reporting an
@@ -1431,24 +1429,6 @@ extern void target_disconnect (const char *, int);
target_commit_resume below. */
extern void target_resume (ptid_t ptid, int step, enum gdb_signal signal);
-/* Commit a series of resumption requests previously prepared with
- target_resume calls.
-
- GDB always calls target_commit_resume after calling target_resume
- one or more times. A target may thus use this method in
- coordination with the target_resume method to batch target-side
- resumption requests. In that case, the target doesn't actually
- resume in its target_resume implementation. Instead, it prepares
- the resumption in target_resume, and defers the actual resumption
- to target_commit_resume. E.g., the remote target uses this to
- coalesce multiple resumption requests in a single vCont packet. */
-extern void target_commit_resume ();
-
-/* Setup to defer target_commit_resume calls, and reactivate
- target_commit_resume on destruction, if it was previously
- active. */
-extern scoped_restore_tmpl<int> make_scoped_defer_target_commit_resume ();
-
/* For target_read_memory see target/target.h. */
/* The default target_ops::to_wait implementation. */
--
2.30.0
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v4 3/3] gdb: generalize commit_resume, avoid commit-resuming when threads have pending statuses
2021-01-25 4:57 [PATCH v4 0/3] Reduce back and forth with target when threads have pending statuses Simon Marchi
2021-01-25 4:57 ` [PATCH v4 1/3] gdb/testsuite: add test for run/attach while program is running Simon Marchi
2021-01-25 4:57 ` [PATCH v4 2/3] gdb: move commit_resume to process_stratum_target Simon Marchi
@ 2021-01-25 4:57 ` Simon Marchi
2021-02-06 18:05 ` Pedro Alves
2021-02-03 1:35 ` [PATCH v4 0/3] Reduce back and forth with target " Pedro Alves
3 siblings, 1 reply; 8+ messages in thread
From: Simon Marchi @ 2021-01-25 4:57 UTC (permalink / raw)
To: gdb-patches
New in v4:
- Move assertions from the linux-nat code to
target_wait/target_resume/target_stop.
- Don't call the commit_resumed method in a destructor. plit
maybe_commit_resumed_all_process_targets into
maybe_set_commit_resumed_all_process_targets, which only (maybe)
sets the commit_resumed_state flag of process targets, and
maybe_call_commit_resumed_all_process_targets, which calls the
commit_resumed method on all process targets with that have the
commit_resumed_state flag set. ~scoped_disable_commit_resumed only
calls maybe_set_commit_resumed_all_process_targets, and
maybe_call_commit_resumed_all_process_targets is called manually
after that.
- Don't use optional in maybe_commit_resumed_all_process_targets (now
maybe_set_commit_resumed_all_process_targets)
- remote: when a phony stop reply is enqueued, because we stop a
thread that wasn't commit-resumed, we move its remote resume state
to "resumed".
- infcmd: move scoped_disable_commit_resumed higher to widen its
scope.
The rationale for this patch comes from the ROCm port [1], the goal
being to reduce the number of back and forths between GDB and the target
when doing successive operations. I'll start with explaining the
rationale and then go over the implementation. In the ROCm / GPU world,
the term "wave" is somewhat equivalent to a "thread" in GDB. So if you
read if from a GPU stand point, just s/thread/wave/.
ROCdbgapi, the library used by GDB [2] to communicate with the GPU
target, gives the illusion that it's possible for the debugger to
control (start and stop) individual threads. But in reality, this is
not how it works. Under the hood, all threads of a queue are controlled
as a group. To stop one thread in a group of running ones, the state of
all threads is retrieved from the GPU, all threads are destroyed, and all
threads but the one we want to stop are re-created from the saved state.
The net result, from the point of view of GDB, is that the library
stopped one thread. The same thing goes if we want to resume one thread
while others are running: the state of all running threads is retrieved
from the GPU, they are all destroyed, and they are all re-created,
including the thread we want to resume.
This leads to some inefficiencies when combined with how GDB works, here
are two examples:
- Stopping all threads: because the target operates in non-stop mode,
when the user interface mode is all-stop, GDB must stop all threads
individually when presenting a stop. Let's suppose we have 1000
threads and the user does ^C. GDB asks the target to stop one
thread. Behind the scenes, the library retrieves 1000 thread states
and restores the 999 others still running ones. GDB asks the target
to stop another one. The target retrieves 999 thread states and
restores the 998 remaining ones. That means that to stop 1000
threads, we did 1000 back and forths with the GPU. It would have
been much better to just retrieve the states once and stop there.
- Resuming with pending events: suppose the 1000 threads hit a
breakpoint at the same time. The breakpoint is conditional and
evaluates to true for the first thread, to false for all others. GDB
pulls one event (for the first thread) from the target, decides that
it should present a stop, so stops all threads using
stop_all_threads. All these other threads have a breakpoint event to
report, which is saved in `thread_info::suspend::waitstatus` for
later. When the user does "continue", GDB resumes that one thread
that did hit the breakpoint. It then processes the pending events
one by one as if they just arrived. It picks one, evaluates the
condition to false, and resumes the thread. It picks another one,
evaluates the condition to false, and resumes the thread. And so on.
In between each resumption, there is a full state retrieval and
re-creation. It would be much nicer if we could wait a little bit
before sending those threads on the GPU, until it processed all those
pending events.
To address this kind of performance issue, ROCdbgapi has a concept
called "forward progress required", which is a boolean state that allows
its user (i.e. GDB) to say "I'm doing a bunch of operations, you can
hold off putting the threads on the GPU until I'm done" (the "forward
progress not required" state). Turning forward progress back on
indicates to the library that all threads that are supposed to be
running should now be really running on the GPU.
It turns out that GDB has a similar concept, though not as general,
commit_resume. One difference is that commit_resume is not stateful:
the target can't look up "does the core need me to schedule resumed
threads for execution right now". It is also specifically linked to the
resume method, it is not used in other contexts. The target accumulates
resumption requests through target_ops::resume calls, and then commits
those resumptions when target_ops::commit_resume is called. The target
has no way to check if it's ok to leave resumed threads stopped in other
target methods.
To bridge the gap, this patch generalizes the commit_resume concept in
GDB to match the forward progress concept of ROCdbgapi. The current
name (commit_resume) can be interpreted as "commit the previous resume
calls". I renamed the concept to "commit_resumed", as in "commit the
threads that are resumed".
In the new version, we have two things in process_stratum_target:
- the commit_resumed_state field: indicates whether GDB requires this
target to have resumed threads committed to the execution
target/device. If false, the target is allowed to leave resumed
threads un-committed at the end of whatever method it is executing.
- the commit_resumed method: called when commit_resumed_state
transitions from false to true. While commit_resumed_state was
false, the target may have left some resumed threads un-committed.
This method being called tells it that it should commit them back to
the execution device.
Let's take the "Stopping all threads" scenario from above and see how it
would work with the ROCm target with this change. Before stopping all
threads, GDB would set the target's commit_resumed_state field to false.
It would then ask the target to stop the first thread. The target would
retrieve all threads' state from the GPU and mark that one as stopped.
Since commit_resumed_state is false, it leaves all the other threads
(still resumed) stopped. GDB would then proceed to call target_stop for
all the other threads. Since resumed threads are not committed, this
doesn't do any back and forth with the GPU.
To simplify the implementation of targets, I made it so that when
calling certain target methods, the contract between the core and the
targets guarantees that commit_resumed_state is false. This way, the
target doesn't need two paths, one commit_resumed_state == true and one
for commit_resumed_state == false. It can just assert that
commit_resumed_state is false and work with that assumption. This also
helps catch places where we forgot to disable commit_resumed_state
before calling the method, which represents a probable optimization
opportunity. I added assertions in the target method wrappers
(target_resume and friends) to have some confidence that this contract
between the core and the targets is respected.
The scoped_disable_commit_resumed type is used to disable the commit
resumed state of all process targets on construction, and selectively re-enable it on destruction (see below for criteria).
Note that it only sets the process_stratum_target::commit_resumed_state
flag. A subsequent call to
maybe_call_commit_resumed_all_process_targets is necessary to call the
process_stratum_target::commit_resumed method on all process targets
that got their commit_resumed_state flag turned back on. This
separation is because we don't want to call the commit_resumed methods
in scoped_disable_commit_resumed's destructor, as they can throw.
On destruction, commit-resumed is not re-enabled for a given target if:
1. this target has no threads resumed, or
2. this target has at least one resumed thread with a pending status known to
the core (saved in thread_info::suspend::waitstatus).
The first point is not technically necessary, because a proper
commit_resumed implementation would be a no-op if the target has no
resumed threads. But since we have a flag do to a quick check, I think
it doesn't hurt.
The second point is more important: together with the
scoped_disable_commit_resumed instance added in fetch_inferior_event, it
makes it so the "Resuming with pending events" described above is
handled efficiently. Here's what happens in that case:
1. The user types "continue".
2. Upon destruction, the scoped_disable_commit_resumed in the `proceed`
function does not enable commit-resumed, as it sees some threads
have pending statuses.
3. fetch_inferior_event is called to handle another event, the
breakpoint hit evaluates to false, and that thread is resumed.
Because there are still more threads with pending statuses, the
destructor of scoped_disable_commit_resumed in fetch_inferior_event
still doesn't enable commit-resumed.
4. Rinse and repeat step 3, until the last pending status is handled by
fetch_inferior_event. In that case, scoped_disable_commit_resumed's
destructor sees there are no more threads with pending statues, so
it asks the target to commit resumed threads.
This allows us to avoid all unnecessary back and forths, there is a
single commit_resumed call once all pending statuses are processed.
This change required remote_target::remote_stop_ns to learn how to
handle stopping threads that were resumed but pending vCont. The
simplest example where that happens is when using the remote target in
all-stop, but with "maint set target-non-stop on", to force it to
operate in non-stop mode under the hood. If two threads hit a
breakpoint at the same time, GDB will receive two stop replies. It will
present the stop for one thread and save the other one in
thread_info::suspend::waitstatus.
Before this patch, when doing "continue", GDB first resumes the thread
without a pending status:
Sending packet: $vCont;c:p172651.172676#f3
It then consumes the pending status in the next fetch_inferior_event
call:
[infrun] do_target_wait_1: Using pending wait status status->kind = stopped, signal = GDB_SIGNAL_TRAP for Thread 1517137.1517137.
[infrun] target_wait (-1.0.0, status) =
[infrun] 1517137.1517137.0 [Thread 1517137.1517137],
[infrun] status->kind = stopped, signal = GDB_SIGNAL_TRAP
It then realizes it needs to stop all threads to present the stop, so
stops the thread it just resumed:
[infrun] stop_all_threads: Thread 1517137.1517137 not executing
[infrun] stop_all_threads: Thread 1517137.1517174 executing, need stop
remote_stop called
Sending packet: $vCont;t:p172651.172676#04
This is an unnecessary resume/stop. With this patch, we don't commit
resumed threads after proceeding, because of the pending status:
[infrun] maybe_commit_resumed_all_process_targets: not requesting commit-resumed for target extended-remote, a thread has a pending waitstatus
When GDB handles the pending status and stop_all_threads runs, we stop a
resumed but pending vCont thread:
remote_stop_ns: Enqueueing phony stop reply for thread pending vCont-resume (1520940, 1520976, 0)
That thread was never actually resumed on the remote stub / gdbserver,
so we shouldn't send a packet to the remote side asking to stop the
thread.
Note that this patch only considers pending statuses known to the core
of GDB, that is the events that were pulled out of the target and stored
in `thread_info::suspend::waitstatus`. In some cases, we could also
avoid unnecessary back and forth when the target has events that it has
not yet reported the core. I plan to implement this as a subsequent
patch, once this series has settled.
gdb/ChangeLog:
* infrun.h (struct scoped_disable_commit_resumed): New.
(maybe_call_commit_resumed_all_process_targets): New.
* infrun.c (do_target_resume): Remove
maybe_commit_resume_process_target call.
(maybe_commit_resume_all_process_targets): Remove.
(scoped_disable_commit_resumed::maybe_set_commit_resumed_all_process_targets):
New.
(maybe_call_commit_resumed_all_process_targets): New.
(defer_enable_commit_resumed): New.
(scoped_disable_commit_resumed::scoped_disable_commit_resumed):
New.
(scoped_disable_commit_resumed::~scoped_disable_commit_resumed):
New.
(proceed): Use scoped_disable_commit_resumed.
(fetch_inferior_event): Use scoped_disable_commit_resumed.
* process-stratum-target.h (class process_stratum_target):
<commit_resume>: Rename to...
<commit_resumed>: ... this.
<commit_resumed_state>: New.
(all_process_targets): New.
(maybe_commit_resume_process_target): Remove.
(make_scoped_defer_process_target_commit_resume): Remove.
* process-stratum-target.c (all_process_targets): New.
(defer_process_target_commit_resume): Remove.
(maybe_commit_resume_process_target): Remove.
(make_scoped_defer_process_target_commit_resume): Remove.
* infcmd.c (run_command_1): Use scoped_disable_commit_resumed.
(attach_command): Use scoped_disable_commit_resumed.
(detach_command): Use scoped_disable_commit_resumed.
(interrupt_target_1): Use scoped_disable_commit_resumed.
* mi/mi-main.c (exec_continue): Use
scoped_disable_commit_resumed.
* record-full.c (record_full_wait_1): Change
commit_resumed_state around calling commit_resumed.
* remote.c (class remote_target) <commit_resume>: Rename to...
<commit_resumed>: ... this.
(remote_target::resume): Add gdb_assert.
(remote_target::commit_resume): Rename to...
(remote_target::commit_resumed): ... this. Check if there is
any thread pending vCont resume.
(struct stop_reply): Move up.
(remote_target::remote_stop_ns): Generate stop replies for
resumed but pending vCont threads.
(remote_target::wait_ns): Add gdb_assert.
[1] https://github.com/ROCm-Developer-Tools/ROCgdb/
[2] https://github.com/ROCm-Developer-Tools/ROCdbgapi
Change-Id: I836135531a29214b21695736deb0a81acf8cf566
---
gdb/infcmd.c | 490 ++++++++++++++++++-----------------
gdb/infrun.c | 137 +++++++++-
gdb/infrun.h | 59 +++++
gdb/mi/mi-main.c | 88 ++++---
gdb/process-stratum-target.c | 37 +--
gdb/process-stratum-target.h | 63 +++--
gdb/record-full.c | 4 +-
gdb/remote.c | 123 ++++++---
gdb/target.c | 8 +
9 files changed, 642 insertions(+), 367 deletions(-)
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index ebaf57592ef5..560a2d84c0b7 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -419,125 +419,131 @@ run_command_1 (const char *args, int from_tty, enum run_how run_how)
dont_repeat ();
- kill_if_already_running (from_tty);
+ {
+ scoped_disable_commit_resumed disable_commit_resumed ("running");
- init_wait_for_inferior ();
- clear_breakpoint_hit_counts ();
+ kill_if_already_running (from_tty);
- /* Clean up any leftovers from other runs. Some other things from
- this function should probably be moved into target_pre_inferior. */
- target_pre_inferior (from_tty);
+ init_wait_for_inferior ();
+ clear_breakpoint_hit_counts ();
- /* The comment here used to read, "The exec file is re-read every
- time we do a generic_mourn_inferior, so we just have to worry
- about the symbol file." The `generic_mourn_inferior' function
- gets called whenever the program exits. However, suppose the
- program exits, and *then* the executable file changes? We need
- to check again here. Since reopen_exec_file doesn't do anything
- if the timestamp hasn't changed, I don't see the harm. */
- reopen_exec_file ();
- reread_symbols ();
+ /* Clean up any leftovers from other runs. Some other things from
+ this function should probably be moved into target_pre_inferior. */
+ target_pre_inferior (from_tty);
- gdb::unique_xmalloc_ptr<char> stripped = strip_bg_char (args, &async_exec);
- args = stripped.get ();
+ /* The comment here used to read, "The exec file is re-read every
+ time we do a generic_mourn_inferior, so we just have to worry
+ about the symbol file." The `generic_mourn_inferior' function
+ gets called whenever the program exits. However, suppose the
+ program exits, and *then* the executable file changes? We need
+ to check again here. Since reopen_exec_file doesn't do anything
+ if the timestamp hasn't changed, I don't see the harm. */
+ reopen_exec_file ();
+ reread_symbols ();
- /* Do validation and preparation before possibly changing anything
- in the inferior. */
+ gdb::unique_xmalloc_ptr<char> stripped = strip_bg_char (args, &async_exec);
+ args = stripped.get ();
- run_target = find_run_target ();
+ /* Do validation and preparation before possibly changing anything
+ in the inferior. */
- prepare_execution_command (run_target, async_exec);
+ run_target = find_run_target ();
- if (non_stop && !run_target->supports_non_stop ())
- error (_("The target does not support running in non-stop mode."));
+ prepare_execution_command (run_target, async_exec);
- /* Done. Can now set breakpoints, change inferior args, etc. */
+ if (non_stop && !run_target->supports_non_stop ())
+ error (_("The target does not support running in non-stop mode."));
- /* Insert temporary breakpoint in main function if requested. */
- if (run_how == RUN_STOP_AT_MAIN)
- {
- std::string arg = string_printf ("-qualified %s", main_name ());
- tbreak_command (arg.c_str (), 0);
- }
+ /* Done. Can now set breakpoints, change inferior args, etc. */
- exec_file = get_exec_file (0);
+ /* Insert temporary breakpoint in main function if requested. */
+ if (run_how == RUN_STOP_AT_MAIN)
+ {
+ std::string arg = string_printf ("-qualified %s", main_name ());
+ tbreak_command (arg.c_str (), 0);
+ }
- /* We keep symbols from add-symbol-file, on the grounds that the
- user might want to add some symbols before running the program
- (right?). But sometimes (dynamic loading where the user manually
- introduces the new symbols with add-symbol-file), the code which
- the symbols describe does not persist between runs. Currently
- the user has to manually nuke all symbols between runs if they
- want them to go away (PR 2207). This is probably reasonable. */
+ exec_file = get_exec_file (0);
- /* If there were other args, beside '&', process them. */
- if (args != NULL)
- set_inferior_args (args);
+ /* We keep symbols from add-symbol-file, on the grounds that the
+ user might want to add some symbols before running the program
+ (right?). But sometimes (dynamic loading where the user manually
+ introduces the new symbols with add-symbol-file), the code which
+ the symbols describe does not persist between runs. Currently
+ the user has to manually nuke all symbols between runs if they
+ want them to go away (PR 2207). This is probably reasonable. */
- if (from_tty)
- {
- uiout->field_string (NULL, "Starting program");
- uiout->text (": ");
- if (exec_file)
- uiout->field_string ("execfile", exec_file);
- uiout->spaces (1);
- /* We call get_inferior_args() because we might need to compute
- the value now. */
- uiout->field_string ("infargs", get_inferior_args ());
- uiout->text ("\n");
- uiout->flush ();
- }
+ /* If there were other args, beside '&', process them. */
+ if (args != NULL)
+ set_inferior_args (args);
- /* We call get_inferior_args() because we might need to compute
- the value now. */
- run_target->create_inferior (exec_file,
- std::string (get_inferior_args ()),
- current_inferior ()->environment.envp (),
- from_tty);
- /* to_create_inferior should push the target, so after this point we
- shouldn't refer to run_target again. */
- run_target = NULL;
-
- /* We're starting off a new process. When we get out of here, in
- non-stop mode, finish the state of all threads of that process,
- but leave other threads alone, as they may be stopped in internal
- events --- the frontend shouldn't see them as stopped. In
- all-stop, always finish the state of all threads, as we may be
- resuming more than just the new process. */
- process_stratum_target *finish_target;
- ptid_t finish_ptid;
- if (non_stop)
- {
- finish_target = current_inferior ()->process_target ();
- finish_ptid = ptid_t (current_inferior ()->pid);
- }
- else
- {
- finish_target = nullptr;
- finish_ptid = minus_one_ptid;
- }
- scoped_finish_thread_state finish_state (finish_target, finish_ptid);
+ if (from_tty)
+ {
+ uiout->field_string (NULL, "Starting program");
+ uiout->text (": ");
+ if (exec_file)
+ uiout->field_string ("execfile", exec_file);
+ uiout->spaces (1);
+ /* We call get_inferior_args() because we might need to compute
+ the value now. */
+ uiout->field_string ("infargs", get_inferior_args ());
+ uiout->text ("\n");
+ uiout->flush ();
+ }
- /* Pass zero for FROM_TTY, because at this point the "run" command
- has done its thing; now we are setting up the running program. */
- post_create_inferior (0);
+ /* We call get_inferior_args() because we might need to compute
+ the value now. */
+ run_target->create_inferior (exec_file,
+ std::string (get_inferior_args ()),
+ current_inferior ()->environment.envp (),
+ from_tty);
+ /* to_create_inferior should push the target, so after this point we
+ shouldn't refer to run_target again. */
+ run_target = NULL;
+
+ /* We're starting off a new process. When we get out of here, in
+ non-stop mode, finish the state of all threads of that process,
+ but leave other threads alone, as they may be stopped in internal
+ events --- the frontend shouldn't see them as stopped. In
+ all-stop, always finish the state of all threads, as we may be
+ resuming more than just the new process. */
+ process_stratum_target *finish_target;
+ ptid_t finish_ptid;
+ if (non_stop)
+ {
+ finish_target = current_inferior ()->process_target ();
+ finish_ptid = ptid_t (current_inferior ()->pid);
+ }
+ else
+ {
+ finish_target = nullptr;
+ finish_ptid = minus_one_ptid;
+ }
+ scoped_finish_thread_state finish_state (finish_target, finish_ptid);
- /* Queue a pending event so that the program stops immediately. */
- if (run_how == RUN_STOP_AT_FIRST_INSN)
- {
- thread_info *thr = inferior_thread ();
- thr->suspend.waitstatus_pending_p = 1;
- thr->suspend.waitstatus.kind = TARGET_WAITKIND_STOPPED;
- thr->suspend.waitstatus.value.sig = GDB_SIGNAL_0;
- }
+ /* Pass zero for FROM_TTY, because at this point the "run" command
+ has done its thing; now we are setting up the running program. */
+ post_create_inferior (0);
+
+ /* Queue a pending event so that the program stops immediately. */
+ if (run_how == RUN_STOP_AT_FIRST_INSN)
+ {
+ thread_info *thr = inferior_thread ();
+ thr->suspend.waitstatus_pending_p = 1;
+ thr->suspend.waitstatus.kind = TARGET_WAITKIND_STOPPED;
+ thr->suspend.waitstatus.value.sig = GDB_SIGNAL_0;
+ }
- /* Start the target running. Do not use -1 continuation as it would skip
- breakpoint right at the entry point. */
- proceed (regcache_read_pc (get_current_regcache ()), GDB_SIGNAL_0);
+ /* Start the target running. Do not use -1 continuation as it would skip
+ breakpoint right at the entry point. */
+ proceed (regcache_read_pc (get_current_regcache ()), GDB_SIGNAL_0);
- /* Since there was no error, there's no need to finish the thread
- states here. */
- finish_state.release ();
+ /* Since there was no error, there's no need to finish the thread
+ states here. */
+ finish_state.release ();
+ }
+
+ maybe_call_commit_resumed_all_process_targets ();
}
static void
@@ -2565,114 +2571,120 @@ attach_command (const char *args, int from_tty)
dont_repeat (); /* Not for the faint of heart */
- if (gdbarch_has_global_solist (target_gdbarch ()))
- /* Don't complain if all processes share the same symbol
- space. */
- ;
- else if (target_has_execution ())
- {
- if (query (_("A program is being debugged already. Kill it? ")))
- target_kill ();
- else
- error (_("Not killed."));
- }
-
- /* Clean up any leftovers from other runs. Some other things from
- this function should probably be moved into target_pre_inferior. */
- target_pre_inferior (from_tty);
-
- gdb::unique_xmalloc_ptr<char> stripped = strip_bg_char (args, &async_exec);
- args = stripped.get ();
-
- attach_target = find_attach_target ();
+ {
+ scoped_disable_commit_resumed disable_commit_resumed ("attaching");
- prepare_execution_command (attach_target, async_exec);
+ if (gdbarch_has_global_solist (target_gdbarch ()))
+ /* Don't complain if all processes share the same symbol
+ space. */
+ ;
+ else if (target_has_execution ())
+ {
+ if (query (_("A program is being debugged already. Kill it? ")))
+ target_kill ();
+ else
+ error (_("Not killed."));
+ }
- if (non_stop && !attach_target->supports_non_stop ())
- error (_("Cannot attach to this target in non-stop mode"));
+ /* Clean up any leftovers from other runs. Some other things from
+ this function should probably be moved into target_pre_inferior. */
+ target_pre_inferior (from_tty);
- attach_target->attach (args, from_tty);
- /* to_attach should push the target, so after this point we
- shouldn't refer to attach_target again. */
- attach_target = NULL;
+ gdb::unique_xmalloc_ptr<char> stripped = strip_bg_char (args, &async_exec);
+ args = stripped.get ();
- /* Set up the "saved terminal modes" of the inferior
- based on what modes we are starting it with. */
- target_terminal::init ();
+ attach_target = find_attach_target ();
- /* Install inferior's terminal modes. This may look like a no-op,
- as we've just saved them above, however, this does more than
- restore terminal settings:
+ prepare_execution_command (attach_target, async_exec);
- - installs a SIGINT handler that forwards SIGINT to the inferior.
- Otherwise a Ctrl-C pressed just while waiting for the initial
- stop would end up as a spurious Quit.
+ if (non_stop && !attach_target->supports_non_stop ())
+ error (_("Cannot attach to this target in non-stop mode"));
- - removes stdin from the event loop, which we need if attaching
- in the foreground, otherwise on targets that report an initial
- stop on attach (which are most) we'd process input/commands
- while we're in the event loop waiting for that stop. That is,
- before the attach continuation runs and the command is really
- finished. */
- target_terminal::inferior ();
+ attach_target->attach (args, from_tty);
+ /* to_attach should push the target, so after this point we
+ shouldn't refer to attach_target again. */
+ attach_target = NULL;
- /* Set up execution context to know that we should return from
- wait_for_inferior as soon as the target reports a stop. */
- init_wait_for_inferior ();
+ /* Set up the "saved terminal modes" of the inferior
+ based on what modes we are starting it with. */
+ target_terminal::init ();
- inferior->needs_setup = 1;
+ /* Install inferior's terminal modes. This may look like a no-op,
+ as we've just saved them above, however, this does more than
+ restore terminal settings:
- if (target_is_non_stop_p ())
- {
- /* If we find that the current thread isn't stopped, explicitly
- do so now, because we're going to install breakpoints and
- poke at memory. */
+ - installs a SIGINT handler that forwards SIGINT to the inferior.
+ Otherwise a Ctrl-C pressed just while waiting for the initial
+ stop would end up as a spurious Quit.
- if (async_exec)
- /* The user requested an `attach&'; stop just one thread. */
- target_stop (inferior_ptid);
- else
- /* The user requested an `attach', so stop all threads of this
- inferior. */
- target_stop (ptid_t (inferior_ptid.pid ()));
- }
+ - removes stdin from the event loop, which we need if attaching
+ in the foreground, otherwise on targets that report an initial
+ stop on attach (which are most) we'd process input/commands
+ while we're in the event loop waiting for that stop. That is,
+ before the attach continuation runs and the command is really
+ finished. */
+ target_terminal::inferior ();
- /* Check for exec file mismatch, and let the user solve it. */
- validate_exec_file (from_tty);
+ /* Set up execution context to know that we should return from
+ wait_for_inferior as soon as the target reports a stop. */
+ init_wait_for_inferior ();
- mode = async_exec ? ATTACH_POST_WAIT_RESUME : ATTACH_POST_WAIT_STOP;
+ inferior->needs_setup = 1;
- /* Some system don't generate traps when attaching to inferior.
- E.g. Mach 3 or GNU hurd. */
- if (!target_attach_no_wait ())
- {
- struct attach_command_continuation_args *a;
+ if (target_is_non_stop_p ())
+ {
+ /* If we find that the current thread isn't stopped, explicitly
+ do so now, because we're going to install breakpoints and
+ poke at memory. */
+
+ if (async_exec)
+ /* The user requested an `attach&'; stop just one thread. */
+ target_stop (inferior_ptid);
+ else
+ /* The user requested an `attach', so stop all threads of this
+ inferior. */
+ target_stop (ptid_t (inferior_ptid.pid ()));
+ }
- /* Careful here. See comments in inferior.h. Basically some
- OSes don't ignore SIGSTOPs on continue requests anymore. We
- need a way for handle_inferior_event to reset the stop_signal
- variable after an attach, and this is what
- STOP_QUIETLY_NO_SIGSTOP is for. */
- inferior->control.stop_soon = STOP_QUIETLY_NO_SIGSTOP;
+ /* Check for exec file mismatch, and let the user solve it. */
+ validate_exec_file (from_tty);
- /* Wait for stop. */
- a = XNEW (struct attach_command_continuation_args);
- a->args = xstrdup (args);
- a->from_tty = from_tty;
- a->mode = mode;
- add_inferior_continuation (attach_command_continuation, a,
- attach_command_continuation_free_args);
+ mode = async_exec ? ATTACH_POST_WAIT_RESUME : ATTACH_POST_WAIT_STOP;
- /* Let infrun consider waiting for events out of this
- target. */
- inferior->process_target ()->threads_executing = true;
+ /* Some system don't generate traps when attaching to inferior.
+ E.g. Mach 3 or GNU hurd. */
+ if (!target_attach_no_wait ())
+ {
+ struct attach_command_continuation_args *a;
+
+ /* Careful here. See comments in inferior.h. Basically some
+ OSes don't ignore SIGSTOPs on continue requests anymore. We
+ need a way for handle_inferior_event to reset the stop_signal
+ variable after an attach, and this is what
+ STOP_QUIETLY_NO_SIGSTOP is for. */
+ inferior->control.stop_soon = STOP_QUIETLY_NO_SIGSTOP;
+
+ /* Wait for stop. */
+ a = XNEW (struct attach_command_continuation_args);
+ a->args = xstrdup (args);
+ a->from_tty = from_tty;
+ a->mode = mode;
+ add_inferior_continuation (attach_command_continuation, a,
+ attach_command_continuation_free_args);
+
+ /* Let infrun consider waiting for events out of this
+ target. */
+ inferior->process_target ()->threads_executing = true;
+
+ if (!target_is_async_p ())
+ mark_infrun_async_event_handler ();
+ return;
+ }
+ else
+ attach_post_wait (args, from_tty, mode);
+ }
- if (!target_is_async_p ())
- mark_infrun_async_event_handler ();
- return;
- }
- else
- attach_post_wait (args, from_tty, mode);
+ maybe_call_commit_resumed_all_process_targets ();
}
/* We had just found out that the target was already attached to an
@@ -2746,39 +2758,49 @@ detach_command (const char *args, int from_tty)
if (inferior_ptid == null_ptid)
error (_("The program is not being run."));
- query_if_trace_running (from_tty);
+ {
+ scoped_disable_commit_resumed disable_commit_resumed ("detaching");
- disconnect_tracing ();
+ query_if_trace_running (from_tty);
- /* Hold a strong reference to the target while (maybe)
- detaching the parent. Otherwise detaching could close the
- target. */
- auto target_ref
- = target_ops_ref::new_reference (current_inferior ()->process_target ());
+ disconnect_tracing ();
- /* Save this before detaching, since detaching may unpush the
- process_stratum target. */
- bool was_non_stop_p = target_is_non_stop_p ();
+ /* Hold a strong reference to the target while (maybe)
+ detaching the parent. Otherwise detaching could close the
+ target. */
+ auto target_ref
+ = target_ops_ref::new_reference (current_inferior ()->process_target ());
- target_detach (current_inferior (), from_tty);
+ /* Save this before detaching, since detaching may unpush the
+ process_stratum target. */
+ bool was_non_stop_p = target_is_non_stop_p ();
- /* The current inferior process was just detached successfully. Get
- rid of breakpoints that no longer make sense. Note we don't do
- this within target_detach because that is also used when
- following child forks, and in that case we will want to transfer
- breakpoints to the child, not delete them. */
- breakpoint_init_inferior (inf_exited);
+ target_detach (current_inferior (), from_tty);
- /* If the solist is global across inferiors, don't clear it when we
- detach from a single inferior. */
- if (!gdbarch_has_global_solist (target_gdbarch ()))
- no_shared_libraries (NULL, from_tty);
+ /* The current inferior process was just detached successfully. Get
+ rid of breakpoints that no longer make sense. Note we don't do
+ this within target_detach because that is also used when
+ following child forks, and in that case we will want to transfer
+ breakpoints to the child, not delete them. */
+ breakpoint_init_inferior (inf_exited);
- if (deprecated_detach_hook)
- deprecated_detach_hook ();
+ /* If the solist is global across inferiors, don't clear it when we
+ detach from a single inferior. */
+ if (!gdbarch_has_global_solist (target_gdbarch ()))
+ no_shared_libraries (NULL, from_tty);
+
+ if (deprecated_detach_hook)
+ deprecated_detach_hook ();
+
+ if (!was_non_stop_p)
+ {
+ process_stratum_target *proc_target
+ = as_process_stratum_target (target_ref.get ());
+ restart_after_all_stop_detach (proc_target);
+ }
+ }
- if (!was_non_stop_p)
- restart_after_all_stop_detach (as_process_stratum_target (target_ref.get ()));
+ maybe_call_commit_resumed_all_process_targets ();
}
/* Disconnect from the current target without resuming it (leaving it
@@ -2827,23 +2849,29 @@ stop_current_target_threads_ns (ptid_t ptid)
void
interrupt_target_1 (bool all_threads)
{
- if (non_stop)
- {
- if (all_threads)
- {
- scoped_restore_current_thread restore_thread;
+ {
+ scoped_disable_commit_resumed inhibit ("interrupting");
- for (inferior *inf : all_inferiors ())
- {
- switch_to_inferior_no_thread (inf);
- stop_current_target_threads_ns (minus_one_ptid);
- }
- }
- else
- stop_current_target_threads_ns (inferior_ptid);
- }
- else
- target_interrupt ();
+ if (non_stop)
+ {
+ if (all_threads)
+ {
+ scoped_restore_current_thread restore_thread;
+
+ for (inferior *inf : all_inferiors ())
+ {
+ switch_to_inferior_no_thread (inf);
+ stop_current_target_threads_ns (minus_one_ptid);
+ }
+ }
+ else
+ stop_current_target_threads_ns (inferior_ptid);
+ }
+ else
+ target_interrupt ();
+ }
+
+ maybe_call_commit_resumed_all_process_targets ();
}
/* interrupt [-a]
diff --git a/gdb/infrun.c b/gdb/infrun.c
index ad6f276038fc..adc99ecc97fc 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -2172,8 +2172,6 @@ do_target_resume (ptid_t resume_ptid, bool step, enum gdb_signal sig)
target_resume (resume_ptid, step, sig);
- maybe_commit_resume_process_target (tp->inf->process_target ());
-
if (target_can_async_p ())
target_async (1);
}
@@ -2760,17 +2758,120 @@ schedlock_applies (struct thread_info *tp)
execution_direction)));
}
-/* Calls maybe_commit_resume_process_target on all process targets. */
+/* See infrun.h. */
-static void
-maybe_commit_resume_all_process_targets ()
+void
+scoped_disable_commit_resumed::maybe_set_commit_resumed_all_process_targets ()
+{
+ for (process_stratum_target *target : all_non_exited_process_targets ())
+ {
+ gdb_assert (!target->commit_resumed_state);
+
+ /* If the target has no resumed threads, it would be useless to ask it
+ to commit the resumed threads. */
+ if (!target->threads_executing)
+ {
+ infrun_debug_printf ("not re-enabling forward progress for target "
+ "%s, no resumed threads",
+ target->shortname ());
+ continue;
+ }
+
+ /* If a thread from this target has some status to report, we better
+ handle it before requiring the target to commit its resumed threads:
+ handling the status might lead to resuming more threads. */
+ bool has_thread_with_pending_status = false;
+ for (thread_info *thread : all_non_exited_threads (target))
+ if (thread->resumed && thread->suspend.waitstatus_pending_p)
+ {
+ has_thread_with_pending_status = true;
+ break;
+ }
+
+ if (has_thread_with_pending_status)
+ {
+ infrun_debug_printf ("not requesting commit-resumed for target %s, a"
+ "thread has a pending waitstatus",
+ target->shortname ());
+ continue;
+ }
+
+ infrun_debug_printf ("enabling commit-resumed for target %s",
+ target->shortname());
+
+ target->commit_resumed_state = true;
+ }
+}
+
+void
+maybe_call_commit_resumed_all_process_targets ()
{
scoped_restore_current_thread restore_thread;
for (process_stratum_target *target : all_non_exited_process_targets ())
{
+ if (!target->commit_resumed_state)
+ continue;
+
+ infrun_debug_printf ("calling commit_resumed for target %s",
+ target->shortname());
+
switch_to_target_no_thread (target);
- maybe_commit_resume_process_target (target);
+ target->commit_resumed ();
+ }
+}
+
+/* To track nesting of scoped_disable_commit_resumed objects, ensuring that
+ only the outermost one attempts to re-enable commit-resumed. */
+
+static bool defer_enable_commit_resumed = false;
+
+scoped_disable_commit_resumed::scoped_disable_commit_resumed
+ (const char *reason)
+ : m_reason (reason),
+ m_prev_defer_value (defer_enable_commit_resumed)
+{
+ infrun_debug_printf ("reason=%s", m_reason);
+
+ defer_enable_commit_resumed = true;
+
+ for (process_stratum_target *target : all_process_targets ())
+ {
+ if (m_prev_defer_value == false)
+ {
+ /* This is the outermost instance: force all
+ COMMIT_RESUMED_STATE to false. */
+ target->commit_resumed_state = false;
+ }
+ else
+ {
+ /* This is not the outermost instance, we expect COMMIT_RESUMED_STATE
+ to have been cleared by the outermost instance. */
+ gdb_assert (!target->commit_resumed_state);
+ }
+ }
+}
+
+scoped_disable_commit_resumed::~scoped_disable_commit_resumed ()
+{
+ infrun_debug_printf ("reason=%s", m_reason);
+
+ gdb_assert (defer_enable_commit_resumed);
+
+ defer_enable_commit_resumed = m_prev_defer_value;
+
+ if (m_prev_defer_value == false)
+ {
+ /* This is the outermost instance, re-enable COMMIT_RESUMED_STATE on the
+ targets where it's possible. */
+ maybe_set_commit_resumed_all_process_targets ();
+ }
+ else
+ {
+ /* This is not the outermost instance, we expect COMMIT_RESUMED_STATE to
+ still be false. */
+ for (process_stratum_target *target : all_process_targets ())
+ gdb_assert (!target->commit_resumed_state);
}
}
@@ -2994,8 +3095,7 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal)
cur_thr->prev_pc = regcache_read_pc_protected (regcache);
{
- scoped_restore save_defer_tc
- = make_scoped_defer_process_target_commit_resume ();
+ scoped_disable_commit_resumed disable_commit_resumed ("proceeding");
started = start_step_over ();
@@ -3065,7 +3165,7 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal)
}
}
- maybe_commit_resume_all_process_targets ();
+ maybe_call_commit_resumed_all_process_targets ();
finish_state.release ();
@@ -3867,8 +3967,22 @@ fetch_inferior_event ()
= make_scoped_restore (&execution_direction,
target_execution_direction ());
+ /* Allow process stratum targets to pause their resumed threads while we
+ handle the event.
+
+ The optional is a bit ugly, but this is to allow calling
+ maybe_call_commit_resumed_all_process_targets after destroying
+ the object. */
+ gdb::optional<scoped_disable_commit_resumed> disable_commit_resumed;
+ disable_commit_resumed.emplace ("handling event");
+
if (!do_target_wait (minus_one_ptid, ecs, TARGET_WNOHANG))
- return;
+ {
+ infrun_debug_printf ("do_target_wait returned no event");
+ disable_commit_resumed.reset ();
+ maybe_call_commit_resumed_all_process_targets ();
+ return;
+ }
gdb_assert (ecs->ws.kind != TARGET_WAITKIND_IGNORE);
@@ -3959,6 +4073,9 @@ fetch_inferior_event ()
/* No error, don't finish the thread states yet. */
finish_state.release ();
+ disable_commit_resumed.reset ();
+ maybe_call_commit_resumed_all_process_targets ();
+
/* This scope is used to ensure that readline callbacks are
reinstalled here. */
}
diff --git a/gdb/infrun.h b/gdb/infrun.h
index e643c84dd80e..148ea79220c7 100644
--- a/gdb/infrun.h
+++ b/gdb/infrun.h
@@ -273,4 +273,63 @@ extern void all_uis_on_sync_execution_starting (void);
detach. */
extern void restart_after_all_stop_detach (process_stratum_target *proc_target);
+/* RAII object to temporarily disable the requirement for process stratum
+ targets to commit their resumed threads.
+
+ On construction, set process_stratum_target::commit_resumed_state to false
+ for all process stratum targets.
+
+ On destruction, set process_stratum_target::commit_resumed_state to true for
+ all process targets, except those that:
+
+ - have no resumed threads
+ - have a resumed thread with a pending status
+
+ The process_stratum_target::commit_resumed method is not called on the
+ targets, because its implementations could throw, and we don't want that in
+ the destructor. Instead, the caller should call the
+ maybe_call_commit_resumed_all_process_targets function after to achieve that.
+
+ The creation of nested scoped_disable_commit_resumed objects is tracked, such
+ that only the outermost instance actually does something, for cases like this:
+
+ void
+ inner_func ()
+ {
+ scoped_disable_commit_resumed disable;
+ // do stuff
+ }
+
+ void
+ outer_func ()
+ {
+ scoped_disable_commit_resumed disable;
+
+ for (... each thread ...)
+ inner_func ();
+ }
+
+ In this case, we don't want the `disable` in `inner_func` to require targets
+ to commit resumed threads in its destructor. */
+
+struct scoped_disable_commit_resumed
+{
+ explicit scoped_disable_commit_resumed (const char *reason);
+ ~scoped_disable_commit_resumed ();
+
+ DISABLE_COPY_AND_ASSIGN (scoped_disable_commit_resumed);
+
+private:
+ /* Helper for the destructor. */
+ void maybe_set_commit_resumed_all_process_targets ();
+
+ const char *m_reason;
+ bool m_prev_defer_value;
+};
+
+/* Call the commit_resumed method on all process targets whose
+ COMMIT_RESUME_STATE is set. */
+
+extern void maybe_call_commit_resumed_all_process_targets ();
+
#endif /* INFRUN_H */
diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index 9a14d78e1e27..b66a5525a00c 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -266,51 +266,57 @@ exec_continue (char **argv, int argc)
{
prepare_execution_command (current_top_target (), mi_async_p ());
- if (non_stop)
- {
- /* In non-stop mode, 'resume' always resumes a single thread.
- Therefore, to resume all threads of the current inferior, or
- all threads in all inferiors, we need to iterate over
- threads.
+ {
+ scoped_disable_commit_resumed disable_commit_resumed ("mi continue");
- See comment on infcmd.c:proceed_thread_callback for rationale. */
- if (current_context->all || current_context->thread_group != -1)
- {
- scoped_restore_current_thread restore_thread;
- int pid = 0;
+ if (non_stop)
+ {
+ /* In non-stop mode, 'resume' always resumes a single thread.
+ Therefore, to resume all threads of the current inferior, or
+ all threads in all inferiors, we need to iterate over
+ threads.
- if (!current_context->all)
- {
- struct inferior *inf
- = find_inferior_id (current_context->thread_group);
+ See comment on infcmd.c:proceed_thread_callback for rationale. */
+ if (current_context->all || current_context->thread_group != -1)
+ {
+ scoped_restore_current_thread restore_thread;
+ int pid = 0;
- pid = inf->pid;
- }
- iterate_over_threads (proceed_thread_callback, &pid);
- }
- else
- {
- continue_1 (0);
- }
- }
- else
- {
- scoped_restore save_multi = make_scoped_restore (&sched_multi);
+ if (!current_context->all)
+ {
+ struct inferior *inf
+ = find_inferior_id (current_context->thread_group);
- if (current_context->all)
- {
- sched_multi = 1;
- continue_1 (0);
- }
- else
- {
- /* In all-stop mode, -exec-continue traditionally resumed
- either all threads, or one thread, depending on the
- 'scheduler-locking' variable. Let's continue to do the
- same. */
- continue_1 (1);
- }
- }
+ pid = inf->pid;
+ }
+ iterate_over_threads (proceed_thread_callback, &pid);
+ }
+ else
+ {
+ continue_1 (0);
+ }
+ }
+ else
+ {
+ scoped_restore save_multi = make_scoped_restore (&sched_multi);
+
+ if (current_context->all)
+ {
+ sched_multi = 1;
+ continue_1 (0);
+ }
+ else
+ {
+ /* In all-stop mode, -exec-continue traditionally resumed
+ either all threads, or one thread, depending on the
+ 'scheduler-locking' variable. Let's continue to do the
+ same. */
+ continue_1 (1);
+ }
+ }
+ }
+
+ maybe_call_commit_resumed_all_process_targets ();
}
static void
diff --git a/gdb/process-stratum-target.c b/gdb/process-stratum-target.c
index ec127f524b74..9877f0d81931 100644
--- a/gdb/process-stratum-target.c
+++ b/gdb/process-stratum-target.c
@@ -99,6 +99,20 @@ all_non_exited_process_targets ()
/* See process-stratum-target.h. */
+std::set<process_stratum_target *>
+all_process_targets ()
+{
+ /* Inferiors may share targets. To eliminate duplicates, use a set. */
+ std::set<process_stratum_target *> targets;
+ for (inferior *inf : all_inferiors ())
+ if (inf->process_target () != nullptr)
+ targets.insert (inf->process_target ());
+
+ return targets;
+}
+
+/* See process-stratum-target.h. */
+
void
switch_to_target_no_thread (process_stratum_target *target)
{
@@ -108,26 +122,3 @@ switch_to_target_no_thread (process_stratum_target *target)
break;
}
}
-
-/* If true, `maybe_commit_resume_process_target` is a no-op. */
-
-static bool defer_process_target_commit_resume;
-
-/* See process-stratum-target.h. */
-
-void
-maybe_commit_resume_process_target (process_stratum_target *proc_target)
-{
- if (defer_process_target_commit_resume)
- return;
-
- proc_target->commit_resume ();
-}
-
-/* See process-stratum-target.h. */
-
-scoped_restore_tmpl<bool>
-make_scoped_defer_process_target_commit_resume ()
-{
- return make_scoped_restore (&defer_process_target_commit_resume, true);
-}
diff --git a/gdb/process-stratum-target.h b/gdb/process-stratum-target.h
index c8060c46be93..3cea911dee09 100644
--- a/gdb/process-stratum-target.h
+++ b/gdb/process-stratum-target.h
@@ -63,19 +63,10 @@ class process_stratum_target : public target_ops
bool has_registers () override;
bool has_execution (inferior *inf) override;
- /* Commit a series of resumption requests previously prepared with
- resume calls.
+ /* Ensure that all resumed threads are committed to the target.
- GDB always calls `commit_resume` on the process stratum target after
- calling `resume` on a target stack. A process stratum target may thus use
- this method in coordination with its `resume` method to batch resumption
- requests. In that case, the target doesn't actually resume in its
- `resume` implementation. Instead, it takes note of resumption intent in
- `resume`, and defers the actual resumption `commit_resume`.
-
- E.g., the remote target uses this to coalesce multiple resumption requests
- in a single vCont packet. */
- virtual void commit_resume () {}
+ See the description of COMMIT_RESUMED_STATE for more details. */
+ virtual void commit_resumed () {}
/* True if any thread is, or may be executing. We need to track
this separately because until we fully sync the thread list, we
@@ -86,6 +77,35 @@ class process_stratum_target : public target_ops
/* The connection number. Visible in "info connections". */
int connection_number = 0;
+
+ /* Whether resumed threads must be committed to the target.
+
+ When true, resumed threads must be committed to the execution target.
+
+ When false, the process stratum target may leave resumed threads stopped
+ when it's convenient or efficient to do so. When the core requires resumed
+ threads to be committed again, this is set back to true and calls the
+ `commit_resumed` method to allow the target to do so.
+
+ To simplify the implementation of process stratum targets, the following
+ methods are guaranteed to be called with COMMIT_RESUMED_STATE set to
+ false:
+
+ - resume
+ - stop
+ - wait
+
+ Knowing this, the process stratum target doesn't need to implement
+ different behaviors depending on the COMMIT_RESUMED_STATE, and can
+ simply assert that it is false.
+
+ Process stratum targets can take advantage of this to batch resumption
+ requests, for example. In that case, the target doesn't actually resume in
+ its `resume` implementation. Instead, it takes note of the resumption
+ intent in `resume` and defers the actual resumption to `commit_resumed`.
+ For example, the remote target uses this to coalesce multiple resumption
+ requests in a single vCont packet. */
+ bool commit_resumed_state = false;
};
/* Downcast TARGET to process_stratum_target. */
@@ -101,24 +121,13 @@ as_process_stratum_target (target_ops *target)
extern std::set<process_stratum_target *> all_non_exited_process_targets ();
+/* Return a collection of all existing process stratum targets. */
+
+extern std::set<process_stratum_target *> all_process_targets ();
+
/* Switch to the first inferior (and program space) of TARGET, and
switch to no thread selected. */
extern void switch_to_target_no_thread (process_stratum_target *target);
-/* Commit a series of resumption requests previously prepared with
- target_resume calls.
-
- This function is a no-op if commit resumes are deferred (see
- `make_scoped_defer_process_target_commit_resume`). */
-
-extern void maybe_commit_resume_process_target
- (process_stratum_target *target);
-
-/* Setup to defer `commit_resume` calls, and re-set to the previous status on
- destruction. */
-
-extern scoped_restore_tmpl<bool>
- make_scoped_defer_process_target_commit_resume ();
-
#endif /* !defined (PROCESS_STRATUM_TARGET_H) */
diff --git a/gdb/record-full.c b/gdb/record-full.c
index 464f4a85c558..8d2f2cd8d7dc 100644
--- a/gdb/record-full.c
+++ b/gdb/record-full.c
@@ -1264,7 +1264,9 @@ record_full_wait_1 (struct target_ops *ops,
"issuing one more step in the "
"target beneath\n");
ops->beneath ()->resume (ptid, step, GDB_SIGNAL_0);
- proc_target->commit_resume ();
+ proc_target->commit_resumed_state = true;
+ proc_target->commit_resumed ();
+ proc_target->commit_resumed_state = false;
continue;
}
}
diff --git a/gdb/remote.c b/gdb/remote.c
index 63dcc6fc9407..8663351081da 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -425,7 +425,7 @@ class remote_target : public process_stratum_target
void detach (inferior *, int) override;
void disconnect (const char *, int) override;
- void commit_resume () override;
+ void commit_resumed () override;
void resume (ptid_t, int, enum gdb_signal) override;
ptid_t wait (ptid_t, struct target_waitstatus *, target_wait_flags) override;
@@ -6384,6 +6384,8 @@ remote_target::resume (ptid_t ptid, int step, enum gdb_signal siggnal)
{
struct remote_state *rs = get_remote_state ();
+ gdb_assert (!this->commit_resumed_state);
+
/* When connected in non-stop mode, the core resumes threads
individually. Resuming remote threads directly in target_resume
would thus result in sending one packet per thread. Instead, to
@@ -6469,6 +6471,36 @@ get_remote_inferior (inferior *inf)
return static_cast<remote_inferior *> (inf->priv.get ());
}
+struct stop_reply : public notif_event
+{
+ ~stop_reply ();
+
+ /* The identifier of the thread about this event */
+ ptid_t ptid;
+
+ /* The remote state this event is associated with. When the remote
+ connection, represented by a remote_state object, is closed,
+ all the associated stop_reply events should be released. */
+ struct remote_state *rs;
+
+ struct target_waitstatus ws;
+
+ /* The architecture associated with the expedited registers. */
+ gdbarch *arch;
+
+ /* Expedited registers. This makes remote debugging a bit more
+ efficient for those targets that provide critical registers as
+ part of their normal status mechanism (as another roundtrip to
+ fetch them is avoided). */
+ std::vector<cached_reg_t> regcache;
+
+ enum target_stop_reason stop_reason;
+
+ CORE_ADDR watch_data_address;
+
+ int core;
+};
+
/* Class used to track the construction of a vCont packet in the
outgoing packet buffer. This is used to send multiple vCont
packets if we have more actions than would fit a single packet. */
@@ -6573,7 +6605,7 @@ vcont_builder::push_action (ptid_t ptid, bool step, gdb_signal siggnal)
/* to_commit_resume implementation. */
void
-remote_target::commit_resume ()
+remote_target::commit_resumed ()
{
int any_process_wildcard;
int may_global_wildcard_vcont;
@@ -6648,6 +6680,8 @@ remote_target::commit_resume ()
disable process and global wildcard resumes appropriately. */
check_pending_events_prevent_wildcard_vcont (&may_global_wildcard_vcont);
+ bool any_pending_vcont_resume = false;
+
for (thread_info *tp : all_non_exited_threads (this))
{
remote_thread_info *priv = get_remote_thread_info (tp);
@@ -6664,6 +6698,9 @@ remote_target::commit_resume ()
continue;
}
+ if (priv->get_resume_state () == resume_state::RESUMED_PENDING_VCONT)
+ any_pending_vcont_resume = true;
+
/* If a thread is the parent of an unfollowed fork, then we
can't do a global wildcard, as that would resume the fork
child. */
@@ -6671,6 +6708,11 @@ remote_target::commit_resume ()
may_global_wildcard_vcont = 0;
}
+ /* We didn't have any resumed thread pending a vCont resume, so nothing to
+ do. */
+ if (!any_pending_vcont_resume)
+ return;
+
/* Now let's build the vCont packet(s). Actions must be appended
from narrower to wider scopes (thread -> process -> global). If
we end up with too many actions for a single packet vcont_builder
@@ -6691,6 +6733,13 @@ remote_target::commit_resume ()
gdb_assert (!thread_is_in_step_over_chain (tp));
+ /* We should never be commit-resuming a thread that has a stop reply.
+ Otherwise, we would end up reporting a stop event for a thread while
+ it is running on the remote target. */
+ remote_state *rs = get_remote_state ();
+ for (const auto &stop_reply : rs->stop_reply_queue)
+ gdb_assert (stop_reply->ptid != tp->ptid);
+
const resumed_pending_vcont_info &info
= remote_thr->resumed_pending_vcont_info ();
@@ -6743,8 +6792,6 @@ remote_target::commit_resume ()
vcont_builder.flush ();
}
-\f
-
/* Non-stop version of target_stop. Uses `vCont;t' to stop a remote
thread, all threads of a remote process, or all threads of all
processes. */
@@ -6756,6 +6803,42 @@ remote_target::remote_stop_ns (ptid_t ptid)
char *p = rs->buf.data ();
char *endp = p + get_remote_packet_size ();
+ gdb_assert (!this->commit_resumed_state);
+
+ /* If any thread that needs to stop was resumed but pending a vCont resume,
+ generate a phony stop_reply. */
+ for (thread_info *tp : all_non_exited_threads (this, ptid))
+ {
+ remote_thread_info *remote_thr = get_remote_thread_info (tp);
+
+ if (remote_thr->get_resume_state ()
+ == resume_state::RESUMED_PENDING_VCONT)
+ {
+ remote_debug_printf ("Enqueueing phony stop reply for thread pending "
+ "vCont-resume (%d, %ld, %ld)", tp->ptid.pid(),
+ tp->ptid.lwp (), tp->ptid.tid ());
+
+ stop_reply *sr = new stop_reply ();
+ sr->ptid = tp->ptid;
+ sr->rs = rs;
+ sr->ws.kind = TARGET_WAITKIND_STOPPED;
+ sr->ws.value.sig = GDB_SIGNAL_0;
+ sr->arch = tp->inf->gdbarch;
+ sr->stop_reason = TARGET_STOPPED_BY_NO_REASON;
+ sr->watch_data_address = 0;
+ sr->core = 0;
+ this->push_stop_reply (sr);
+
+ /* Pretend that this thread was actually resumed on the remote target,
+ then stopped. If we leave it in the RESUMED_PENDING_VCONT state
+ and the commit_resumed method is called while the stop reply is
+ still in the queue, we'll end up reporting a stop event to the core
+ for that thread while it is running on the remote target... that
+ would be bad. */
+ remote_thr->set_resumed ();
+ }
+ }
+
/* FIXME: This supports_vCont_probed check is a workaround until
packet_support is per-connection. */
if (packet_support (PACKET_vCont) == PACKET_SUPPORT_UNKNOWN
@@ -6960,36 +7043,6 @@ remote_console_output (const char *msg)
gdb_stdtarg->flush ();
}
-struct stop_reply : public notif_event
-{
- ~stop_reply ();
-
- /* The identifier of the thread about this event */
- ptid_t ptid;
-
- /* The remote state this event is associated with. When the remote
- connection, represented by a remote_state object, is closed,
- all the associated stop_reply events should be released. */
- struct remote_state *rs;
-
- struct target_waitstatus ws;
-
- /* The architecture associated with the expedited registers. */
- gdbarch *arch;
-
- /* Expedited registers. This makes remote debugging a bit more
- efficient for those targets that provide critical registers as
- part of their normal status mechanism (as another roundtrip to
- fetch them is avoided). */
- std::vector<cached_reg_t> regcache;
-
- enum target_stop_reason stop_reason;
-
- CORE_ADDR watch_data_address;
-
- int core;
-};
-
/* Return the length of the stop reply queue. */
int
@@ -7936,6 +7989,8 @@ remote_target::wait_ns (ptid_t ptid, struct target_waitstatus *status,
int ret;
int is_notif = 0;
+ gdb_assert (!this->commit_resumed_state);
+
/* If in non-stop mode, get out of getpkt even if a
notification is received. */
diff --git a/gdb/target.c b/gdb/target.c
index 58fd75687d0a..db0c5f76e0a9 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -1987,6 +1987,9 @@ target_wait (ptid_t ptid, struct target_waitstatus *status,
target_wait_flags options)
{
target_ops *target = current_top_target ();
+ process_stratum_target *proc_target = current_inferior ()->process_target ();
+
+ gdb_assert (!proc_target->commit_resumed_state);
if (!target->can_async_p ())
gdb_assert ((options & TARGET_WNOHANG) == 0);
@@ -2040,6 +2043,7 @@ void
target_resume (ptid_t ptid, int step, enum gdb_signal signal)
{
process_stratum_target *curr_target = current_inferior ()->process_target ();
+ gdb_assert (!curr_target->commit_resumed_state);
target_dcache_invalidate ();
@@ -3113,6 +3117,10 @@ target_update_thread_list (void)
void
target_stop (ptid_t ptid)
{
+ process_stratum_target *proc_target = current_inferior ()->process_target ();
+
+ gdb_assert (!proc_target->commit_resumed_state);
+
if (!may_stop)
{
warning (_("May not interrupt or stop the target, ignoring attempt"));
--
2.30.0
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v4 0/3] Reduce back and forth with target when threads have pending statuses
2021-01-25 4:57 [PATCH v4 0/3] Reduce back and forth with target when threads have pending statuses Simon Marchi
` (2 preceding siblings ...)
2021-01-25 4:57 ` [PATCH v4 3/3] gdb: generalize commit_resume, avoid commit-resuming when threads have pending statuses Simon Marchi
@ 2021-02-03 1:35 ` Pedro Alves
3 siblings, 0 replies; 8+ messages in thread
From: Pedro Alves @ 2021-02-03 1:35 UTC (permalink / raw)
To: Simon Marchi, gdb-patches
On 25/01/21 04:57, Simon Marchi via Gdb-patches wrote:
> This is the v4 of patches 3 and 4 of this series:
>
> https://sourceware.org/pipermail/gdb-patches/2021-January/174786.html
>
> The "better handling of 'S' packets" part of the series was already
> merged.
>
> Patch 1 is new, it adds a test to cover a case that didn't seem already
> covered, that is running or attaching while the inferior is running.
>
> Note that this series was developped and tested on top of these other
> series/patches, so they can be considered prerequisites (applied in this
> order):
>
> Fix detach + displaced-step regression + N bugs more
> https://sourceware.org/pipermail/gdb-patches/2021-January/175040.html
Alright, to try to unblock you, I've merged this series now.
>
> Clear target async event handlers in wait method
> https://sourceware.org/pipermail/gdb-patches/2020-November/173633.html
>
> gdb: remove unneeded argument in check_multi_target_resumption
> https://sourceware.org/pipermail/gdb-patches/2021-January/175057.html
I'll reply to these as soon as I'm able.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v4 3/3] gdb: generalize commit_resume, avoid commit-resuming when threads have pending statuses
2021-01-25 4:57 ` [PATCH v4 3/3] gdb: generalize commit_resume, avoid commit-resuming when threads have pending statuses Simon Marchi
@ 2021-02-06 18:05 ` Pedro Alves
0 siblings, 0 replies; 8+ messages in thread
From: Pedro Alves @ 2021-02-06 18:05 UTC (permalink / raw)
To: Simon Marchi, gdb-patches
Hi Simon.
This is looking good, but unfortunately, there's still one design problem to address.
See below.
On 25/01/21 04:57, Simon Marchi via Gdb-patches wrote:
> @@ -3867,8 +3967,22 @@ fetch_inferior_event ()
> = make_scoped_restore (&execution_direction,
> target_execution_direction ());
>
> + /* Allow process stratum targets to pause their resumed threads while we
> + handle the event.
> +
> + The optional is a bit ugly, but this is to allow calling
> + maybe_call_commit_resumed_all_process_targets after destroying
> + the object. */
> + gdb::optional<scoped_disable_commit_resumed> disable_commit_resumed;
> + disable_commit_resumed.emplace ("handling event");
> +
> if (!do_target_wait (minus_one_ptid, ecs, TARGET_WNOHANG))
> - return;
> + {
> + infrun_debug_printf ("do_target_wait returned no event");
> + disable_commit_resumed.reset ();
> + maybe_call_commit_resumed_all_process_targets ();
> + return;
> + }
>
> gdb_assert (ecs->ws.kind != TARGET_WAITKIND_IGNORE);
>
> @@ -3959,6 +4073,9 @@ fetch_inferior_event ()
> /* No error, don't finish the thread states yet. */
> finish_state.release ();
>
> + disable_commit_resumed.reset ();
> + maybe_call_commit_resumed_all_process_targets ();
> +
> /* This scope is used to ensure that readline callbacks are
> reinstalled here. */
> }
Here (above) 's the problem.
The issue is that as a result of handling an event, we can end re-resuming
the target and synchronously waiting until that resumption is done, before
we get to this disable_commit_resumed.reset() call.
For example, try this:
(any program with a loop will do, this is just the one I was already looking at)
shell1:
$ cd build/gdb
$ ../gdbserver/gdbserver :9999 ./testsuite/outputs/gdb.threads/detach-step-over/detach-step-over
shell2:
$ export g="./gdb -data-directory=data-directory"
$ $g ./testsuite/outputs/gdb.threads/detach-step-over/detach-step-over -ex "set sysroot" -ex "maint set target-non-stop on" -ex "tar remote :9999" -ex "b detach-step-over.c:54" -ex "c" -ex "set scheduler-locking on"
....
Breakpoint 1 at 0x555555555227: file /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/gdb.threads/detach-step-over.c, line 54.
Continuing.
....
[Switching to Thread 2456237.2456238]
Thread 2 "detach-step-ove" hit Breakpoint 1, thread_func (arg=0x0) at /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/gdb.threads/detach-step-over.c:54
54 counter++; /* Set breakpoint here. */
(gdb) commands
Type commands for breakpoint(s) 1, one per line.
End with a line saying just "end".
>p (void*) malloc (1)
>end
(gdb) c
Continuing.
Thread 2 "detach-step-ove" hit Breakpoint 1, thread_func (arg=0x0) at /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/gdb.threads/detach-step-over.c:54
54 counter++; /* Set breakpoint here. */
^C^C^C^C^C^C help me I am stuck ^C^C
So above we have:
- remote target + "maint set target-non-stop on" to ensure we go via the commit-resume paths
- a breakpoint with a command that does an infcall
When the breakpoint triggers, GDB runs its command list, which does an infcall that never
actually runs. GDB never resumes the target and then hangs waiting for a stop.
In this scenario, the breakpoint commands are run here:
(top-gdb) bt
#0 bpstat_do_actions () at /home/pedro/gdb/binutils-gdb/src/gdb/breakpoint.c:4599
#1 0x0000557c5d698f7f in inferior_event_handler (event_type=INF_EXEC_COMPLETE) at /home/pedro/gdb/binutils-gdb/src/gdb/inf-loop.c:71
#2 0x0000557c5d6b76f6 in fetch_inferior_event () at /home/pedro/gdb/binutils-gdb/src/gdb/infrun.c:4051
There are are other paths that can lead to the same outcome.
For example, fetch_inferior_event -> normal_stop -> execute_cmd_pre_hook
runs the hook-stop, and a hook-stop may include a command that resumes the target.
Also note that proceed can also call normal_stop directly, which can again
lead to execute_cmd_pre_hook and to bpstat_do_actions.
In all the cases that we end up resuming the target but not returning
immediately, we "nest" an event loop, blocking in wait_sync_command_done.
When we get to wait_sync_command_done, defer_enable_commit_resumed is still
true, we never actually committed the resumes. So we wait forever for a
target stop, which will never come.
I think a fix could be for wait_sync_command_done to be a "synchronization"
point, and have it clear defer_enable_commit_resumed and force a commit-resume.
I suspect running the whole testsuite against extended-remote gdbserver
with "maint set target-non-stop on" would catch these cases.
> + /* If a thread from this target has some status to report, we better
> + handle it before requiring the target to commit its resumed threads:
> + handling the status might lead to resuming more threads. */
> + bool has_thread_with_pending_status = false;
> + for (thread_info *thread : all_non_exited_threads (target))
> + if (thread->resumed && thread->suspend.waitstatus_pending_p)
Typically the "we (had) better do X" idiom expresses urgency lest we face
possible consequences, like "we had better finish this patch before the
deadline!" But here, I don't think it's the case -- letting the target commit
its resumed threads may just not be optimal. I'd suggest rewording
the comment to avoid the idiom, since as is it reads to me like resuming
immediately would be incorrect and have really bad consequences somehow
(and then the somehow isn't explained).
> + if (m_prev_defer_value == false)
> + {
Any reason to write that, instead of:
if (!m_prev_defer_value)
?
Pedro Alves
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v4 1/3] gdb/testsuite: add test for run/attach while program is running
2021-01-25 4:57 ` [PATCH v4 1/3] gdb/testsuite: add test for run/attach while program is running Simon Marchi
@ 2021-02-06 18:22 ` Pedro Alves
2021-03-17 12:52 ` Pedro Alves
0 siblings, 1 reply; 8+ messages in thread
From: Pedro Alves @ 2021-02-06 18:22 UTC (permalink / raw)
To: Simon Marchi, gdb-patches
On 25/01/21 04:57, Simon Marchi via Gdb-patches wrote:
> A previous version of this patch series broke the use case of doing
> "run" or "attach" while the program is running, but it wasn't caught by
> the testsuite, which means it's not covered. Add a test for that.
>
> gdb/testsuite/ChangeLog:
>
> * gdb.base/run-attach-while-running.exp: New.
> * gdb.base/run-attach-while-running.c: New.
>
> Change-Id: I77f098ec0b28dc2d4575ea80e941f6a75273e431
> ---
> .../gdb.base/run-attach-while-running.c | 69 +++++++++
> .../gdb.base/run-attach-while-running.exp | 131 ++++++++++++++++++
> 2 files changed, 200 insertions(+)
> create mode 100644 gdb/testsuite/gdb.base/run-attach-while-running.c
> create mode 100644 gdb/testsuite/gdb.base/run-attach-while-running.exp
>
> diff --git a/gdb/testsuite/gdb.base/run-attach-while-running.c b/gdb/testsuite/gdb.base/run-attach-while-running.c
> new file mode 100644
> index 000000000000..57bebbe6456e
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/run-attach-while-running.c
> @@ -0,0 +1,69 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> + Copyright 2021 Free Software Foundation, Inc.
> +
> + This program is free software; you can redistribute it and/or modify
> + it under the terms of the GNU General Public License as published by
> + the Free Software Foundation; either version 3 of the License, or
> + (at your option) any later version.
> +
> + This program is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + GNU General Public License for more details.
> +
> + You should have received a copy of the GNU General Public License
> + along with this program. If not, see <http://www.gnu.org/licenses/>. */
> +
> +#include <unistd.h>
> +#include <assert.h>
> +
> +#ifndef WITH_THREADS
> +# error "WITH_THREADS must be defined."
> +#endif
> +
> +#if WITH_THREADS
> +# include <pthread.h>
> +
> +static pthread_barrier_t barrier;
> +
> +static void *
> +thread_func (void *p)
> +{
> + pthread_barrier_wait (&barrier);
> +
> + for (int i = 0; i < 30; i++)
> + sleep (1);
> +
> + return NULL;
> +}
> +
> +#endif /* WITH_THREADS */
> +
> +static void
> +all_started (void)
> +{}
> +
> +int
> +main (void)
> +{
> + alarm (30);
> +
> +#if WITH_THREADS
> + int ret = pthread_barrier_init (&barrier, NULL, 2);
> + assert (ret == 0);
> +
> + pthread_t thread;
> + ret = pthread_create (&thread, NULL, thread_func, NULL);
> + assert (ret == 0);
> +
> + pthread_barrier_wait (&barrier);
> +#endif /* WITH_THREADS */
> +
> + all_started ();
> +
> + for (int i = 0; i < 30; i++)
> + sleep (1);
> +
> + return 0;
> +}
> diff --git a/gdb/testsuite/gdb.base/run-attach-while-running.exp b/gdb/testsuite/gdb.base/run-attach-while-running.exp
> new file mode 100644
> index 000000000000..385bbc410dbf
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/run-attach-while-running.exp
> @@ -0,0 +1,131 @@
> +# Copyright 2021 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program. If not, see <http://www.gnu.org/licenses/>.
> +
> +# Test doing a "run" or an "attach" while the program is running.
> +#
> +# We test a non-threaded and a threaded configuration, so that targets that
> +# don't support threads get some testing, but we also test with threads when
> +# possible in case that triggers some multi-thread-specific bugs.
> +
> +standard_testfile
> +
> +set binfile_threads ${binfile}-threads
> +set binfile_nothreads ${binfile}-nothreads
> +unset binfile
> +
> +# Valid parameter / axis values:
> +#
> +# - non-stop: "off" of "on"
> +# - threaded: 0 or 1
> +# - run-or-attach: "run" or "attach"
> +
> +proc_with_prefix test { non-stop threaded run-or-attach } {
> + if { ${run-or-attach} == "attach" && ![can_spawn_for_attach] } {
> + unsupported "attach not supported"
> + return
> + }
> +
> + # Choose the right (threaded or not) binfile.
> + if { $threaded } {
> + set binfile $::binfile_threads
> + } else {
> + set binfile $::binfile_nothreads
> + }
> +
> + save_vars ::GDBFLAGS {
> + set ::GDBFLAGS "$::GDBFLAGS -ex \"set non-stop ${non-stop}\""
> +
> + # The test doesn't work when the remote target uses the synchronous
> + # remote protocol, because GDB can't kill the remote inferior while it
> + # is running, when we "run" or "attach" again. When aswering "yes" to
> + # the "Start it from the beginning?" question, we otherwise get:
> + #
> + # Cannot execute this command while the target is running. Use the
> + # "interrupt" command to stop the target and then try again.
> + #
> + # Interrupting the target would defeat the purpose of the test. So
> + # when non-stop is off and using the remote target, force the target
> + # to use the async / non-stop version of the protocol.
> + if { [target_info exists gdb_protocol] && ${non-stop} == "off" } {
> + set ::GDBFLAGS "$::GDBFLAGS -ex \"maint set target-non-stop on\""
> + }
> +
> + clean_restart $binfile
> + }
> +
> + if { ![runto_main] } {
> + untested "could not run to main"
> + return
> + }
> +
> + gdb_breakpoint "all_started" "temporary"
> + gdb_continue_to_breakpoint "continue to all_started"
> +
> + # If all-stop, everything stopped when we hit the all_started breakpoint,
> + # so resume execution in background. If running the non-threaded version,
> + # our only thread is stopped in any case, so resume as well. But if we are
> + # in non-stop with two threads, we have one running and one stopped, leave
> + # it like this, it makes an interesting test case.
> + if { ${non-stop} == "off" || !${threaded} } {
> + gdb_test "continue &" "Continuing."
> + }
> +
> + gdb_test_no_output "set confirm off"
> +
> + # Run again (or, connect to a new stub if using a stub), take advantage
> + # of the fact that runto_main leaves the breakpoint on main in place.
> + if { ${run-or-attach} == "run" } {
> + gdb_run_cmd
> + gdb_test "" "Breakpoint $::decimal, .*main.*" "hit main breakpoint after re-run"
> + } elseif { ${run-or-attach} == "attach" } {
> + set test_spawn_id [spawn_wait_for_attach $binfile]
> + set test_pid [spawn_id_get_pid $test_spawn_id]
> +
> + gdb_test "attach $test_pid" "Attaching to program: .*" "attach to process"
> +
> + gdb_exit
> + kill_wait_spawned_process $test_spawn_id
> + } else {
> + error "Invalid value for run-or-attach"
> + }
> +}
> +
> +# Build and test with the non-threaded version.
> +if { [build_executable "failed to prepare" ${binfile_nothreads} ${srcfile} \
> + {debug additional_flags=-DWITH_THREADS=0} ] } {
> + return
> +}
> +
> +with_test_prefix {threaded=0} {
> + foreach_with_prefix run-or-attach {run attach} {
> + foreach_with_prefix non-stop {off on} {
> + test ${non-stop} 0 ${run-or-attach}
> + }
> + }
> +}
> +
> +# Build and test with the threaded version.
> +if { [build_executable "failed to prepare" ${binfile_threads} ${srcfile} \
> + {debug pthreads additional_flags=-DWITH_THREADS=1} ] } {
> + return
> +}
> +
> +with_test_prefix {threaded=1} {
> + foreach_with_prefix run-or-attach {run attach} {
> + foreach_with_prefix non-stop {off on} {
> + test ${non-stop} 1 ${run-or-attach}
> + }
> + }
> +}
>
I was surprised to see the thread 0/1 cases separated instead of in a
foreach_with_prefix loop. I gave it a try, and it results in a net reduction
of lines of code. See below.
Otherwise, LGTM. Thanks!
From 3bbd9ea217b9ebad2978bd061b56f7cf2fe92667 Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>
Date: Thu, 4 Feb 2021 20:53:23 +0000
Subject: [PATCH] foreach_with_prefix
---
.../gdb.base/run-attach-while-running.exp | 40 ++++++-------------
1 file changed, 13 insertions(+), 27 deletions(-)
diff --git a/gdb/testsuite/gdb.base/run-attach-while-running.exp b/gdb/testsuite/gdb.base/run-attach-while-running.exp
index 385bbc410db..ee6bde78b6b 100644
--- a/gdb/testsuite/gdb.base/run-attach-while-running.exp
+++ b/gdb/testsuite/gdb.base/run-attach-while-running.exp
@@ -37,13 +37,6 @@ proc_with_prefix test { non-stop threaded run-or-attach } {
return
}
- # Choose the right (threaded or not) binfile.
- if { $threaded } {
- set binfile $::binfile_threads
- } else {
- set binfile $::binfile_nothreads
- }
-
save_vars ::GDBFLAGS {
set ::GDBFLAGS "$::GDBFLAGS -ex \"set non-stop ${non-stop}\""
@@ -62,7 +55,7 @@ proc_with_prefix test { non-stop threaded run-or-attach } {
set ::GDBFLAGS "$::GDBFLAGS -ex \"maint set target-non-stop on\""
}
- clean_restart $binfile
+ clean_restart $::binfile
}
if { ![runto_main] } {
@@ -90,7 +83,7 @@ proc_with_prefix test { non-stop threaded run-or-attach } {
gdb_run_cmd
gdb_test "" "Breakpoint $::decimal, .*main.*" "hit main breakpoint after re-run"
} elseif { ${run-or-attach} == "attach" } {
- set test_spawn_id [spawn_wait_for_attach $binfile]
+ set test_spawn_id [spawn_wait_for_attach $::binfile]
set test_pid [spawn_id_get_pid $test_spawn_id]
gdb_test "attach $test_pid" "Attaching to program: .*" "attach to process"
@@ -103,29 +96,22 @@ proc_with_prefix test { non-stop threaded run-or-attach } {
}
# Build and test with the non-threaded version.
-if { [build_executable "failed to prepare" ${binfile_nothreads} ${srcfile} \
- {debug additional_flags=-DWITH_THREADS=0} ] } {
- return
-}
-with_test_prefix {threaded=0} {
- foreach_with_prefix run-or-attach {run attach} {
- foreach_with_prefix non-stop {off on} {
- test ${non-stop} 0 ${run-or-attach}
- }
+foreach_with_prefix threaded {0 1} {
+ set options [list debug additional_flags=-DWITH_THREADS=$threaded]
+ if { $threaded } {
+ set binfile $binfile_threads
+ lappend options pthreads
+ } else {
+ set binfile $binfile_nothreads
+ }
+ if { [build_executable "failed to prepare" ${binfile} ${srcfile} $options] } {
+ continue
}
-}
-
-# Build and test with the threaded version.
-if { [build_executable "failed to prepare" ${binfile_threads} ${srcfile} \
- {debug pthreads additional_flags=-DWITH_THREADS=1} ] } {
- return
-}
-with_test_prefix {threaded=1} {
foreach_with_prefix run-or-attach {run attach} {
foreach_with_prefix non-stop {off on} {
- test ${non-stop} 1 ${run-or-attach}
+ test ${non-stop} ${threaded} ${run-or-attach}
}
}
}
--
2.26.2
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v4 1/3] gdb/testsuite: add test for run/attach while program is running
2021-02-06 18:22 ` Pedro Alves
@ 2021-03-17 12:52 ` Pedro Alves
0 siblings, 0 replies; 8+ messages in thread
From: Pedro Alves @ 2021-03-17 12:52 UTC (permalink / raw)
To: Simon Marchi, gdb-patches
Hi!
On 06/02/21 18:22, Pedro Alves wrote:
> I was surprised to see the thread 0/1 cases separated instead of in a
> foreach_with_prefix loop. I gave it a try, and it results in a net reduction
> of lines of code. See below.
>
> Otherwise, LGTM. Thanks!
FYI, I squashed in those changes and merged it to master.
Pedro Alves
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-03-17 12:53 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-25 4:57 [PATCH v4 0/3] Reduce back and forth with target when threads have pending statuses Simon Marchi
2021-01-25 4:57 ` [PATCH v4 1/3] gdb/testsuite: add test for run/attach while program is running Simon Marchi
2021-02-06 18:22 ` Pedro Alves
2021-03-17 12:52 ` Pedro Alves
2021-01-25 4:57 ` [PATCH v4 2/3] gdb: move commit_resume to process_stratum_target Simon Marchi
2021-01-25 4:57 ` [PATCH v4 3/3] gdb: generalize commit_resume, avoid commit-resuming when threads have pending statuses Simon Marchi
2021-02-06 18:05 ` Pedro Alves
2021-02-03 1:35 ` [PATCH v4 0/3] Reduce back and forth with target " Pedro Alves
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).