This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 1/2] mi: Restore original thread/frame when specifying --thread or --thread-group
On 16-08-03 09:41 AM, Pedro Alves wrote:
> I think you need to consider infcalls, and canned sequence of commands
> Python/Scheme, etc., basically any place that sets ui->async = 0 ...
>
> guile/guile.c:169: current_ui->async = 0;
> guile/guile.c:202: current_ui->async = 0;
> guile/guile.c:332: current_ui->async = 0;
> guile/scm-ports.c:521: current_ui->async = 0;
> top.c:700: current_ui->async = 0;
> compile/compile.c:95: current_ui->async = 0;
> compile/compile.c:137: current_ui->async = 0;
> compile/compile.c:191: current_ui->async = 0;
> infcall.c:586: current_ui->async = 0;
> python/python.c:326: current_ui->async = 0;
> python/python.c:471: current_ui->async = 0;
> python/python.c:655: current_ui->async = 0;
> cli/cli-script.c:384: current_ui->async = 0;
> cli/cli-script.c:666: current_ui->async = 0;
> cli/cli-script.c:690: current_ui->async = 0;
> cli/cli-script.c:1697: current_ui->async = 0;
>
> ... because these will only return when execution next stops,
> so the cleanup runs after, and undoes the thread change.
Ok, I have not tested all of these yet, but this is how far I have got today.
> E.g., this infcall stops due to a breakpoint on another
> thread, but MI still reverts back to the "--thread" thread:
>
> (gdb)
> b usleep
> [...]
> ^done
> (gdb)
>
> thread 1
> [...]
> ^done
> =thread-selected,id="1"
> (gdb)
>
> -interpreter-exec --thread 1 console "print sleep (10)"
> [...]
> ~"[Switching to Thread 0x7ffff6ffb700 (LWP 7818)]\n"
> ~"\n"
> ~"Thread 3 \"threads\" hit Breakpoint 2, usleep (useconds=1) at ../sysdeps/posix/usleep.c:26\n"
> ~"26\t struct timespec ts = { .tv_sec = (long int) (useconds / 1000000),\n"
> *stopped,reason="breakpoint-hit",disp="keep",bkptno="2",frame={addr="0x00007ffff78f6030",func="usleep",args=[{name="useconds",value="1"}],file="../sysdeps/posix/usleep.c",fullname="/usr/src/debug/glibc-2.22/sysdeps/posix/usleep.c",line="26"},thread-id="3",stopped-threads="all",core="2"
> &"The program stopped in another thread while making a function call from GDB.\n"
> &"Evaluation of the expression containing the function\n"
> &"(__sleep) will be abandoned.\n"
> &"When the function is done executing, GDB will silently stop.\n"
> ^error,msg="The program stopped in another thread while making a function call from GDB.\nEvaluation of the expression containing the function\n(__sleep) will be abandoned.\nWhen the function is done executing, GDB will silently stop."
> (gdb)
>
> thread
> [...]
> ~"[Current thread is 1 (Thread 0x7ffff7fc8700 (LWP 7813))]\n"
> ^done
> (gdb)
>
> Above should have been 3.
I tried to set command_changes_user_selected_thread in normal_stop, but it didn't work
right away. That's because a breakpoint hit during an infcall calls error(), and the code
that discards the cleanup when command_changes_user_selected_thread is true is not executed.
I solved that by adding a try/catch in mi_cmd_execute that discards the cleanup and rethrows
the exception.
> E.g., this user-defined command starts the program and stops in
> thread 2.1, but MI still reverts back to the "--thread" thread:
>
> (gdb) define foo
> ~">" inferior 2
> ~">" start
> ~">" end
> (gdb)
>
> info inferiors
> ~" Num Description Executable \n"
> ~"* 1 process 8216 /home/pedro/brno/pedro/gdb/tests/threads \n"
> ~" 2 <null> /home/pedro/brno/pedro/gdb/tests/threads \n"
> ^done
> (gdb)
>
> info threads
> ~" Id Target Id Frame \n"
> ~"* 1.1 Thread 0x7ffff7fc8700 (LWP 8216) \"threads\" main () at threads.c:54\n"
> ^done
> (gdb)
>
> -interpreter-exec --thread 1 console "foo"
> ~"[Switching to inferior 2 [<null>] (/home/pedro/brno/pedro/gdb/tests/threads)]\n"
> [...]
> ~"\n"
> ~"Thread 2.1 \"threads\" hit Temporary breakpoint 3, main () at threads.c:54\n"
> ~"54\t long i = 0;\n"
> *stopped,reason="breakpoint-hit",disp="del",bkptno="3",frame={addr="0x000000000040076e",func="main",args=[],file="threads.c",fullname="/home/pedro/brno/pedro/gdb/tests/threads.c",line="54"},thread-id="3",stopped-threads="all",core="7"
> =breakpoint-deleted,id="3"
> (gdb)
>
> thread
> &"thread\n"
> ~"[Current thread is 1.1 (Thread 0x7ffff7fc8700 (LWP 8216))]\n"
> ^done
> (gdb)
I was surprised this wasn't automatically solved with the fix in normal_stop. That's
because "start" sets previous_inferior_ptid to the initial thread when it is started
(in init_wait_for_inferior). Actually, I think that starting a new inferior is a case
where we want to change the user selected thread. Setting command_changes_user_selected_thread
in init_wait_for_inferior fixes this.
It also fixes if you do just "-exec-run --thread-group i1" with mi-async=on and non-stop=on.
With master gdb, the first thread will be selected. With my previous patch, no thread would
be selected.
> While at it, "inferior 2" is a command that explicitly changes
> the thread, but misses setting the new flag:
>
> -interpreter-exec --thread 1 console "inferior 2"
> ~"[Switching to inferior 2 [process 8603] (/home/pedro/brno/pedro/gdb/tests/threads)]\n"
> ~"[Switching to thread 2.1 (Thread 0x7ffff7fc8700 (LWP 8603))] \n"
> ~"#0 main () at threads.c:54\n"
> ~"54\t long i = 0;\n"
> ^done
> (gdb)
>
> thread
> &"thread \n"
> ~"[Current thread is 1.1 (Thread 0x7ffff7fc8700 (LWP 8599))]\n"
> ^done
> (gdb)
Right, that's fixed by setting the command_changes_user_selected_thread in inferior_command.
> It may be this requires setting the flag in most of the places
> that we'd set the user-selected thread in the other approach,
> not sure.
It looks like it.
Here's an updated patch. Notes/todo:
- I did not test with compile yet.
- I thought about that just before sending: I think there's also something to do about
"frame X", "up" and "down", e.g.:
-interpreter-exec --thread 1 console "up"
won't work.
Thanks for the comments!
---
>From baff2cfffe4ff36871320e41ddaf18b95c0eaa11 Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@ericsson.com>
Date: Mon, 1 Aug 2016 17:12:51 -0400
Subject: [PATCH] mi: Restore original thread/frame when specifying --thread or
--thread-group
We are in the process of integrating Pedro's great separate ui work [0]
in Eclipse CDT. With this, the hope is that GDB users will feel right
at home with a console that behaves just like plain GDB in a terminal,
and appreciate the graphical support of CDT.
Earlier [1], we found that if the current thread is X, passing
"--thread Y" to an MI command would leave Y as the current thread.
This is not a problem for CDT itself, since it always uses --thread for
MI commands. However, it also affects the user selected thread. So the
internal front-end logic can unexpectedly change the currently selected
thread from the point of view of the user using the console.
So, to avoid changing the current selection under the user's feet while
they are typing a command, --thread and --thread-group should leave the
inferior/thread/frame selection in their original state.
This patch adds a restore_current_thread cleanup whenever --thread or
--thread-group is used (using --frame implies using --thread). To
support the case where the --thread flag would be used along with a
command that is meant to change the thread, it also adds a global
variable "command_changes_user_selected_ptid". The GDB core can set
this variable to indicate that it wants to change the user-selected
thread. When this variable is set, the cleanup is discarded so that the
newly selected thread is kept. Because there is a single chain of
cleanups, I had to reorder things in mi_cmd_execute so that it was
possible to only discard this cleanup.
As a side effect, the code that checks whether a =thread-selected event
should be emitted was simplified.
This is intended to be a temporary solution, until we implement
something like what Pedro suggested in [2].
Regtested with --target_board=unix on Linux x86.
[0] https://sourceware.org/ml/gdb-patches/2016-05/msg00098.html
[1] https://www.sourceware.org/ml/gdb/2015-10/msg00073.html
[2] https://sourceware.org/ml/gdb-patches/2016-08/msg00031.html
gdb/ChangeLog:
* infcmd.c (command_changes_user_selected_ptid): New variable.
* inferior.h (command_changes_user_selected_ptid): New
declaration.
* infrun.c (init_wait_for_inferior): Set
command_changes_user_selected_ptid.
(normal_stop): Likewise.
* mi/mi-main.c (mi_execute_command): Simplify computation of
report_change.
(mi_cmd_execute): Create restore current thread cleanup if
one of --thread or --thread-group is used. Move language and
suppress_notification code blocks up. Discard current thread
cleanup if command_changes_user_selected_ptid is set.
* thread.c (do_captured_thread_select): Set
command_changes_user_selected_ptid.
* top.c (prepare_execute_command): Unset
command_changes_user_selected_ptid.
---
gdb/infcmd.c | 4 ++++
gdb/inferior.c | 2 ++
gdb/inferior.h | 5 +++++
gdb/infrun.c | 2 ++
gdb/mi/mi-main.c | 63 ++++++++++++++++++++++++++++++++++----------------------
gdb/thread.c | 1 +
6 files changed, 52 insertions(+), 25 deletions(-)
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 58ba1cb..65adc63 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -124,6 +124,10 @@ static char *inferior_io_terminal_scratch;
ptid_t inferior_ptid;
+/* See inferior.h. */
+
+int command_changes_user_selected_ptid;
+
/* Address at which inferior stopped. */
CORE_ADDR stop_pc;
diff --git a/gdb/inferior.c b/gdb/inferior.c
index 45b3141..cb7db1b 100644
--- a/gdb/inferior.c
+++ b/gdb/inferior.c
@@ -722,6 +722,8 @@ inferior_command (char *args, int from_tty)
struct inferior *inf;
int num;
+ command_changes_user_selected_ptid = 1;
+
num = parse_and_eval_long (args);
inf = find_inferior_id (num);
diff --git a/gdb/inferior.h b/gdb/inferior.h
index 571d26a..edf829e 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -82,6 +82,11 @@ extern const char *get_inferior_io_terminal (void);
extern ptid_t inferior_ptid;
+/* Flag indicating that we explicitly want to change the user selected
+ thread/ptid. */
+
+extern int command_changes_user_selected_ptid;
+
extern void generic_mourn_inferior (void);
extern CORE_ADDR unsigned_pointer_to_address (struct gdbarch *gdbarch,
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 90841f4..894ccf9 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -3247,6 +3247,7 @@ init_wait_for_inferior (void)
target_last_wait_ptid = minus_one_ptid;
previous_inferior_ptid = inferior_ptid;
+ command_changes_user_selected_ptid = 1;
/* Discard any skipped inlined frames. */
clear_inline_frame_state (minus_one_ptid);
@@ -8313,6 +8314,7 @@ normal_stop (void)
annotate_thread_changed ();
}
previous_inferior_ptid = inferior_ptid;
+ command_changes_user_selected_ptid = 1;
}
if (last.kind == TARGET_WAITKIND_NO_RESUMED)
diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index 1913157..2001078 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -2170,18 +2170,9 @@ mi_execute_command (const char *cmd, int from_tty)
= (struct mi_interp *) top_level_interpreter_data ();
int report_change = 0;
- if (command->thread == -1)
- {
- report_change = (!ptid_equal (previous_ptid, null_ptid)
- && !ptid_equal (inferior_ptid, previous_ptid)
- && !ptid_equal (inferior_ptid, null_ptid));
- }
- else if (!ptid_equal (inferior_ptid, null_ptid))
- {
- struct thread_info *ti = inferior_thread ();
-
- report_change = (ti->global_num != command->thread);
- }
+ report_change = (!ptid_equal (previous_ptid, null_ptid)
+ && !ptid_equal (inferior_ptid, previous_ptid)
+ && !ptid_equal (inferior_ptid, null_ptid));
if (report_change)
{
@@ -2208,8 +2199,10 @@ static void
mi_cmd_execute (struct mi_parse *parse)
{
struct cleanup *cleanup;
+ struct cleanup *thread_restore_cleanup = NULL;
cleanup = prepare_execute_command ();
+ command_changes_user_selected_ptid = 0;
if (parse->all && parse->thread_group != -1)
error (_("Cannot specify --thread-group together with --all"));
@@ -2223,6 +2216,18 @@ mi_cmd_execute (struct mi_parse *parse)
if (parse->frame != -1 && parse->thread == -1)
error (_("Cannot specify --frame without --thread"));
+ if (parse->language != language_unknown)
+ {
+ make_cleanup_restore_current_language ();
+ set_language (parse->language);
+ }
+
+ if (parse->cmd->suppress_notification != NULL)
+ {
+ make_cleanup_restore_integer (parse->cmd->suppress_notification);
+ *parse->cmd->suppress_notification = 1;
+ }
+
if (parse->thread_group != -1)
{
struct inferior *inf = find_inferior_id (parse->thread_group);
@@ -2231,6 +2236,8 @@ mi_cmd_execute (struct mi_parse *parse)
if (!inf)
error (_("Invalid thread group for the --thread-group option"));
+ thread_restore_cleanup = make_cleanup_restore_current_thread ();
+
set_current_inferior (inf);
/* This behaviour means that if --thread-group option identifies
an inferior with multiple threads, then a random one will be
@@ -2253,6 +2260,8 @@ mi_cmd_execute (struct mi_parse *parse)
if (is_exited (tp->ptid))
error (_("Thread id: %d has terminated"), parse->thread);
+ thread_restore_cleanup = make_cleanup_restore_current_thread ();
+
switch_to_thread (tp->ptid);
}
@@ -2269,23 +2278,23 @@ mi_cmd_execute (struct mi_parse *parse)
error (_("Invalid frame id: %d"), frame);
}
- if (parse->language != language_unknown)
- {
- make_cleanup_restore_current_language ();
- set_language (parse->language);
- }
-
current_context = parse;
- if (parse->cmd->suppress_notification != NULL)
- {
- make_cleanup_restore_integer (parse->cmd->suppress_notification);
- *parse->cmd->suppress_notification = 1;
- }
-
if (parse->cmd->argv_func != NULL)
{
- parse->cmd->argv_func (parse->command, parse->argv, parse->argc);
+ TRY
+ {
+ parse->cmd->argv_func (parse->command, parse->argv, parse->argc);
+ }
+ CATCH (exception, RETURN_MASK_ALL)
+ {
+ if (command_changes_user_selected_ptid && thread_restore_cleanup != NULL)
+ discard_cleanups (thread_restore_cleanup);
+
+ throw_exception (exception);
+ }
+ END_CATCH
+
}
else if (parse->cmd->cli.cmd != 0)
{
@@ -2309,6 +2318,10 @@ mi_cmd_execute (struct mi_parse *parse)
make_cleanup_ui_file_delete (stb);
error_stream (stb);
}
+
+ if (command_changes_user_selected_ptid && thread_restore_cleanup != NULL)
+ discard_cleanups (thread_restore_cleanup);
+
do_cleanups (cleanup);
}
diff --git a/gdb/thread.c b/gdb/thread.c
index ab98777..7aa1ce7 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -2055,6 +2055,7 @@ do_captured_thread_select (struct ui_out *uiout, void *tidstr_v)
error (_("Thread ID %s has terminated."), tidstr);
switch_to_thread (tp->ptid);
+ command_changes_user_selected_ptid = 1;
annotate_thread_changed ();
--
2.9.2