* [PATCH 2/3] gdb: boolify thread_info's 'stop_requested' field
2024-03-28 17:35 [PATCH 1/3] gdb: print 'stop_requested' in infrun_debug_show_threads Tankut Baris Aktemur
@ 2024-03-28 17:35 ` Tankut Baris Aktemur
2024-03-28 17:35 ` [PATCH 3/3] gdb: avoid redundant calls to target_stop during attach Tankut Baris Aktemur
1 sibling, 0 replies; 3+ messages in thread
From: Tankut Baris Aktemur @ 2024-03-28 17:35 UTC (permalink / raw)
To: gdb-patches
Boolify the field. The 'set_stop_requested' function was already
taking a bool parameter, whose value is assigned to the field.
---
gdb/gdbthread.h | 2 +-
gdb/infcmd.c | 2 +-
gdb/infrun.c | 8 ++++----
3 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
index 11c553a99ca..02ff50afcd9 100644
--- a/gdb/gdbthread.h
+++ b/gdb/gdbthread.h
@@ -530,7 +530,7 @@ class thread_info : public intrusive_list_node<thread_info>,
struct target_waitstatus pending_follow;
/* True if this thread has been explicitly requested to stop. */
- int stop_requested = 0;
+ bool stop_requested = false;
/* The initiating frame of a nexting operation, used for deciding
which exceptions to intercept. If it is null_frame_id no
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index ac41ebf11b4..68ecdb9feba 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -2879,7 +2879,7 @@ stop_current_target_threads_ns (ptid_t ptid)
all-stop mode, we will only get one stop event --- it's undefined
which thread will report the event. */
set_stop_requested (current_inferior ()->process_target (),
- ptid, 1);
+ ptid, true);
}
/* See inferior.h. */
diff --git a/gdb/infrun.c b/gdb/infrun.c
index bbb98f6dcdb..c0ebc95a061 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -1284,7 +1284,7 @@ follow_exec (ptid_t ptid, const char *exec_file_target)
/* The user may have had the main thread held stopped in the
previous image (e.g., schedlock on, or non-stop). Release
it now. */
- th->stop_requested = 0;
+ th->stop_requested = false;
update_breakpoints_after_exec ();
@@ -3079,7 +3079,7 @@ clear_proceed_status_thread (struct thread_info *tp)
tp->control.step_stack_frame_id = null_frame_id;
tp->control.step_over_calls = STEP_OVER_UNDEBUGGABLE;
tp->control.step_start_function = nullptr;
- tp->stop_requested = 0;
+ tp->stop_requested = false;
tp->control.stop_step = 0;
@@ -5455,7 +5455,7 @@ handle_one (const wait_one_event &event)
if (t == nullptr)
t = add_thread (event.target, event.ptid);
- t->stop_requested = 0;
+ t->stop_requested = false;
t->set_executing (false);
t->set_resumed (false);
t->control.may_range_step = 0;
@@ -5695,7 +5695,7 @@ stop_all_threads (const char *reason, inferior *inf)
infrun_debug_printf (" %s executing, need stop",
t->ptid.to_string ().c_str ());
target_stop (t->ptid);
- t->stop_requested = 1;
+ t->stop_requested = true;
}
else
{
--
2.34.1
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
^ permalink raw reply [flat|nested] 3+ messages in thread* [PATCH 3/3] gdb: avoid redundant calls to target_stop during attach
2024-03-28 17:35 [PATCH 1/3] gdb: print 'stop_requested' in infrun_debug_show_threads Tankut Baris Aktemur
2024-03-28 17:35 ` [PATCH 2/3] gdb: boolify thread_info's 'stop_requested' field Tankut Baris Aktemur
@ 2024-03-28 17:35 ` Tankut Baris Aktemur
1 sibling, 0 replies; 3+ messages in thread
From: Tankut Baris Aktemur @ 2024-03-28 17:35 UTC (permalink / raw)
To: gdb-patches
When attaching to a non-stop target (e.g. the native Linux target in
the default mode or the remote target after "maint set target-non-stop
on" or "set non-stop on"), GDB sends a stop request to the target at
(from infcmd.c:attach_command)
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 ()));
}
Suppose we are in all-stop-on-top-of-non-stop mode with multiple
threads. After sending the stop requests to the threads and
completing the attach command, but before giving the prompt to the
user, GDB waits for the target in 'wait_sync_command_done' and
receives a stop event. After handling the event, GDB attempts to stop
all threads at
(top-gdb) bt
#0 stop_all_threads (reason=0x555557023ef0 "presenting stop to user in all-stop", inf=0x0) at src/gdb/infrun.c:5594
#1 0x00005555561c0cb5 in stop_all_threads_if_all_stop_mode () at src/gdb/infrun.c:4326
#2 0x00005555561c19a9 in fetch_inferior_event () at src/gdb/infrun.c:4676
#3 0x00005555561993b3 in inferior_event_handler (event_type=INF_REG_EVENT) at src/gdb/inf-loop.c:42
#4 0x000055555621b0e3 in handle_target_event (error=0, client_data=0x0) at src/gdb/linux-nat.c:4316
#5 0x0000555556f37bf6 in handle_file_event (file_ptr=0x5555587344f0, ready_mask=1) at src/gdbsupport/event-loop.cc:573
#6 0x0000555556f381f0 in gdb_wait_for_event (block=0) at src/gdbsupport/event-loop.cc:694
#7 0x0000555556f36ea7 in gdb_do_one_event (mstimeout=-1) at src/gdbsupport/event-loop.cc:217
#8 0x000055555662ce24 in wait_sync_command_done () at src/gdb/top.c:427
#9 0x000055555662ced0 in maybe_wait_sync_command_done (was_sync=0) at src/gdb/top.c:444
#10 0x000055555662d591 in execute_command (p=0x7fffffffe2ba "2", from_tty=1) at src/gdb/top.c:577
#11 0x000055555626fe2d in catch_command_errors (command=0x55555662ceed <execute_command(char const*, int)>, arg=0x7fffffffe2ad "attach 2892262", from_tty=1, do_bp_actions=true) at src/gdb/main.c:513
#12 0x000055555627007c in execute_cmdargs (cmdarg_vec=0x7fffffffdad0, file_type=CMDARG_FILE, cmd_type=CMDARG_COMMAND, ret=0x7fffffffda1c) at src/gdb/main.c:612
#13 0x000055555627163b in captured_main_1 (context=0x7fffffffdd20) at src/gdb/main.c:1293
#14 0x0000555556271871 in captured_main (data=0x7fffffffdd20) at src/gdb/main.c:1314
#15 0x0000555556271920 in gdb_main (args=0x7fffffffdd20) at src/gdb/main.c:1343
#16 0x0000555555cf8d0b in main (argc=9, argv=0x7fffffffde58) at src/gdb/gdb.c:39
In stop_all_threads, the thread states are
[infrun] infrun_debug_show_threads: enter
[infrun] infrun_debug_show_threads: non-exited threads:
[infrun] infrun_debug_show_threads: thread 2892262.2892262.0, executing = 0, resumed = 0, stop_requested = 0, state = STOPPED
[infrun] infrun_debug_show_threads: thread 2892262.2892271.0, executing = 1, resumed = 0, stop_requested = 0, state = RUNNING
[infrun] infrun_debug_show_threads: thread 2892262.2892272.0, executing = 1, resumed = 0, stop_requested = 0, state = RUNNING
[infrun] infrun_debug_show_threads: thread 2892262.2892273.0, executing = 1, resumed = 0, stop_requested = 0, state = RUNNING
[infrun] infrun_debug_show_threads: thread 2892262.2892274.0, executing = 1, resumed = 0, stop_requested = 0, state = RUNNING
[infrun] infrun_debug_show_threads: exit
Note that the first thread appears as 'executing = 0, state =
STOPPED', because it is the thread for which the stop event was
received first. Also note that for all of the threads we have
'stop_requested = 0' even though we had sent stop requests already in
'attach_command'. This causes GDB to call 'target_stop' again for
each thread (except the first eventing one, for which 'executing = 0').
The goal of this patch is to avoid the unnecessary calls to
'target_stop', but first a note about this motivation:
The Linux native target keeps track of whether a stop request was
already sent to a thread, and if so, does not send a request again.
The remote target peeks the pending stop events and if there is
already an event queued for the thread, skips sending a vCont packet.
That is, calling 'target_stop' is harmless. From that perspective, it
may seem like this patch is implementing a very minor optimization.
However, the actual motivation of the patch is to improve the internal
state book-keeping in GDB and to make the execution flow easier to
follow for the developer.
To avoid the redundant calls to 'target_stop', the solution we take is
to set the 'stop_requested' field of threads in 'attach_command' after
stopping the target. We use the existing
'stop_current_target_threads_ns' function for this purpose.
When we use this approach, in the same 'stop_all_threads' above, this
time we get
[infrun] infrun_debug_show_threads: enter
[infrun] infrun_debug_show_threads: non-exited threads:
[infrun] infrun_debug_show_threads: thread 2892262.2892262.0, executing = 0, resumed = 0, stop_requested = 1, state = STOPPED
[infrun] infrun_debug_show_threads: thread 2892262.2892271.0, executing = 1, resumed = 0, stop_requested = 1, state = RUNNING
[infrun] infrun_debug_show_threads: thread 2892262.2892272.0, executing = 1, resumed = 0, stop_requested = 1, state = RUNNING
[infrun] infrun_debug_show_threads: thread 2892262.2892273.0, executing = 1, resumed = 0, stop_requested = 1, state = RUNNING
[infrun] infrun_debug_show_threads: thread 2892262.2892274.0, executing = 1, resumed = 0, stop_requested = 1, state = RUNNING
[infrun] infrun_debug_show_threads: exit
The 'stop_requested = 1' values help GDB avoid skip the 'target_stop'
calls. GDB goes on to fetch the pending stop events and finishes
'stop_all_threads' successfully.
Above, there is one glitch, though, at
[infrun] infrun_debug_show_threads: thread 2892262.2892262.0, executing = 0, resumed = 0, stop_requested = 1, state = STOPPED
The 'stop_requested' field remained set. This causes a failure in
'gdb.threads/attach-non-stop.exp' where a thread that is expected to
become running remains stopped, because of the following check in
'proceed_after_attach':
if (!thread->executing ()
&& !thread->stop_requested
&& thread->stop_signal () == GDB_SIGNAL_0)
{
switch_to_thread (thread);
clear_proceed_status (0);
proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT);
}
I think the solution is to clear the 'stop_requested' field after we
process the stop event. I first thought of doing this inside
'mark_non_executing_threads', but this does not work because the
'stop_requested' field is still used in the execution flow thereafter.
Then I settled at 'fetch_inferior_event' in the block where we are
sure that we should stop.
Continuing the investigation, let us now focus on the synchronous
attach command (i.e. "attach <PID>" without any "&") when the non-stop
mode is on. In 'attach_command', GDB binds a continuation to the
inferior:
/* Wait for stop. */
inferior->add_continuation ([=] ()
{
attach_post_wait (from_tty, mode);
});
The 'mode' parameter is ATTACH_POST_WAIT_STOP in the sync mode.
In 'attach_post_wait', we have
else if (mode == ATTACH_POST_WAIT_STOP)
{
/* The user requested a plain `attach', so be sure to leave
the inferior stopped. */
/* At least the current thread is already stopped. */
/* In all-stop, by definition, all threads have to be already
stopped at this point. In non-stop, however, although the
selected thread is stopped, others may still be executing.
Be sure to explicitly stop all threads of the process. This
should have no effect on already stopped threads. */
if (non_stop)
target_stop (ptid_t (inferior->pid));
...
If 'non_stop' is true, then 'target_is_non_stop_p ()' must have been
true as well, and because 'async_exec' is false, we must have done
'target_stop (ptid_t (inferior_ptid.pid ()))' in 'attach_command'
already. This means, the call to 'target_stop' in the code above is
unnecessary and can be removed. We make this simplification in the
patch.
Regression-tested on X86-64 Linux using the unix, native-gdbserver,
and native-extended-gdbserver board files.
---
gdb/infcmd.c | 31 ++++++++++++++++---------------
gdb/infrun.c | 1 +
2 files changed, 17 insertions(+), 15 deletions(-)
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 68ecdb9feba..247db761a87 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -63,6 +63,8 @@ static void until_next_command (int);
static void step_1 (int, int, const char *);
+static void stop_current_target_threads_ns (ptid_t);
+
#define ERROR_NO_INFERIOR \
if (!target_has_execution ()) error (_("The program is not being run."));
@@ -2568,14 +2570,12 @@ attach_post_wait (int from_tty, enum attach_post_wait_mode mode)
/* At least the current thread is already stopped. */
- /* In all-stop, by definition, all threads have to be already
- stopped at this point. In non-stop, however, although the
- selected thread is stopped, others may still be executing.
- Be sure to explicitly stop all threads of the process. This
- should have no effect on already stopped threads. */
- if (non_stop)
- target_stop (ptid_t (inferior->pid));
- else if (target_is_non_stop_p ())
+ /* In all-stop targets, by definition, all threads have to be
+ already stopped at this point. In non-stop targets, however,
+ be sure to explicitly stop all threads of the process if we
+ are in all-stop mode. This should have no effect on already
+ stopped threads. */
+ if (!non_stop && target_is_non_stop_p ())
{
struct thread_info *lowest = inferior_thread ();
@@ -2686,13 +2686,14 @@ attach_command (const char *args, int from_tty)
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 ()));
+ /* If the user requested an `attach&', stop just one thread.
+ If the user requested an `attach', stop all threads of this
+ inferior. */
+ ptid_t stop_ptid = (async_exec
+ ? inferior_ptid
+ : ptid_t (inferior_ptid.pid ()));
+
+ stop_current_target_threads_ns (stop_ptid);
}
/* Check for exec file mismatch, and let the user solve it. */
diff --git a/gdb/infrun.c b/gdb/infrun.c
index c0ebc95a061..91b0bdea46f 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -4684,6 +4684,7 @@ fetch_inferior_event ()
bool should_notify_stop = true;
bool proceeded = false;
+ set_stop_requested (ecs.target, ecs.ptid, false);
stop_all_threads_if_all_stop_mode ();
clean_up_just_stopped_threads_fsms (&ecs);
--
2.34.1
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
^ permalink raw reply [flat|nested] 3+ messages in thread