From: Andrew Burgess <aburgess@redhat.com>
To: Jan Vrany via Gdb-patches <gdb-patches@sourceware.org>,
gdb-patches@sourceware.org
Cc: Jan Vrany <jan.vrany@labware.com>
Subject: Re: [PATCH v4] gdb/mi: PR20684, preserve user selected thread and frame when invoking MI commands
Date: Tue, 08 Mar 2022 16:58:11 +0000 [thread overview]
Message-ID: <871qzcwfdo.fsf@redhat.com> (raw)
In-Reply-To: <20220302132330.1083078-1-jan.vrany@labware.com>
Jan Vrany via Gdb-patches <gdb-patches@sourceware.org> writes:
> When invoking MI commands with --thread and/or --frame, user selected
> thread and frame was not preserved:
Jan,
Thanks for working on this. I've now pushed this patch with a couple of
minor adjustments in the testsuite (just adding extra comments).
Thanks,
Andrew
>
> (gdb)
> info thread
> &"info thread\n"
> ~" Id Target Id Frame \n"
> ~"* 1 Thread 0x7ffff7c30740 (LWP 19302) \"user-selected-c\" main () at /home/uuu/gdb/gdb/testsuite/gdb.mi/user-selected-context-sync.c:60\n"
> ~" 2 Thread 0x7ffff7c2f700 (LWP 19306) \"user-selected-c\" child_sub_function () at /home/uuu/gdb/gdb/testsuite/gdb.mi/user-selected-context-sync.c:30\n"
> ~" 3 Thread 0x7ffff742e700 (LWP 19307) \"user-selected-c\" child_sub_function () at /home/uuu/gdb/gdb/testsuite/gdb.mi/user-selected-context-sync.c:30\n"
> ^done
> (gdb)
> info frame
> &"info frame\n"
> ~"Stack level 0, frame at 0x7fffffffdf90:\n"
> ~" rip = 0x555555555207 in main (/home/uuu/gdb/gdb/testsuite/gdb.mi/user-selected-context-sync.c:60); saved rip = 0x7ffff7c5709b\n"
> ~" source language c.\n"
> ~" Arglist at 0x7fffffffdf80, args: \n"
> ~" Locals at 0x7fffffffdf80, Previous frame's sp is 0x7fffffffdf90\n"
> ~" Saved registers:\n "
> ~" rbp at 0x7fffffffdf80, rip at 0x7fffffffdf88\n"
> ^done
> (gdb)
> -stack-info-depth --thread 3
> ^done,depth="4"
> (gdb)
> info thread
> &"info thread\n"
> ~" Id Target Id Frame \n"
> ~" 1 Thread 0x7ffff7c30740 (LWP 19302) \"user-selected-c\" main () at /home/uuu/gdb/gdb/testsuite/gdb.mi/user-selected-context-sync.c:60\n"
> ~" 2 Thread 0x7ffff7c2f700 (LWP 19306) \"user-selected-c\" child_sub_function () at /home/uuu/gdb/gdb/testsuite/gdb.mi/user-selected-context-sync.c:30\n"
> ~"* 3 Thread 0x7ffff742e700 (LWP 19307) \"user-selected-c\" child_sub_function () at /home/uuu/gdb/gdb/testsuite/gdb.mi/user-selected-context-sync.c:30\n"
> ^done
> (gdb)
> info frame
> &"info frame\n"
> ~"Stack level 0, frame at 0x7ffff742dee0:\n"
> ~" rip = 0x555555555169 in child_sub_function (/home/uuu/gdb/gdb/testsuite/gdb.mi/user-selected-context-sync.c:30); saved rip = 0x555555555188\n"
> ~" called by frame at 0x7ffff742df00\n"
> ~" source language c.\n"
> ~" Arglist at 0x7ffff742ded0, args: \n"
> ~" Locals at 0x7ffff742ded0, Previous frame's sp is 0x7ffff742dee0\n"
> ~" Saved registers:\n "
> ~" rbp at 0x7ffff742ded0, rip at 0x7ffff742ded8\n"
> ^done
> (gdb)
>
> This caused problems for frontends that provide access to CLI because UI
> may silently change the context for CLI commands (as demonstrated above).
>
> This commit fixes the problem by restoring thread and frame in
> mi_cmd_execute (). With this change, there are only two GDB/MI commands
> that can change user selected context: -thread-select and -stack-select-frame.
> This allows us to remove all and rather complicated logic of notifying
> about user selected context change from mi_execute_command (), leaving it
> to these two commands themselves to notify.
>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=20684
> ---
> gdb/mi/mi-cmd-stack.c | 11 ++
> gdb/mi/mi-cmds.h | 12 ++
> gdb/mi/mi-main.c | 74 ++-------
> gdb/testsuite/gdb.mi/mi-cmd-user-context.exp | 155 +++++++++++++++++++
> 4 files changed, 189 insertions(+), 63 deletions(-)
> create mode 100644 gdb/testsuite/gdb.mi/mi-cmd-user-context.exp
>
> diff --git a/gdb/mi/mi-cmd-stack.c b/gdb/mi/mi-cmd-stack.c
> index e63f1706149..1be8aa81c3d 100644
> --- a/gdb/mi/mi-cmd-stack.c
> +++ b/gdb/mi/mi-cmd-stack.c
> @@ -36,6 +36,8 @@
> #include "mi-parse.h"
> #include "gdbsupport/gdb_optional.h"
> #include "safe-ctype.h"
> +#include "inferior.h"
> +#include "observable.h"
>
> enum what_to_list { locals, arguments, all };
>
> @@ -756,7 +758,16 @@ mi_cmd_stack_select_frame (const char *command, char **argv, int argc)
> if (argc == 0 || argc > 1)
> error (_("-stack-select-frame: Usage: FRAME_SPEC"));
>
> + ptid_t previous_ptid = inferior_ptid;
> +
> select_frame_for_mi (parse_frame_specification (argv[0]));
> +
> + /* Notify if the thread has effectively changed. */
> + if (inferior_ptid != previous_ptid)
> + {
> + gdb::observers::user_selected_context_changed.notify
> + (USER_SELECTED_THREAD | USER_SELECTED_FRAME);
> + }
> }
>
> void
> diff --git a/gdb/mi/mi-cmds.h b/gdb/mi/mi-cmds.h
> index 2a93a9f5476..785652ee1c9 100644
> --- a/gdb/mi/mi-cmds.h
> +++ b/gdb/mi/mi-cmds.h
> @@ -23,6 +23,7 @@
> #define MI_MI_CMDS_H
>
> #include "gdbsupport/gdb_optional.h"
> +#include "mi/mi-main.h"
>
> enum print_values {
> PRINT_NO_VALUES,
> @@ -163,6 +164,17 @@ struct mi_command
> wrong. */
> void invoke (struct mi_parse *parse) const;
>
> + /* Return whether this command preserves user selected context (thread
> + and frame). */
> + bool preserve_user_selected_context () const
> + {
> + /* Here we exploit the fact that if MI command is supposed to change
> + user context, then it should not emit change notifications. Therefore if
> + command does not suppress user context change notifications, then it should
> + preserve the context. */
> + return m_suppress_notification != &mi_suppress_notification.user_selected_context;
> + }
> +
> protected:
>
> /* The core of command invocation, this needs to be overridden in each
> diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
> index 4860da7536a..23e2373dcec 100644
> --- a/gdb/mi/mi-main.c
> +++ b/gdb/mi/mi-main.c
> @@ -1879,34 +1879,6 @@ mi_print_exception (const char *token, const struct gdb_exception &exception)
> fputs_unfiltered ("\n", mi->raw_stdout);
> }
>
> -/* Determine whether the parsed command already notifies the
> - user_selected_context_changed observer. */
> -
> -static int
> -command_notifies_uscc_observer (struct mi_parse *command)
> -{
> - if (command->op == CLI_COMMAND)
> - {
> - /* CLI commands "thread" and "inferior" already send it. */
> - return (startswith (command->command, "thread ")
> - || startswith (command->command, "inferior "));
> - }
> - else /* MI_COMMAND */
> - {
> - if (strcmp (command->command, "interpreter-exec") == 0
> - && command->argc > 1)
> - {
> - /* "thread" and "inferior" again, but through -interpreter-exec. */
> - return (startswith (command->argv[1], "thread ")
> - || startswith (command->argv[1], "inferior "));
> - }
> -
> - else
> - /* -thread-select already sends it. */
> - return strcmp (command->command, "thread-select") == 0;
> - }
> -}
> -
> void
> mi_execute_command (const char *cmd, int from_tty)
> {
> @@ -1932,8 +1904,6 @@ mi_execute_command (const char *cmd, int from_tty)
>
> if (command != NULL)
> {
> - ptid_t previous_ptid = inferior_ptid;
> -
> command->token = token;
>
> if (do_timings)
> @@ -1963,37 +1933,6 @@ mi_execute_command (const char *cmd, int from_tty)
>
> bpstat_do_actions ();
>
> - if (/* The notifications are only output when the top-level
> - interpreter (specified on the command line) is MI. */
> - top_level_interpreter ()->interp_ui_out ()->is_mi_like_p ()
> - /* Don't try report anything if there are no threads --
> - the program is dead. */
> - && any_thread_p ()
> - /* If the command already reports the thread change, no need to do it
> - again. */
> - && !command_notifies_uscc_observer (command.get ()))
> - {
> - int report_change = 0;
> -
> - if (command->thread == -1)
> - {
> - report_change = (previous_ptid != null_ptid
> - && inferior_ptid != previous_ptid
> - && inferior_ptid != null_ptid);
> - }
> - else if (inferior_ptid != null_ptid)
> - {
> - struct thread_info *ti = inferior_thread ();
> -
> - report_change = (ti->global_num != command->thread);
> - }
> -
> - if (report_change)
> - {
> - gdb::observers::user_selected_context_changed.notify
> - (USER_SELECTED_THREAD | USER_SELECTED_FRAME);
> - }
> - }
> }
> }
>
> @@ -2037,6 +1976,7 @@ mi_cmd_execute (struct mi_parse *parse)
> set_current_program_space (inf->pspace);
> }
>
> + gdb::optional<scoped_restore_current_thread> thread_saver;
> if (parse->thread != -1)
> {
> thread_info *tp = find_thread_global_id (parse->thread);
> @@ -2047,9 +1987,13 @@ mi_cmd_execute (struct mi_parse *parse)
> if (tp->state == THREAD_EXITED)
> error (_("Thread id: %d has terminated"), parse->thread);
>
> + if (parse->cmd->preserve_user_selected_context ())
> + thread_saver.emplace ();
> +
> switch_to_thread (tp);
> }
>
> + gdb::optional<scoped_restore_selected_frame> frame_saver;
> if (parse->frame != -1)
> {
> struct frame_info *fid;
> @@ -2057,8 +2001,12 @@ mi_cmd_execute (struct mi_parse *parse)
>
> fid = find_relative_frame (get_current_frame (), &frame);
> if (frame == 0)
> - /* find_relative_frame was successful */
> - select_frame (fid);
> + {
> + if (parse->cmd->preserve_user_selected_context ())
> + frame_saver.emplace ();
> +
> + select_frame (fid);
> + }
> else
> error (_("Invalid frame id: %d"), frame);
> }
> diff --git a/gdb/testsuite/gdb.mi/mi-cmd-user-context.exp b/gdb/testsuite/gdb.mi/mi-cmd-user-context.exp
> new file mode 100644
> index 00000000000..a373dd3a4b0
> --- /dev/null
> +++ b/gdb/testsuite/gdb.mi/mi-cmd-user-context.exp
> @@ -0,0 +1,155 @@
> +# Copyright 2022 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 that GDB/MI commands preserve user selected context when
> +# passed --thread and/or --frame.
> +
> +load_lib mi-support.exp
> +
> +standard_testfile user-selected-context-sync.c
> +
> +if {[build_executable $testfile.exp $testfile ${srcfile} "debug pthreads"] == -1} {
> + untested "failed to compile"
> + return -1
> +}
> +
> +set main_break_line [gdb_get_line_number "main break line"]
> +
> +set any "\[^\r\n\]*"
> +set nl "\[\r\n\]"
> +
> +mi_clean_restart $binfile
> +mi_create_breakpoint "$srcfile:$main_break_line" "set breakpoint in main"
> +mi_run_cmd
> +mi_expect_stop "breakpoint-hit" "main" "" $srcfile $main_break_line \
> + { "" "disp=\"keep\"" } "run to breakpoint in main"
> +
> +mi_gdb_test "thread" \
> + ".*Current thread is 1.*" \
> + "info thread 1"
> +
> +#=========================
> +
> +mi_gdb_test "-stack-info-depth --thread 3" \
> + "\\^done,depth=.*" \
> + "-stack-info-depth --thread 3"
> +
> +mi_gdb_test "thread" \
> + ".*Current thread is 1.*" \
> + "info thread 2"
> +
> +#=========================
> +
> +mi_gdb_test "-thread-select 3" \
> + "\\^done,.*" \
> + "-thread-select 3"
> +
> +mi_gdb_test "thread" \
> + ".*Current thread is 3.*" \
> + "info thread 3"
> +
> +#=========================
> +
> +mi_gdb_test "-thread-select --thread 2 1" \
> + "\\^done,.*" \
> + "-thread-select --thread 2 1"
> +
> +mi_gdb_test "thread" \
> + ".*Current thread is 1.*" \
> + "info thread 4"
> +
> +#=========================
> +
> +mi_gdb_test "-thread-select --thread 2 2" \
> + "\\^done,.*" \
> + "-thread-select --thread 2 2"
> +
> +mi_gdb_test "thread" \
> + ".*Current thread is 2.*" \
> + "info thread 5"
> +
> +#=========================
> +
> +mi_gdb_test "frame" \
> + ".*#0 0x.*" \
> + "frame 1"
> +
> +mi_gdb_test "-stack-info-frame --thread 2 --frame 1" \
> + "\\^done,frame=\{level=\"1\".*" \
> + "-stack-info-frame 1"
> +
> +mi_gdb_test "thread" \
> + ".*Current thread is 2.*" \
> + "info thread 6"
> +
> +mi_gdb_test "frame" \
> + ".*#0 0x.*" \
> + "frame 2"
> +
> +#=========================
> +
> +mi_gdb_test "-stack-info-frame --thread 3 --frame 1" \
> + "\\^done,frame=\{level=\"1\".*" \
> + "-stack-info-frame 2"
> +
> +mi_gdb_test "thread" \
> + ".*Current thread is 2.*" \
> + "info thread 7"
> +
> +mi_gdb_test "frame" \
> + ".*#0 0x.*" \
> + "frame 3"
> +
> +#=========================
> +
> +mi_gdb_test "-stack-select-frame --thread 2 --frame 0 1" \
> + "\\^done" \
> + "--stack-select-frame 1"
> +
> +mi_gdb_test "thread" \
> + ".*Current thread is 2.*" \
> + "info thread 8"
> +
> +mi_gdb_test "frame" \
> + ".*#1 0x.*" \
> + "frame 4"
> +
> +#=========================
> +
> +mi_gdb_test "-stack-select-frame --thread 2 --frame 2 2" \
> + "\\^done" \
> + "--stack-select-frame 2"
> +
> +mi_gdb_test "thread" \
> + ".*Current thread is 2.*" \
> + "info thread 9"
> +
> +mi_gdb_test "frame" \
> + ".*#2 0x.*" \
> + "frame 5"
> +
> +#=========================
> +
> +mi_gdb_test "-stack-select-frame --thread 1 --frame 0 0" \
> + "\\^done" \
> + "--stack-select-frame 3"
> +
> +mi_gdb_test "thread" \
> + ".*Current thread is 1.*" \
> + "info thread 10"
> +
> +mi_gdb_test "frame" \
> + ".*#0 main.*" \
> + "frame 6"
> --
> 2.34.1
prev parent reply other threads:[~2022-03-08 16:58 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-01-25 14:51 [PATCH 0/1] PR20684, preserve user selected context " Jan Vrany
2022-01-25 14:51 ` [PATCH 1/1] gdb/mi: PR20684, preserve user selected thread and frame " Jan Vrany
2022-01-25 18:59 ` Simon Marchi
2022-01-26 13:21 ` [PATCH v2 0/1] PR20684, preserve user selected context " Jan Vrany
2022-01-26 13:21 ` [PATCH v2 1/1] gdb/mi: PR20684, preserve user selected thread and frame " Jan Vrany
2022-01-26 15:01 ` Simon Marchi
2022-01-26 17:08 ` Jan Vrany
2022-01-27 14:50 ` [PATCH v3 0/1] PR20684, preserve user selected context " Jan Vrany
2022-02-07 11:56 ` Jan Vrany
2022-02-17 16:07 ` [PING] " Jan Vrany
2022-01-27 14:50 ` [PATCH v3 1/1] gdb/mi: PR20684, preserve user selected thread and frame " Jan Vrany
2022-02-17 22:24 ` Andrew Burgess
2022-02-18 17:45 ` Jan Vrany
2022-03-01 11:59 ` [PING] " Jan Vrany
2022-03-02 10:00 ` Andrew Burgess
2022-03-02 13:23 ` [PATCH v4] " Jan Vrany
2022-03-08 16:58 ` Andrew Burgess [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=871qzcwfdo.fsf@redhat.com \
--to=aburgess@redhat.com \
--cc=gdb-patches@sourceware.org \
--cc=jan.vrany@labware.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).