* [RFC PATCH] gdb/mi: consistently notify user when GDB/MI client uses -thread-select @ 2022-03-14 11:57 Jan Vrany 2022-03-15 17:10 ` Andrew Burgess 0 siblings, 1 reply; 4+ messages in thread From: Jan Vrany @ 2022-03-14 11:57 UTC (permalink / raw) To: gdb-patches; +Cc: Jan Vrany, aburgess Hi Andrew, this is a follow-up patch, trying to address inconsistency in GDB/MI notifications you pointed out the other day [1]. The below patch seems to fix it when I manually test it, but I did not manage to write a test. I tried several things, but always get timeouts. See my (failed) attempt in below patch (note, that the expected output is wrong by purpose to ensure the test actually works). At this point I'd like to take you offer of help - if you have an idea how to write the test, I'd be great. [1]: https://sourceware.org/pipermail/gdb-patches/2022-February/185989.html -- >8 -- GDB notifies users about user selected thread changes somewhat inconsistently as mentioned on gdb-patches mailing list here: https://sourceware.org/pipermail/gdb-patches/2022-February/185989.html Consider GDB debugging a multi-threaded inferior with both CLI and GDB/MI interfaces connected to separate terminals. Assuming inferior is stopped and thread 1 is selected, when a thread 2 is selected using '-thread-select 2' command on GDB/MI terminal: -thread-select 2 ^done,new-thread-id="2",frame={level="0",addr="0x00005555555551cd",func="child_sub_function",args=[],file="/home/jv/Projects/gdb/users_jv_patches/gdb/testsuite/gdb.mi/user-selected-context-sync.c",fullname="/home/uuu/gdb/gdb/testsuite/gdb.mi/user-selected-context-sync.c",line="30",arch="i386:x86-64"} (gdb) and on CLI terminal we get the notification (as expected): [Switching to thread 2 (Thread 0x7ffff7daa640 (LWP 389659))] #0 child_sub_function () at /home/uuu/gdb/gdb/testsuite/gdb.mi/user-selected-context-sync.c:30 30 volatile int dummy = 0; However, now that thread 2 is selected, if thread 1 is selected using 'thread-select --thread 1 1' command on GDB/MI terminal terminal: -thread-select --thread 1 1 ^done,new-thread-id="1",frame={level="0",addr="0x0000555555555294",func="main",args=[],file="/home/jv/Projects/gdb/users_jv_patches/gdb/testsuite/gdb.mi/user-selected-context-sync.c",fullname="/home/jv/Projects/gdb/users_jv_patches/gdb/testsuite/gdb.mi/user-selected-context-sync.c",line="66",arch="i386:x86-64"} (gdb) but no notification is printed on CLI terminal, despite the fact that user selected thread has changed. The problem is that when `-thread-select --thread 1 1` is executed then thread is switched to thread 1 before mi_cmd_thread_select () is called, therefore the condition "inferior_ptid != previous_ptid" there does not hold. To address this problem, we have to move notification logic up to mi_cmd_execute () where --thread option is processed and notify user selected contents observers there if context changes. However, this in itself breaks GDB/MI because it would cause context notification to be sent on MI channel. This is because by the time we notify, MI notification suppression is already restored (done in mi_command::invoke(). Therefore we had to lift notification suppression logic also up to mi_cmd_execute (). With this change, all gdb.mi tests pass, tested on x86_64-linux. TODO: * fix test --- gdb/mi/mi-cmd-stack.c | 10 --- gdb/mi/mi-cmds.c | 2 - gdb/mi/mi-cmds.h | 16 ++-- gdb/mi/mi-main.c | 32 +++++--- ...mi-user-selected-context-notifications.exp | 78 +++++++++++++++++++ 5 files changed, 109 insertions(+), 29 deletions(-) create mode 100644 gdb/testsuite/gdb.mi/mi-user-selected-context-notifications.exp diff --git a/gdb/mi/mi-cmd-stack.c b/gdb/mi/mi-cmd-stack.c index 1be8aa81c3d..afe584b7fca 100644 --- a/gdb/mi/mi-cmd-stack.c +++ b/gdb/mi/mi-cmd-stack.c @@ -757,17 +757,7 @@ 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.c b/gdb/mi/mi-cmds.c index cd7cabdda9b..957dfb4e03d 100644 --- a/gdb/mi/mi-cmds.c +++ b/gdb/mi/mi-cmds.c @@ -171,8 +171,6 @@ mi_command::mi_command (const char *name, int *suppress_notification) void mi_command::invoke (struct mi_parse *parse) const { - gdb::optional<scoped_restore_tmpl<int>> restore - = do_suppress_notification (); this->do_invoke (parse); } diff --git a/gdb/mi/mi-cmds.h b/gdb/mi/mi-cmds.h index 785652ee1c9..da1598a3f32 100644 --- a/gdb/mi/mi-cmds.h +++ b/gdb/mi/mi-cmds.h @@ -175,14 +175,6 @@ struct mi_command return m_suppress_notification != &mi_suppress_notification.user_selected_context; } -protected: - - /* The core of command invocation, this needs to be overridden in each - base class. PARSE is the parsed command line from the user. */ - virtual void do_invoke (struct mi_parse *parse) const = 0; - -private: - /* If this command was created with a suppress notifications pointer, then this function will set the suppress flag and return a gdb::optional with its value set to an object that will restore the @@ -192,6 +184,14 @@ struct mi_command then this function returns an empty gdb::optional. */ gdb::optional<scoped_restore_tmpl<int>> do_suppress_notification () const; +protected: + + /* The core of command invocation, this needs to be overridden in each + base class. PARSE is the parsed command line from the user. */ + virtual void do_invoke (struct mi_parse *parse) const = 0; + +private: + /* The name of the command. */ const char *m_name; diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c index 73380f5e668..a5b847c7039 100644 --- a/gdb/mi/mi-main.c +++ b/gdb/mi/mi-main.c @@ -556,19 +556,10 @@ mi_cmd_thread_select (const char *command, char **argv, int argc) if (thr == NULL) error (_("Thread ID %d not known."), num); - ptid_t previous_ptid = inferior_ptid; - thread_select (argv[0], thr); print_selected_thread_frame (current_uiout, USER_SELECTED_THREAD | USER_SELECTED_FRAME); - - /* 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 @@ -1975,6 +1966,16 @@ mi_execute_command (const char *cmd, int from_tty) } } +/* Determine whether the thread has changed. */ + +static bool +command_changed_user_selected_thread (ptid_t previous_ptid, ptid_t current_ptid) +{ + return (previous_ptid != null_ptid + && current_ptid != previous_ptid + && current_ptid != null_ptid); +} + static void mi_cmd_execute (struct mi_parse *parse) { @@ -2015,6 +2016,8 @@ mi_cmd_execute (struct mi_parse *parse) set_current_program_space (inf->pspace); } + ptid_t previous_ptid = inferior_ptid; + gdb::optional<scoped_restore_current_thread> thread_saver; if (parse->thread != -1) { @@ -2060,7 +2063,18 @@ mi_cmd_execute (struct mi_parse *parse) current_context = parse; gdb_assert (parse->cmd != nullptr); + + gdb::optional<scoped_restore_tmpl<int>> restore_suppress_notification + = parse->cmd->do_suppress_notification (); + parse->cmd->invoke (parse); + + if (!parse->cmd->preserve_user_selected_context () + && command_changed_user_selected_thread (previous_ptid, inferior_ptid)) + { + gdb::observers::user_selected_context_changed.notify + (USER_SELECTED_THREAD | USER_SELECTED_FRAME); + } } /* See mi-main.h. */ diff --git a/gdb/testsuite/gdb.mi/mi-user-selected-context-notifications.exp b/gdb/testsuite/gdb.mi/mi-user-selected-context-notifications.exp new file mode 100644 index 00000000000..cf9c02e54a4 --- /dev/null +++ b/gdb/testsuite/gdb.mi/mi-user-selected-context-notifications.exp @@ -0,0 +1,78 @@ +# Copyright 2022-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 user is propery notified on CLI channel when. +# selected thread and/or frame is changed using GDB/MI. +# +# See https://sourceware.org/pipermail/gdb-patches/2022-February/185989.html + +load_lib mi-support.exp + +standard_testfile user-selected-context-sync.c + +# Multiple threads are needed, therefore only native gdb and extended +# gdbserver modes are supported. +if [use_gdb_stub] { + untested "using gdb stub" + return +} + +if {[build_executable $testfile.exp $testfile ${srcfile} "debug pthreads"] == -1} { + untested "failed to compile" + return -1 +} + +if [eval mi_gdb_start { "separate-mi-tty" }] { + return +} + +# Useful for debugging: +verbose -log "Channels:" +verbose -log " inferior_spawn_id=$inferior_spawn_id" +verbose -log " gdb_spawn_id=$gdb_spawn_id" +verbose -log " gdb_main_spawn_id=$gdb_main_spawn_id" +verbose -log " mi_spawn_id=$mi_spawn_id" + +mi_gdb_load $binfile +if { [mi_runto_main] < 0 } { + return +} + +switch_gdb_spawn_id $gdb_main_spawn_id + +gdb_test "break $srcfile:child_function" \ + "Breakpoint.*at.* file .*$srcfile, line.*" \ + "breakpoint function in file" + +gdb_test "continue" \ + ".*Thread 2.*hit Breakpoint 2.*" \ + "continue to child_function" + +switch_gdb_spawn_id $mi_spawn_id + +send_gdb "111-thread-select 1" +gdb_expect { + -i "$mi_spawn_id" + -re "\\^done,new-thread-id=\"2\".*" { + fail "111-thread-select 1 (MI output)" + } + -i "$gdb_main_spawn_id" + -re "\\\[Switching to thread 1.*" { + fail "111-thread-select 1 (CLI output)" + } + timeout { + fail "111-thread-select 1 (timeout)" + } +} -- 2.35.1 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RFC PATCH] gdb/mi: consistently notify user when GDB/MI client uses -thread-select 2022-03-14 11:57 [RFC PATCH] gdb/mi: consistently notify user when GDB/MI client uses -thread-select Jan Vrany @ 2022-03-15 17:10 ` Andrew Burgess 2022-03-16 14:03 ` [PATCH v2] " Jan Vrany 0 siblings, 1 reply; 4+ messages in thread From: Andrew Burgess @ 2022-03-15 17:10 UTC (permalink / raw) To: Jan Vrany; +Cc: gdb-patches * Jan Vrany via Gdb-patches <gdb-patches@sourceware.org> [2022-03-14 11:57:41 +0000]: > Hi Andrew, > > this is a follow-up patch, trying to address inconsistency in GDB/MI > notifications you pointed out the other day [1]. > > The below patch seems to fix it when I manually test it, but I did not > manage to write a test. I tried several things, but always get timeouts. > See my (failed) attempt in below patch (note, that the expected output is > wrong by purpose to ensure the test actually works). > > At this point I'd like to take you offer of help - if you have an idea how > to write the test, I'd be great. No problem. Thanks for looking at this, the work so far looks great. As for testing, I can't claim too much credit, I ended up dropping the test you'd started, and just extended the existing test script gdb.mi/user-selected-context-sync.exp as this covers all the related cases, just not the specific cases you are testing. Except, it does actually cover one of the cases you are fixing - the test includes a kfail for bug gdb/20631 - which is fixed by your commit - good job! So while I was writing the tests I was also adding tests for selecting the frame, as this seemed like a related problem, checking things like this: # Currently in thread 2, frame 0, then... -stack-select-frame --thread 2 --frame 1 1 After this I would expect a change of frame announcement in the CLI, which didn't work with your original patch. The patch below includes a fix for this problem too, as well as some tests for this. I don't know if you had any other changes you wanted to make to this patch or not, feel free to review the changes I've made, and if you're happy, just post this to the list as a v2 for review. Thanks, Andrew --- commit ae700a9c07c3573e1e7adde0905156a75aa22aac Author: Jan Vrany <jan.vrany@labware.com> Date: Mon Mar 14 11:57:41 2022 +0000 gdb/mi: consistently notify user when GDB/MI client uses -thread-select GDB notifies users about user selected thread changes somewhat inconsistently as mentioned on gdb-patches mailing list here: https://sourceware.org/pipermail/gdb-patches/2022-February/185989.html Consider GDB debugging a multi-threaded inferior with both CLI and GDB/MI interfaces connected to separate terminals. Assuming inferior is stopped and thread 1 is selected, when a thread 2 is selected using '-thread-select 2' command on GDB/MI terminal: -thread-select 2 ^done,new-thread-id="2",frame={level="0",addr="0x00005555555551cd",func="child_sub_function",args=[],file="/home/jv/Projects/gdb/users_jv_patches/gdb/testsuite/gdb.mi/user-selected-context-sync.c",fullname="/home/uuu/gdb/gdb/testsuite/gdb.mi/user-selected-context-sync.c",line="30",arch="i386:x86-64"} (gdb) and on CLI terminal we get the notification (as expected): [Switching to thread 2 (Thread 0x7ffff7daa640 (LWP 389659))] #0 child_sub_function () at /home/uuu/gdb/gdb/testsuite/gdb.mi/user-selected-context-sync.c:30 30 volatile int dummy = 0; However, now that thread 2 is selected, if thread 1 is selected using 'thread-select --thread 1 1' command on GDB/MI terminal terminal: -thread-select --thread 1 1 ^done,new-thread-id="1",frame={level="0",addr="0x0000555555555294",func="main",args=[],file="/home/jv/Projects/gdb/users_jv_patches/gdb/testsuite/gdb.mi/user-selected-context-sync.c",fullname="/home/jv/Projects/gdb/users_jv_patches/gdb/testsuite/gdb.mi/user-selected-context-sync.c",line="66",arch="i386:x86-64"} (gdb) but no notification is printed on CLI terminal, despite the fact that user selected thread has changed. The problem is that when `-thread-select --thread 1 1` is executed then thread is switched to thread 1 before mi_cmd_thread_select () is called, therefore the condition "inferior_ptid != previous_ptid" there does not hold. To address this problem, we have to move notification logic up to mi_cmd_execute () where --thread option is processed and notify user selected contents observers there if context changes. However, this in itself breaks GDB/MI because it would cause context notification to be sent on MI channel. This is because by the time we notify, MI notification suppression is already restored (done in mi_command::invoke(). Therefore we had to lift notification suppression logic also up to mi_cmd_execute (). With this change, all gdb.mi tests pass, tested on x86_64-linux. Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=20631 diff --git a/gdb/mi/mi-cmd-stack.c b/gdb/mi/mi-cmd-stack.c index 1be8aa81c3d..e894411765a 100644 --- a/gdb/mi/mi-cmd-stack.c +++ b/gdb/mi/mi-cmd-stack.c @@ -757,17 +757,7 @@ 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); - } + select_frame (parse_frame_specification (argv[0])); } void diff --git a/gdb/mi/mi-cmds.c b/gdb/mi/mi-cmds.c index 38fbe0d8a32..d21b5d48548 100644 --- a/gdb/mi/mi-cmds.c +++ b/gdb/mi/mi-cmds.c @@ -176,8 +176,6 @@ mi_command::mi_command (const char *name, int *suppress_notification) void mi_command::invoke (struct mi_parse *parse) const { - gdb::optional<scoped_restore_tmpl<int>> restore - = do_suppress_notification (); this->do_invoke (parse); } diff --git a/gdb/mi/mi-cmds.h b/gdb/mi/mi-cmds.h index 47b90a26064..41806f1d334 100644 --- a/gdb/mi/mi-cmds.h +++ b/gdb/mi/mi-cmds.h @@ -175,14 +175,6 @@ struct mi_command return m_suppress_notification != &mi_suppress_notification.user_selected_context; } -protected: - - /* The core of command invocation, this needs to be overridden in each - base class. PARSE is the parsed command line from the user. */ - virtual void do_invoke (struct mi_parse *parse) const = 0; - -private: - /* If this command was created with a suppress notifications pointer, then this function will set the suppress flag and return a gdb::optional with its value set to an object that will restore the @@ -192,6 +184,14 @@ struct mi_command then this function returns an empty gdb::optional. */ gdb::optional<scoped_restore_tmpl<int>> do_suppress_notification () const; +protected: + + /* The core of command invocation, this needs to be overridden in each + base class. PARSE is the parsed command line from the user. */ + virtual void do_invoke (struct mi_parse *parse) const = 0; + +private: + /* The name of the command. */ const char *m_name; diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c index 73380f5e668..abd033b22ae 100644 --- a/gdb/mi/mi-main.c +++ b/gdb/mi/mi-main.c @@ -556,19 +556,10 @@ mi_cmd_thread_select (const char *command, char **argv, int argc) if (thr == NULL) error (_("Thread ID %d not known."), num); - ptid_t previous_ptid = inferior_ptid; - thread_select (argv[0], thr); print_selected_thread_frame (current_uiout, USER_SELECTED_THREAD | USER_SELECTED_FRAME); - - /* 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 @@ -1975,6 +1966,37 @@ mi_execute_command (const char *cmd, int from_tty) } } +/* Captures the current user selected context state, that is the current + thread and frame. Later we can then check if the user selected context + has changed at all. */ + +struct user_selected_context +{ + /* Constructor. */ + user_selected_context () + : m_previous_ptid (inferior_ptid), + m_previous_frame (deprecated_safe_get_selected_frame ()) + { /* Nothing. */ } + + /* Return true if the user selected context has changed since this object + was created. */ + bool has_changed () const + { + return ((m_previous_ptid != null_ptid + && inferior_ptid != null_ptid + && m_previous_ptid != inferior_ptid) + || m_previous_frame != deprecated_safe_get_selected_frame ()); + } +private: + /* The previously selected thread. This might be null_ptid if there was + no previously selected thread. */ + ptid_t m_previous_ptid; + + /* The previously selected frame. This might be nullptr if there was no + previously selected frame. */ + frame_info *m_previous_frame; +}; + static void mi_cmd_execute (struct mi_parse *parse) { @@ -2015,6 +2037,8 @@ mi_cmd_execute (struct mi_parse *parse) set_current_program_space (inf->pspace); } + user_selected_context current_user_selected_context; + gdb::optional<scoped_restore_current_thread> thread_saver; if (parse->thread != -1) { @@ -2060,7 +2084,16 @@ mi_cmd_execute (struct mi_parse *parse) current_context = parse; gdb_assert (parse->cmd != nullptr); + + gdb::optional<scoped_restore_tmpl<int>> restore_suppress_notification + = parse->cmd->do_suppress_notification (); + parse->cmd->invoke (parse); + + if (!parse->cmd->preserve_user_selected_context () + && current_user_selected_context.has_changed ()) + gdb::observers::user_selected_context_changed.notify + (USER_SELECTED_THREAD | USER_SELECTED_FRAME); } /* See mi-main.h. */ diff --git a/gdb/stack.c b/gdb/stack.c index 8855ed8fe70..10da88b88e5 100644 --- a/gdb/stack.c +++ b/gdb/stack.c @@ -1841,14 +1841,6 @@ select_frame_command_core (struct frame_info *fi, bool ignored) gdb::observers::user_selected_context_changed.notify (USER_SELECTED_FRAME); } -/* See stack.h. */ - -void -select_frame_for_mi (struct frame_info *fi) -{ - select_frame_command_core (fi, false /* Ignored. */); -} - /* The core of all the "frame" sub-commands. Select frame FI, and if this means we change frame send out a change notification (otherwise, just reprint the current frame summary). */ diff --git a/gdb/stack.h b/gdb/stack.h index c49d2e2cbef..f78aedf1c85 100644 --- a/gdb/stack.h +++ b/gdb/stack.h @@ -20,12 +20,6 @@ #ifndef STACK_H #define STACK_H -/* Access method used by the MI -stack-select-frame command to switch to - frame FI. This differs from SELECT_FRAME in that the observers for a - user selected context change will be triggered. */ - -void select_frame_for_mi (struct frame_info *fi); - gdb::unique_xmalloc_ptr<char> find_frame_funname (struct frame_info *frame, enum language *funlang, struct symbol **funcp); diff --git a/gdb/testsuite/gdb.mi/user-selected-context-sync.exp b/gdb/testsuite/gdb.mi/user-selected-context-sync.exp index 3b4ff0bf2e4..9444ca5acf4 100644 --- a/gdb/testsuite/gdb.mi/user-selected-context-sync.exp +++ b/gdb/testsuite/gdb.mi/user-selected-context-sync.exp @@ -916,7 +916,7 @@ proc_with_prefix test_mi_thread_select { mode } { } } - with_test_prefix "thread 1.2 with --thread" { + with_test_prefix "thread 1.2 with --thread 2" { # Test selecting a thread from MI with a --thread option. This test # verifies that even if the thread GDB would switch to is the same as # the thread specified with --thread, an event is still sent to CLI. @@ -930,10 +930,28 @@ proc_with_prefix test_mi_thread_select { mode } { } with_spawn_id $gdb_main_spawn_id { - # This doesn't work as of now, no event is sent on CLI. It is - # commented out so we don't have to wait for the timeout every time. - # match_re_or_ensure_no_output "$cli_re\r\n" "-thread-select, event on cli" - kfail "gdb/20631" "thread-select, event on cli" + match_re_or_ensure_no_output "$cli_re\r\n" "-thread-select, event on cli" + } + } + + with_test_prefix "thread 1.2 with --thread 3" { + # Test selecting a thread from MI with a --thread option. + # This test verifies that when different thread numbers are + # passed to the --thread option and the underlying + # -thread-select command, the correct thread is selected. + # In this case this is thread 1.2 + + reset_selection "1.1" + + set mi_re [make_mi_re $mode 2 0 response] + set cli_re [make_cli_re $mode -1 1.2 0] + + with_spawn_id $mi_spawn_id { + mi_gdb_test "-thread-select --thread 3 2" $mi_re "-thread-select" + } + + with_spawn_id $gdb_main_spawn_id { + match_re_or_ensure_no_output "$cli_re\r\n" "-thread-select, event on cli" } } @@ -974,6 +992,36 @@ proc_with_prefix test_mi_stack_select_frame { mode } { with_spawn_id $gdb_main_spawn_id { match_re_or_ensure_no_output $cli_re "-stack-select-frame again, event on CLI" } + + # Now use the '-stack-select-frame' command with the --frame + # option, this verifies that even when the frame GDB would + # swith to is the same as the frame specified with --frame, an + # event is still sent to the CLI. + + set cli_re [make_cli_re $mode -1 -1 0] + + with_spawn_id $mi_spawn_id { + mi_gdb_test "-stack-select-frame --thread 2 --frame 0 0" $mi_re + } + + with_spawn_id $gdb_main_spawn_id { + match_re_or_ensure_no_output "$cli_re\r\n" "-stack-select-frame with --frame 0, event on CLI" + } + + # Now use the '-stack-select-frame' command with the --frame + # option, this verifies that the correct event is sent to the + # CLI when the frame specified with --frame is different to + # the actual frame selected. + + set cli_re [make_cli_re $mode -1 -1 1] + + with_spawn_id $mi_spawn_id { + mi_gdb_test "-stack-select-frame --thread 2 --frame 2 1" $mi_re + } + + with_spawn_id $gdb_main_spawn_id { + match_re_or_ensure_no_output "$cli_re\r\n" "-stack-select-frame with --frame 2, event on CLI" + } } with_test_prefix "thread 1.3" { ^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH v2] gdb/mi: consistently notify user when GDB/MI client uses -thread-select 2022-03-15 17:10 ` Andrew Burgess @ 2022-03-16 14:03 ` Jan Vrany 2022-03-16 14:35 ` Andrew Burgess 0 siblings, 1 reply; 4+ messages in thread From: Jan Vrany @ 2022-03-16 14:03 UTC (permalink / raw) To: gdb-patches; +Cc: Jan Vrany, aburgess Changes since v1: * all improvements and tests by Andrew - thanks a lot! * merged mi_command::invoke() and mi_command::do_invoke() into single method (mi_command::invoke()). See the commit message for rationale. -- >8 -- GDB notifies users about user selected thread changes somewhat inconsistently as mentioned on gdb-patches mailing list here: https://sourceware.org/pipermail/gdb-patches/2022-February/185989.html Consider GDB debugging a multi-threaded inferior with both CLI and GDB/MI interfaces connected to separate terminals. Assuming inferior is stopped and thread 1 is selected, when a thread 2 is selected using '-thread-select 2' command on GDB/MI terminal: -thread-select 2 ^done,new-thread-id="2",frame={level="0",addr="0x00005555555551cd",func="child_sub_function",args=[],file="/home/jv/Projects/gdb/users_jv_patches/gdb/testsuite/gdb.mi/user-selected-context-sync.c",fullname="/home/uuu/gdb/gdb/testsuite/gdb.mi/user-selected-context-sync.c",line="30",arch="i386:x86-64"} (gdb) and on CLI terminal we get the notification (as expected): [Switching to thread 2 (Thread 0x7ffff7daa640 (LWP 389659))] #0 child_sub_function () at /home/uuu/gdb/gdb/testsuite/gdb.mi/user-selected-context-sync.c:30 30 volatile int dummy = 0; However, now that thread 2 is selected, if thread 1 is selected using 'thread-select --thread 1 1' command on GDB/MI terminal terminal: -thread-select --thread 1 1 ^done,new-thread-id="1",frame={level="0",addr="0x0000555555555294",func="main",args=[],file="/home/jv/Projects/gdb/users_jv_patches/gdb/testsuite/gdb.mi/user-selected-context-sync.c",fullname="/home/jv/Projects/gdb/users_jv_patches/gdb/testsuite/gdb.mi/user-selected-context-sync.c",line="66",arch="i386:x86-64"} (gdb) but no notification is printed on CLI terminal, despite the fact that user selected thread has changed. The problem is that when `-thread-select --thread 1 1` is executed then thread is switched to thread 1 before mi_cmd_thread_select () is called, therefore the condition "inferior_ptid != previous_ptid" there does not hold. To address this problem, we have to move notification logic up to mi_cmd_execute () where --thread option is processed and notify user selected contents observers there if context changes. However, this in itself breaks GDB/MI because it would cause context notification to be sent on MI channel. This is because by the time we notify, MI notification suppression is already restored (done in mi_command::invoke(). Therefore we had to lift notification suppression logic also up to mi_cmd_execute (). This change in made distinction between mi_command::invoke() and mi_command::do_invoke() unnecessary as all mi_command::invoke() did (after the change) was to call do_invoke(). So this patches removes do_invoke() and moves the command execution logic directly to invoke(). With this change, all gdb.mi tests pass, tested on x86_64-linux. Co-authored-by: Andrew Burgess <aburgess@redhat.com> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=20631 --- gdb/mi/mi-cmd-stack.c | 12 +--- gdb/mi/mi-cmds.c | 18 +----- gdb/mi/mi-cmds.h | 17 ++---- gdb/mi/mi-main.c | 51 +++++++++++++--- gdb/python/py-micmd.c | 5 +- gdb/stack.c | 8 --- gdb/stack.h | 6 -- .../gdb.mi/user-selected-context-sync.exp | 58 +++++++++++++++++-- 8 files changed, 106 insertions(+), 69 deletions(-) diff --git a/gdb/mi/mi-cmd-stack.c b/gdb/mi/mi-cmd-stack.c index 1be8aa81c3d..e894411765a 100644 --- a/gdb/mi/mi-cmd-stack.c +++ b/gdb/mi/mi-cmd-stack.c @@ -757,17 +757,7 @@ 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); - } + select_frame (parse_frame_specification (argv[0])); } void diff --git a/gdb/mi/mi-cmds.c b/gdb/mi/mi-cmds.c index 38fbe0d8a32..60fec0a0b85 100644 --- a/gdb/mi/mi-cmds.c +++ b/gdb/mi/mi-cmds.c @@ -45,11 +45,9 @@ struct mi_command_mi : public mi_command gdb_assert (func != nullptr); } -protected: - /* Called when this MI command has been invoked, calls m_argv_function with arguments contained within PARSE. */ - void do_invoke (struct mi_parse *parse) const override + void invoke (struct mi_parse *parse) const override { mi_parse_argv (parse->args, parse); @@ -83,13 +81,11 @@ struct mi_command_cli : public mi_command m_args_p (args_p) { /* Nothing. */ } -protected: - /* Called when this MI command has been invoked, calls the m_cli_name CLI function. In m_args_p is true then the argument string from within PARSE is passed through to the CLI function, otherwise nullptr is passed through to the CLI function as its argument string. */ - void do_invoke (struct mi_parse *parse) const override + void invoke (struct mi_parse *parse) const override { const char *args = m_args_p ? parse->args : nullptr; mi_execute_cli_command (m_cli_name, m_args_p, args); @@ -173,16 +169,6 @@ mi_command::mi_command (const char *name, int *suppress_notification) /* See mi-cmds.h. */ -void -mi_command::invoke (struct mi_parse *parse) const -{ - gdb::optional<scoped_restore_tmpl<int>> restore - = do_suppress_notification (); - this->do_invoke (parse); -} - -/* See mi-cmds.h. */ - gdb::optional<scoped_restore_tmpl<int>> mi_command::do_suppress_notification () const { diff --git a/gdb/mi/mi-cmds.h b/gdb/mi/mi-cmds.h index 47b90a26064..05b702f83ec 100644 --- a/gdb/mi/mi-cmds.h +++ b/gdb/mi/mi-cmds.h @@ -160,9 +160,10 @@ struct mi_command const char *name () const { return m_name; } - /* Execute the MI command. Can throw an exception if something goes - wrong. */ - void invoke (struct mi_parse *parse) const; + /* Execute the MI command. this needs to be overridden in each + base class. PARSE is the parsed command line from the user. + Can throw an exception if something goes wrong. */ + virtual void invoke (struct mi_parse *parse) const = 0; /* Return whether this command preserves user selected context (thread and frame). */ @@ -175,14 +176,6 @@ struct mi_command return m_suppress_notification != &mi_suppress_notification.user_selected_context; } -protected: - - /* The core of command invocation, this needs to be overridden in each - base class. PARSE is the parsed command line from the user. */ - virtual void do_invoke (struct mi_parse *parse) const = 0; - -private: - /* If this command was created with a suppress notifications pointer, then this function will set the suppress flag and return a gdb::optional with its value set to an object that will restore the @@ -192,6 +185,8 @@ struct mi_command then this function returns an empty gdb::optional. */ gdb::optional<scoped_restore_tmpl<int>> do_suppress_notification () const; +private: + /* The name of the command. */ const char *m_name; diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c index 73380f5e668..abd033b22ae 100644 --- a/gdb/mi/mi-main.c +++ b/gdb/mi/mi-main.c @@ -556,19 +556,10 @@ mi_cmd_thread_select (const char *command, char **argv, int argc) if (thr == NULL) error (_("Thread ID %d not known."), num); - ptid_t previous_ptid = inferior_ptid; - thread_select (argv[0], thr); print_selected_thread_frame (current_uiout, USER_SELECTED_THREAD | USER_SELECTED_FRAME); - - /* 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 @@ -1975,6 +1966,37 @@ mi_execute_command (const char *cmd, int from_tty) } } +/* Captures the current user selected context state, that is the current + thread and frame. Later we can then check if the user selected context + has changed at all. */ + +struct user_selected_context +{ + /* Constructor. */ + user_selected_context () + : m_previous_ptid (inferior_ptid), + m_previous_frame (deprecated_safe_get_selected_frame ()) + { /* Nothing. */ } + + /* Return true if the user selected context has changed since this object + was created. */ + bool has_changed () const + { + return ((m_previous_ptid != null_ptid + && inferior_ptid != null_ptid + && m_previous_ptid != inferior_ptid) + || m_previous_frame != deprecated_safe_get_selected_frame ()); + } +private: + /* The previously selected thread. This might be null_ptid if there was + no previously selected thread. */ + ptid_t m_previous_ptid; + + /* The previously selected frame. This might be nullptr if there was no + previously selected frame. */ + frame_info *m_previous_frame; +}; + static void mi_cmd_execute (struct mi_parse *parse) { @@ -2015,6 +2037,8 @@ mi_cmd_execute (struct mi_parse *parse) set_current_program_space (inf->pspace); } + user_selected_context current_user_selected_context; + gdb::optional<scoped_restore_current_thread> thread_saver; if (parse->thread != -1) { @@ -2060,7 +2084,16 @@ mi_cmd_execute (struct mi_parse *parse) current_context = parse; gdb_assert (parse->cmd != nullptr); + + gdb::optional<scoped_restore_tmpl<int>> restore_suppress_notification + = parse->cmd->do_suppress_notification (); + parse->cmd->invoke (parse); + + if (!parse->cmd->preserve_user_selected_context () + && current_user_selected_context.has_changed ()) + gdb::observers::user_selected_context_changed.notify + (USER_SELECTED_THREAD | USER_SELECTED_FRAME); } /* See mi-main.h. */ diff --git a/gdb/python/py-micmd.c b/gdb/python/py-micmd.c index 4665fcc75c7..399e89b0298 100644 --- a/gdb/python/py-micmd.c +++ b/gdb/python/py-micmd.c @@ -134,9 +134,8 @@ struct mi_command_py : public mi_command m_pyobj = new_pyobj; } -protected: /* Called when the MI command is invoked. */ - virtual void do_invoke(struct mi_parse *parse) const override; + virtual void invoke(struct mi_parse *parse) const override; private: /* The Python object representing this MI command. */ @@ -311,7 +310,7 @@ serialize_mi_result (PyObject *result) command line arguments from the user. */ void -mi_command_py::do_invoke (struct mi_parse *parse) const +mi_command_py::invoke (struct mi_parse *parse) const { PYMICMD_SCOPED_DEBUG_ENTER_EXIT; diff --git a/gdb/stack.c b/gdb/stack.c index 8855ed8fe70..10da88b88e5 100644 --- a/gdb/stack.c +++ b/gdb/stack.c @@ -1841,14 +1841,6 @@ select_frame_command_core (struct frame_info *fi, bool ignored) gdb::observers::user_selected_context_changed.notify (USER_SELECTED_FRAME); } -/* See stack.h. */ - -void -select_frame_for_mi (struct frame_info *fi) -{ - select_frame_command_core (fi, false /* Ignored. */); -} - /* The core of all the "frame" sub-commands. Select frame FI, and if this means we change frame send out a change notification (otherwise, just reprint the current frame summary). */ diff --git a/gdb/stack.h b/gdb/stack.h index c49d2e2cbef..f78aedf1c85 100644 --- a/gdb/stack.h +++ b/gdb/stack.h @@ -20,12 +20,6 @@ #ifndef STACK_H #define STACK_H -/* Access method used by the MI -stack-select-frame command to switch to - frame FI. This differs from SELECT_FRAME in that the observers for a - user selected context change will be triggered. */ - -void select_frame_for_mi (struct frame_info *fi); - gdb::unique_xmalloc_ptr<char> find_frame_funname (struct frame_info *frame, enum language *funlang, struct symbol **funcp); diff --git a/gdb/testsuite/gdb.mi/user-selected-context-sync.exp b/gdb/testsuite/gdb.mi/user-selected-context-sync.exp index 3b4ff0bf2e4..9444ca5acf4 100644 --- a/gdb/testsuite/gdb.mi/user-selected-context-sync.exp +++ b/gdb/testsuite/gdb.mi/user-selected-context-sync.exp @@ -916,7 +916,7 @@ proc_with_prefix test_mi_thread_select { mode } { } } - with_test_prefix "thread 1.2 with --thread" { + with_test_prefix "thread 1.2 with --thread 2" { # Test selecting a thread from MI with a --thread option. This test # verifies that even if the thread GDB would switch to is the same as # the thread specified with --thread, an event is still sent to CLI. @@ -930,10 +930,28 @@ proc_with_prefix test_mi_thread_select { mode } { } with_spawn_id $gdb_main_spawn_id { - # This doesn't work as of now, no event is sent on CLI. It is - # commented out so we don't have to wait for the timeout every time. - # match_re_or_ensure_no_output "$cli_re\r\n" "-thread-select, event on cli" - kfail "gdb/20631" "thread-select, event on cli" + match_re_or_ensure_no_output "$cli_re\r\n" "-thread-select, event on cli" + } + } + + with_test_prefix "thread 1.2 with --thread 3" { + # Test selecting a thread from MI with a --thread option. + # This test verifies that when different thread numbers are + # passed to the --thread option and the underlying + # -thread-select command, the correct thread is selected. + # In this case this is thread 1.2 + + reset_selection "1.1" + + set mi_re [make_mi_re $mode 2 0 response] + set cli_re [make_cli_re $mode -1 1.2 0] + + with_spawn_id $mi_spawn_id { + mi_gdb_test "-thread-select --thread 3 2" $mi_re "-thread-select" + } + + with_spawn_id $gdb_main_spawn_id { + match_re_or_ensure_no_output "$cli_re\r\n" "-thread-select, event on cli" } } @@ -974,6 +992,36 @@ proc_with_prefix test_mi_stack_select_frame { mode } { with_spawn_id $gdb_main_spawn_id { match_re_or_ensure_no_output $cli_re "-stack-select-frame again, event on CLI" } + + # Now use the '-stack-select-frame' command with the --frame + # option, this verifies that even when the frame GDB would + # swith to is the same as the frame specified with --frame, an + # event is still sent to the CLI. + + set cli_re [make_cli_re $mode -1 -1 0] + + with_spawn_id $mi_spawn_id { + mi_gdb_test "-stack-select-frame --thread 2 --frame 0 0" $mi_re + } + + with_spawn_id $gdb_main_spawn_id { + match_re_or_ensure_no_output "$cli_re\r\n" "-stack-select-frame with --frame 0, event on CLI" + } + + # Now use the '-stack-select-frame' command with the --frame + # option, this verifies that the correct event is sent to the + # CLI when the frame specified with --frame is different to + # the actual frame selected. + + set cli_re [make_cli_re $mode -1 -1 1] + + with_spawn_id $mi_spawn_id { + mi_gdb_test "-stack-select-frame --thread 2 --frame 2 1" $mi_re + } + + with_spawn_id $gdb_main_spawn_id { + match_re_or_ensure_no_output "$cli_re\r\n" "-stack-select-frame with --frame 2, event on CLI" + } } with_test_prefix "thread 1.3" { -- 2.35.1 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] gdb/mi: consistently notify user when GDB/MI client uses -thread-select 2022-03-16 14:03 ` [PATCH v2] " Jan Vrany @ 2022-03-16 14:35 ` Andrew Burgess 0 siblings, 0 replies; 4+ messages in thread From: Andrew Burgess @ 2022-03-16 14:35 UTC (permalink / raw) To: Jan Vrany, gdb-patches; +Cc: Jan Vrany Jan Vrany <jan.vrany@labware.com> writes: > Changes since v1: > > * all improvements and tests by Andrew - thanks a lot! > * merged mi_command::invoke() and mi_command::do_invoke() into > single method (mi_command::invoke()). See the commit message > for rationale. > > -- >8 -- > GDB notifies users about user selected thread changes somewhat > inconsistently as mentioned on gdb-patches mailing list here: > > https://sourceware.org/pipermail/gdb-patches/2022-February/185989.html > > Consider GDB debugging a multi-threaded inferior with both CLI and GDB/MI > interfaces connected to separate terminals. > > Assuming inferior is stopped and thread 1 is selected, when a thread > 2 is selected using '-thread-select 2' command on GDB/MI terminal: > > -thread-select 2 > ^done,new-thread-id="2",frame={level="0",addr="0x00005555555551cd",func="child_sub_function",args=[],file="/home/jv/Projects/gdb/users_jv_patches/gdb/testsuite/gdb.mi/user-selected-context-sync.c",fullname="/home/uuu/gdb/gdb/testsuite/gdb.mi/user-selected-context-sync.c",line="30",arch="i386:x86-64"} > (gdb) > > and on CLI terminal we get the notification (as expected): > > [Switching to thread 2 (Thread 0x7ffff7daa640 (LWP 389659))] > #0 child_sub_function () at /home/uuu/gdb/gdb/testsuite/gdb.mi/user-selected-context-sync.c:30 > 30 volatile int dummy = 0; > > However, now that thread 2 is selected, if thread 1 is selected > using 'thread-select --thread 1 1' command on GDB/MI terminal > terminal: > > -thread-select --thread 1 1 > ^done,new-thread-id="1",frame={level="0",addr="0x0000555555555294",func="main",args=[],file="/home/jv/Projects/gdb/users_jv_patches/gdb/testsuite/gdb.mi/user-selected-context-sync.c",fullname="/home/jv/Projects/gdb/users_jv_patches/gdb/testsuite/gdb.mi/user-selected-context-sync.c",line="66",arch="i386:x86-64"} > (gdb) > > but no notification is printed on CLI terminal, despite the fact > that user selected thread has changed. > > The problem is that when `-thread-select --thread 1 1` is executed > then thread is switched to thread 1 before mi_cmd_thread_select () is > called, therefore the condition "inferior_ptid != previous_ptid" > there does not hold. > > To address this problem, we have to move notification logic up to > mi_cmd_execute () where --thread option is processed and notify > user selected contents observers there if context changes. > > However, this in itself breaks GDB/MI because it would cause context > notification to be sent on MI channel. This is because by the time > we notify, MI notification suppression is already restored (done in > mi_command::invoke(). Therefore we had to lift notification suppression > logic also up to mi_cmd_execute (). This change in made distinction > between mi_command::invoke() and mi_command::do_invoke() unnecessary > as all mi_command::invoke() did (after the change) was to call > do_invoke(). So this patches removes do_invoke() and moves the command > execution logic directly to invoke(). > > With this change, all gdb.mi tests pass, tested on x86_64-linux. > > Co-authored-by: Andrew Burgess <aburgess@redhat.com> > Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=20631 > --- > gdb/mi/mi-cmd-stack.c | 12 +--- > gdb/mi/mi-cmds.c | 18 +----- > gdb/mi/mi-cmds.h | 17 ++---- > gdb/mi/mi-main.c | 51 +++++++++++++--- > gdb/python/py-micmd.c | 5 +- > gdb/stack.c | 8 --- > gdb/stack.h | 6 -- > .../gdb.mi/user-selected-context-sync.exp | 58 +++++++++++++++++-- > 8 files changed, 106 insertions(+), 69 deletions(-) Thanks this looks great. Please go ahead and push this. Thanks, Andrew > > diff --git a/gdb/mi/mi-cmd-stack.c b/gdb/mi/mi-cmd-stack.c > index 1be8aa81c3d..e894411765a 100644 > --- a/gdb/mi/mi-cmd-stack.c > +++ b/gdb/mi/mi-cmd-stack.c > @@ -757,17 +757,7 @@ 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); > - } > + select_frame (parse_frame_specification (argv[0])); > } > > void > diff --git a/gdb/mi/mi-cmds.c b/gdb/mi/mi-cmds.c > index 38fbe0d8a32..60fec0a0b85 100644 > --- a/gdb/mi/mi-cmds.c > +++ b/gdb/mi/mi-cmds.c > @@ -45,11 +45,9 @@ struct mi_command_mi : public mi_command > gdb_assert (func != nullptr); > } > > -protected: > - > /* Called when this MI command has been invoked, calls m_argv_function > with arguments contained within PARSE. */ > - void do_invoke (struct mi_parse *parse) const override > + void invoke (struct mi_parse *parse) const override > { > mi_parse_argv (parse->args, parse); > > @@ -83,13 +81,11 @@ struct mi_command_cli : public mi_command > m_args_p (args_p) > { /* Nothing. */ } > > -protected: > - > /* Called when this MI command has been invoked, calls the m_cli_name > CLI function. In m_args_p is true then the argument string from > within PARSE is passed through to the CLI function, otherwise nullptr > is passed through to the CLI function as its argument string. */ > - void do_invoke (struct mi_parse *parse) const override > + void invoke (struct mi_parse *parse) const override > { > const char *args = m_args_p ? parse->args : nullptr; > mi_execute_cli_command (m_cli_name, m_args_p, args); > @@ -173,16 +169,6 @@ mi_command::mi_command (const char *name, int *suppress_notification) > > /* See mi-cmds.h. */ > > -void > -mi_command::invoke (struct mi_parse *parse) const > -{ > - gdb::optional<scoped_restore_tmpl<int>> restore > - = do_suppress_notification (); > - this->do_invoke (parse); > -} > - > -/* See mi-cmds.h. */ > - > gdb::optional<scoped_restore_tmpl<int>> > mi_command::do_suppress_notification () const > { > diff --git a/gdb/mi/mi-cmds.h b/gdb/mi/mi-cmds.h > index 47b90a26064..05b702f83ec 100644 > --- a/gdb/mi/mi-cmds.h > +++ b/gdb/mi/mi-cmds.h > @@ -160,9 +160,10 @@ struct mi_command > const char *name () const > { return m_name; } > > - /* Execute the MI command. Can throw an exception if something goes > - wrong. */ > - void invoke (struct mi_parse *parse) const; > + /* Execute the MI command. this needs to be overridden in each > + base class. PARSE is the parsed command line from the user. > + Can throw an exception if something goes wrong. */ > + virtual void invoke (struct mi_parse *parse) const = 0; > > /* Return whether this command preserves user selected context (thread > and frame). */ > @@ -175,14 +176,6 @@ struct mi_command > return m_suppress_notification != &mi_suppress_notification.user_selected_context; > } > > -protected: > - > - /* The core of command invocation, this needs to be overridden in each > - base class. PARSE is the parsed command line from the user. */ > - virtual void do_invoke (struct mi_parse *parse) const = 0; > - > -private: > - > /* If this command was created with a suppress notifications pointer, > then this function will set the suppress flag and return a > gdb::optional with its value set to an object that will restore the > @@ -192,6 +185,8 @@ struct mi_command > then this function returns an empty gdb::optional. */ > gdb::optional<scoped_restore_tmpl<int>> do_suppress_notification () const; > > +private: > + > /* The name of the command. */ > const char *m_name; > > diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c > index 73380f5e668..abd033b22ae 100644 > --- a/gdb/mi/mi-main.c > +++ b/gdb/mi/mi-main.c > @@ -556,19 +556,10 @@ mi_cmd_thread_select (const char *command, char **argv, int argc) > if (thr == NULL) > error (_("Thread ID %d not known."), num); > > - ptid_t previous_ptid = inferior_ptid; > - > thread_select (argv[0], thr); > > print_selected_thread_frame (current_uiout, > USER_SELECTED_THREAD | USER_SELECTED_FRAME); > - > - /* 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 > @@ -1975,6 +1966,37 @@ mi_execute_command (const char *cmd, int from_tty) > } > } > > +/* Captures the current user selected context state, that is the current > + thread and frame. Later we can then check if the user selected context > + has changed at all. */ > + > +struct user_selected_context > +{ > + /* Constructor. */ > + user_selected_context () > + : m_previous_ptid (inferior_ptid), > + m_previous_frame (deprecated_safe_get_selected_frame ()) > + { /* Nothing. */ } > + > + /* Return true if the user selected context has changed since this object > + was created. */ > + bool has_changed () const > + { > + return ((m_previous_ptid != null_ptid > + && inferior_ptid != null_ptid > + && m_previous_ptid != inferior_ptid) > + || m_previous_frame != deprecated_safe_get_selected_frame ()); > + } > +private: > + /* The previously selected thread. This might be null_ptid if there was > + no previously selected thread. */ > + ptid_t m_previous_ptid; > + > + /* The previously selected frame. This might be nullptr if there was no > + previously selected frame. */ > + frame_info *m_previous_frame; > +}; > + > static void > mi_cmd_execute (struct mi_parse *parse) > { > @@ -2015,6 +2037,8 @@ mi_cmd_execute (struct mi_parse *parse) > set_current_program_space (inf->pspace); > } > > + user_selected_context current_user_selected_context; > + > gdb::optional<scoped_restore_current_thread> thread_saver; > if (parse->thread != -1) > { > @@ -2060,7 +2084,16 @@ mi_cmd_execute (struct mi_parse *parse) > current_context = parse; > > gdb_assert (parse->cmd != nullptr); > + > + gdb::optional<scoped_restore_tmpl<int>> restore_suppress_notification > + = parse->cmd->do_suppress_notification (); > + > parse->cmd->invoke (parse); > + > + if (!parse->cmd->preserve_user_selected_context () > + && current_user_selected_context.has_changed ()) > + gdb::observers::user_selected_context_changed.notify > + (USER_SELECTED_THREAD | USER_SELECTED_FRAME); > } > > /* See mi-main.h. */ > diff --git a/gdb/python/py-micmd.c b/gdb/python/py-micmd.c > index 4665fcc75c7..399e89b0298 100644 > --- a/gdb/python/py-micmd.c > +++ b/gdb/python/py-micmd.c > @@ -134,9 +134,8 @@ struct mi_command_py : public mi_command > m_pyobj = new_pyobj; > } > > -protected: > /* Called when the MI command is invoked. */ > - virtual void do_invoke(struct mi_parse *parse) const override; > + virtual void invoke(struct mi_parse *parse) const override; > > private: > /* The Python object representing this MI command. */ > @@ -311,7 +310,7 @@ serialize_mi_result (PyObject *result) > command line arguments from the user. */ > > void > -mi_command_py::do_invoke (struct mi_parse *parse) const > +mi_command_py::invoke (struct mi_parse *parse) const > { > PYMICMD_SCOPED_DEBUG_ENTER_EXIT; > > diff --git a/gdb/stack.c b/gdb/stack.c > index 8855ed8fe70..10da88b88e5 100644 > --- a/gdb/stack.c > +++ b/gdb/stack.c > @@ -1841,14 +1841,6 @@ select_frame_command_core (struct frame_info *fi, bool ignored) > gdb::observers::user_selected_context_changed.notify (USER_SELECTED_FRAME); > } > > -/* See stack.h. */ > - > -void > -select_frame_for_mi (struct frame_info *fi) > -{ > - select_frame_command_core (fi, false /* Ignored. */); > -} > - > /* The core of all the "frame" sub-commands. Select frame FI, and if this > means we change frame send out a change notification (otherwise, just > reprint the current frame summary). */ > diff --git a/gdb/stack.h b/gdb/stack.h > index c49d2e2cbef..f78aedf1c85 100644 > --- a/gdb/stack.h > +++ b/gdb/stack.h > @@ -20,12 +20,6 @@ > #ifndef STACK_H > #define STACK_H > > -/* Access method used by the MI -stack-select-frame command to switch to > - frame FI. This differs from SELECT_FRAME in that the observers for a > - user selected context change will be triggered. */ > - > -void select_frame_for_mi (struct frame_info *fi); > - > gdb::unique_xmalloc_ptr<char> find_frame_funname (struct frame_info *frame, > enum language *funlang, > struct symbol **funcp); > diff --git a/gdb/testsuite/gdb.mi/user-selected-context-sync.exp b/gdb/testsuite/gdb.mi/user-selected-context-sync.exp > index 3b4ff0bf2e4..9444ca5acf4 100644 > --- a/gdb/testsuite/gdb.mi/user-selected-context-sync.exp > +++ b/gdb/testsuite/gdb.mi/user-selected-context-sync.exp > @@ -916,7 +916,7 @@ proc_with_prefix test_mi_thread_select { mode } { > } > } > > - with_test_prefix "thread 1.2 with --thread" { > + with_test_prefix "thread 1.2 with --thread 2" { > # Test selecting a thread from MI with a --thread option. This test > # verifies that even if the thread GDB would switch to is the same as > # the thread specified with --thread, an event is still sent to CLI. > @@ -930,10 +930,28 @@ proc_with_prefix test_mi_thread_select { mode } { > } > > with_spawn_id $gdb_main_spawn_id { > - # This doesn't work as of now, no event is sent on CLI. It is > - # commented out so we don't have to wait for the timeout every time. > - # match_re_or_ensure_no_output "$cli_re\r\n" "-thread-select, event on cli" > - kfail "gdb/20631" "thread-select, event on cli" > + match_re_or_ensure_no_output "$cli_re\r\n" "-thread-select, event on cli" > + } > + } > + > + with_test_prefix "thread 1.2 with --thread 3" { > + # Test selecting a thread from MI with a --thread option. > + # This test verifies that when different thread numbers are > + # passed to the --thread option and the underlying > + # -thread-select command, the correct thread is selected. > + # In this case this is thread 1.2 > + > + reset_selection "1.1" > + > + set mi_re [make_mi_re $mode 2 0 response] > + set cli_re [make_cli_re $mode -1 1.2 0] > + > + with_spawn_id $mi_spawn_id { > + mi_gdb_test "-thread-select --thread 3 2" $mi_re "-thread-select" > + } > + > + with_spawn_id $gdb_main_spawn_id { > + match_re_or_ensure_no_output "$cli_re\r\n" "-thread-select, event on cli" > } > } > > @@ -974,6 +992,36 @@ proc_with_prefix test_mi_stack_select_frame { mode } { > with_spawn_id $gdb_main_spawn_id { > match_re_or_ensure_no_output $cli_re "-stack-select-frame again, event on CLI" > } > + > + # Now use the '-stack-select-frame' command with the --frame > + # option, this verifies that even when the frame GDB would > + # swith to is the same as the frame specified with --frame, an > + # event is still sent to the CLI. > + > + set cli_re [make_cli_re $mode -1 -1 0] > + > + with_spawn_id $mi_spawn_id { > + mi_gdb_test "-stack-select-frame --thread 2 --frame 0 0" $mi_re > + } > + > + with_spawn_id $gdb_main_spawn_id { > + match_re_or_ensure_no_output "$cli_re\r\n" "-stack-select-frame with --frame 0, event on CLI" > + } > + > + # Now use the '-stack-select-frame' command with the --frame > + # option, this verifies that the correct event is sent to the > + # CLI when the frame specified with --frame is different to > + # the actual frame selected. > + > + set cli_re [make_cli_re $mode -1 -1 1] > + > + with_spawn_id $mi_spawn_id { > + mi_gdb_test "-stack-select-frame --thread 2 --frame 2 1" $mi_re > + } > + > + with_spawn_id $gdb_main_spawn_id { > + match_re_or_ensure_no_output "$cli_re\r\n" "-stack-select-frame with --frame 2, event on CLI" > + } > } > > with_test_prefix "thread 1.3" { > -- > 2.35.1 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-03-16 14:35 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-03-14 11:57 [RFC PATCH] gdb/mi: consistently notify user when GDB/MI client uses -thread-select Jan Vrany 2022-03-15 17:10 ` Andrew Burgess 2022-03-16 14:03 ` [PATCH v2] " Jan Vrany 2022-03-16 14:35 ` Andrew Burgess
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).