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-02 01:38 PM, Simon Marchi wrote:
> On 16-08-02 10:49 AM, Pedro Alves wrote:
>> On 08/01/2016 10:14 PM, Simon Marchi wrote:
>>> I can get around #1 by having an ugly global variable
>>> "meant_to_change_current_thread" that can be set to mean that the thread
>>> should not be restored to its original value, because the command
>>> actually wants to change the user selected thread. gdb_thread_select,
>>> for example, would do that. It really feels hackish though.
>>
>> Yeah. Ugly, though might be the simplest.
>>
>>>
>>> Perhaps #2 could be solved the same way, but I don't really know where
>>> we would set meant_to_change_current_thread. IOW, where does GDB take
>>> the conscious decision to change/leave the user selected thread to the
>>> thread that hit the breakpoint?
>>
>> It's done in normal_stop, here:
>>
>> ...
>> Also skip saying anything in non-stop mode. In that mode, as we
>> don't want GDB to switch threads behind the user's back, to avoid
>> races where the user is typing a command to apply to thread x,
>> but GDB switches to thread y before the user finishes entering
>> the command, fetch_inferior_event installs a cleanup to restore
>> the current thread back to the thread the user had selected right
>> after this event is handled, so we're not really switching, only
>> informing of a stop. */
>> if (!non_stop
>> && !ptid_equal (previous_inferior_ptid, inferior_ptid)
>> && target_has_execution
>> && last.kind != TARGET_WAITKIND_SIGNALLED
>> && last.kind != TARGET_WAITKIND_EXITED
>> && last.kind != TARGET_WAITKIND_NO_RESUMED)
>> {
>> SWITCH_THRU_ALL_UIS (state)
>> {
>> target_terminal_ours_for_output ();
>> printf_filtered (_("[Switching to %s]\n"),
>> target_pid_to_str (inferior_ptid));
>> annotate_thread_changed ();
>> }
>> previous_inferior_ptid = inferior_ptid;
>> }
>>
>>
>> An alternative may be to decouple the "user-selected thread" from
>> "inferior_ptid" further. previous_inferior_ptid is already
>> something like that, but not explicitly.
>>
>> So we'd make previous_inferior_ptid be the "user-selected thread", and make
>> the places that want to explicitly change the user-selected thread change
>> that global. That'd be gdb_thread_select, etc. and likely "kill", "detach",
>> "attach", "run" and "target remote" too.
>>
>> The input/command entry points would then be responsible for making
>> inferior_ptid (the internal selected thread) point to the user-selected
>> thread. We'd no longer need to use make_cleanup_restore_current_thread
>> to restore back the internal gdb selected thread, as it's simply
>> no longer necessary. Reducing all the temporary thread switching
>> may be a good thing. E.g., it likely reduces register cache refetching.
>>
>> This would be similar to how remote.c uses a lazy scheme that reduces
>> Hg traffic, where gdb keeps a local cache of the remote's selected thread,
>> and delays updating it on the remote target end until memory, registers
>> etc. are accessed. That is, even if core gdb switches inferior_ptid around,
>> that doesn't trigger immediate Hg packets. If the user has thread 3
>> selected, and gdb happens to need to read memory/registers off of
>> thread 1, spread over several packets, then we only switch the remote
>> side to thread 1 once, and don't switch it back to thread 3, even if
>> inferior_ptid is switched back.
>>
>> So, assuming a simply implementation for clarity, here's what
>> would happen inside gdb's little brain.
>>
>> Assume the user-selected thread starts out as thread 1:
>>
>>> -thread-select --thread 3 4
>>
>> . MI starts processing input. The user-selected thread is thread 1,
>> so MI switches inferior_ptid to match. Whatever was inferior_ptid
>> before is irrelevant.
>>
>> . MI processes the "--thread 3" switch, which is handled by MI common
>> code, and this causes inferior_ptid to be set to thread 3
>> (but not the user-selected thread global).
>>
>> . The "-thread-select" implementation switches both
>> inferior_ptid and the current user-selected thread to
>> thread 4.
>>
>>> -interpreter-exec --thread 3 console "thread 5"
>>
>> Similar. The last point is replaced by:
>>
>> . The "thread 5" implementation switches the user-visible
>> thread to thread 5.
>>
>>> -interpreter-exec --thread 1 console "c"
>>
>> and then thread 3 hits a breakpoint.
>>
>> This is similar too. The last point is replaced by:
>>
>> . normal_stop switches the user-visible thread to thread 3.
>
> Ok, I kinda had the same design idea, but it was blurry. Now that you explain it
> (I didn't know what previous_inferior_ptid was), it's clear. I'll try to prototype it
> quickly to see if it's viable.
I started to prototype what you described, but it's definitely not a small job. It would
also be too big of a change to put in 7.12. I'd be really interested to work on that for
the next development cycle though.
In the mean time, here is a new version of the patch that implements what I had described
(with the global flag), which I think is less risky.
>From d2b49f3b291ce9fa603a65324b8760bab3831ad3 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.
* 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.h | 5 +++++
gdb/mi/mi-main.c | 48 ++++++++++++++++++++++++------------------------
gdb/thread.c | 1 +
gdb/top.c | 1 +
5 files changed, 35 insertions(+), 24 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.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/mi/mi-main.c b/gdb/mi/mi-main.c
index b1cbd8b..0933615 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -2163,18 +2163,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)
{
@@ -2201,6 +2192,7 @@ static void
mi_cmd_execute (struct mi_parse *parse)
{
struct cleanup *cleanup;
+ struct cleanup *thread_restore_cleanup = NULL;
cleanup = prepare_execute_command ();
@@ -2216,6 +2208,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);
@@ -2224,6 +2228,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
@@ -2246,6 +2252,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);
}
@@ -2262,20 +2270,8 @@ 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);
@@ -2302,6 +2298,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 ();
diff --git a/gdb/top.c b/gdb/top.c
index 36c300e..79b1ad6 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -503,6 +503,7 @@ prepare_execute_command (void)
mark = value_mark ();
cleanup = make_cleanup_value_free_to_mark (mark);
+ command_changes_user_selected_ptid = 0;
/* With multiple threads running while the one we're examining is
stopped, the dcache can get stale without us being able to detect
--
2.9.2