* [PATCH 0/1] PR20684, preserve user selected context when invoking MI commands
@ 2022-01-25 14:51 Jan Vrany via Gdb-patches
2022-01-25 14:51 ` [PATCH 1/1] gdb/mi: PR20684, preserve user selected thread and frame " Jan Vrany via Gdb-patches
0 siblings, 1 reply; 17+ messages in thread
From: Jan Vrany via Gdb-patches @ 2022-01-25 14:51 UTC (permalink / raw)
To: gdb-patches; +Cc: Jan Vrany
This is a next iteration of patch posted earlier [1].
Changes since previous patch:
* updates to accomodate new C++ification of MI commands pushed in
December last year.
* updates to handle cases pointed out by Simon [2]
* add testcase
[1]: https://sourceware.org/pipermail/gdb-patches/2021-February/175897.html
[2]: https://sourceware.org/pipermail/gdb-patches/2021-March/177242.html
Jan Vrany (1):
gdb/mi: PR20684, preserve user selected thread and frame when invoking
MI commands
gdb/mi/mi-cmds.h | 12 ++++
gdb/mi/mi-main.c | 13 +++-
gdb/testsuite/gdb.mi/pr20684.exp | 117 +++++++++++++++++++++++++++++++
3 files changed, 140 insertions(+), 2 deletions(-)
create mode 100644 gdb/testsuite/gdb.mi/pr20684.exp
--
2.30.2
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 1/1] gdb/mi: PR20684, preserve user selected thread and frame when invoking MI commands
2022-01-25 14:51 [PATCH 0/1] PR20684, preserve user selected context when invoking MI commands Jan Vrany via Gdb-patches
@ 2022-01-25 14:51 ` Jan Vrany via Gdb-patches
2022-01-25 18:59 ` Simon Marchi via Gdb-patches
0 siblings, 1 reply; 17+ messages in thread
From: Jan Vrany via Gdb-patches @ 2022-01-25 14:51 UTC (permalink / raw)
To: gdb-patches; +Cc: Jan Vrany
When invoking MI commands with --thread and/or --frame, user selected
thread and frame was not preserved:
(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 was problematic for frontends that provide access to CLI because UI
may silently change the context for CLI commands (as demonstrated above).
---
gdb/mi/mi-cmds.h | 12 ++++
gdb/mi/mi-main.c | 13 +++-
gdb/testsuite/gdb.mi/pr20684.exp | 117 +++++++++++++++++++++++++++++++
3 files changed, 140 insertions(+), 2 deletions(-)
create mode 100644 gdb/testsuite/gdb.mi/pr20684.exp
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..b25e87a1476 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -2037,6 +2037,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 +2048,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 +2062,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/pr20684.exp b/gdb/testsuite/gdb.mi/pr20684.exp
new file mode 100644
index 00000000000..7d682146fc4
--- /dev/null
+++ b/gdb/testsuite/gdb.mi/pr20684.exp
@@ -0,0 +1,117 @@
+# 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/>.
+
+load_lib mi-support.exp
+
+standard_testfile user-selected-context-sync.c
+
+set compile_options "debug pthreads"
+if {[build_executable $testfile.exp $testfile ${srcfile} ${compile_options}] == -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"
+
+#(gdb)
+#info thread
+#&"info thread\n"
+#~" Id Target Id Frame \n"
+#~"* 1 Thread 0x7ffff7dbe740 (LWP 411075) \"pr20684\" main () at /home/jv/Projects/gdb/users_jv_patches/gdb/testsuite/gdb.mi/user-selected-context-sync.c:60\n"
+#~" 2 Thread 0x7ffff7dbd700 (LWP 411079) \"pr20684\" child_sub_function () at /home/jv/Projects/gdb/users_jv_patches/gdb/testsuite/gdb.mi/user-selected-context-sync.c:30\n"
+#~" 3 Thread 0x7ffff75bc700 (LWP 411080) \"pr20684\" futex_wait (private=0, expected=0, futex_word=0x555555558084 <barrier+4>) at ../sysdeps/nptl/futex-internal.h:144\n"
+#^done
+#(gdb)
+
+mi_gdb_test "info thread" \
+ [ join { ".*" \
+ "~\" Id Target Id.*\[\r\n\]" \
+ "~\"\\* 1 Thread 0x.*\[\r\n\]" \
+ "~\" 2 Thread 0x.*\[\r\n\]" \
+ "~\" 3 Thread 0x.*\[\r\n\]" \
+ "\\^done" } "" ] \
+ "info thread 1"
+
+#=========================
+
+mi_gdb_test "-stack-info-depth --thread 3" \
+ "\\^done,depth=.*" \
+ "-stack-info-depth --thread 3"
+
+mi_gdb_test "info thread" \
+ [ join { ".*" \
+ "~\" Id Target Id.*\[\r\n\]" \
+ "~\"\\* 1 Thread 0x.*\[\r\n\]" \
+ "~\" 2 Thread 0x.*\[\r\n\]" \
+ "~\" 3 Thread 0x.*\[\r\n\]" \
+ "\\^done" } "" ] \
+ "info thread 2"
+
+#=========================
+
+mi_gdb_test "-thread-select 3" \
+ "\\^done,.*" \
+ "-thread-select 3"
+
+mi_gdb_test "info thread" \
+ [ join { ".*" \
+ "~\" Id Target Id.*\[\r\n\]" \
+ "~\" 1 Thread 0x.*\[\r\n\]" \
+ "~\" 2 Thread 0x.*\[\r\n\]" \
+ "~\"\\* 3 Thread 0x.*\[\r\n\]" \
+ "\\^done" } "" ] \
+ "info thread 3"
+
+#=========================
+
+mi_gdb_test "-thread-select --thread 2 1" \
+ "\\^done,.*" \
+ "-thread-select --thread 2 1"
+
+mi_gdb_test "info thread" \
+ [ join { ".*" \
+ "~\" Id Target Id.*\[\r\n\]" \
+ "~\"\\* 1 Thread 0x.*\[\r\n\]" \
+ "~\" 2 Thread 0x.*\[\r\n\]" \
+ "~\" 3 Thread 0x.*\[\r\n\]" \
+ "\\^done" } "" ] \
+ "info thread 4"
+
+#=========================
+
+mi_gdb_test "-thread-select --thread 2 2" \
+ "\\^done,.*" \
+ "-thread-select --thread 2 2"
+
+mi_gdb_test "info thread" \
+ [ join { ".*" \
+ "~\" Id Target Id.*\[\r\n\]" \
+ "~\" 1 Thread 0x.*\[\r\n\]" \
+ "~\"\\* 2 Thread 0x.*\[\r\n\]" \
+ "~\" 3 Thread 0x.*\[\r\n\]" \
+ "\\^done" } "" ] \
+ "info thread 5"
+
+
+
--
2.30.2
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/1] gdb/mi: PR20684, preserve user selected thread and frame when invoking MI commands
2022-01-25 14:51 ` [PATCH 1/1] gdb/mi: PR20684, preserve user selected thread and frame " Jan Vrany via Gdb-patches
@ 2022-01-25 18:59 ` Simon Marchi via Gdb-patches
2022-01-26 13:21 ` [PATCH v2 0/1] PR20684, preserve user selected context " Jan Vrany via Gdb-patches
2022-01-26 13:21 ` [PATCH v2 1/1] gdb/mi: PR20684, preserve user selected thread and frame " Jan Vrany via Gdb-patches
0 siblings, 2 replies; 17+ messages in thread
From: Simon Marchi via Gdb-patches @ 2022-01-25 18:59 UTC (permalink / raw)
To: Jan Vrany, gdb-patches
On 2022-01-25 09:51, Jan Vrany via Gdb-patches wrote:
> When invoking MI commands with --thread and/or --frame, user selected
> thread and frame was not preserved:
>
> (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 was problematic for frontends that provide access to CLI because UI
> may silently change the context for CLI commands (as demonstrated above).
Please add the trailer:
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=20684
I got these warnings while applying, please fix:
Applying: gdb/mi: PR20684, preserve user selected thread and frame when invoking MI commands
.git/rebase-apply/patch:144: space before tab in indent.
"-stack-info-depth --thread 3"
.git/rebase-apply/patch:159: space before tab in indent.
"-thread-select 3"
.git/rebase-apply/patch:174: space before tab in indent.
"-thread-select --thread 2 1"
.git/rebase-apply/patch:189: space before tab in indent.
"-thread-select --thread 2 2"
.git/rebase-apply/patch:199: new blank line at EOF.
+
> @@ -2057,8 +2062,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/pr20684.exp b/gdb/testsuite/gdb.mi/pr20684.exp
> new file mode 100644
> index 00000000000..7d682146fc4
> --- /dev/null
> +++ b/gdb/testsuite/gdb.mi/pr20684.exp
Could you please give this file a descriptive name, instead of the PR
number?
> @@ -0,0 +1,117 @@
> +# Copyright 2022 Free Software Foundation, Inc.
Spurious tab (maybe one of the locations that git pointed out).
> +
> +# 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/>.
Please add a short comment explaining what the test does, what it
intends to test. I'm trying to push to have those on every new test, so
that when you open up a test file, you can understand quickly what it's
about.
> +
> +load_lib mi-support.exp
> +
> +standard_testfile user-selected-context-sync.c
> +
> +set compile_options "debug pthreads"
> +if {[build_executable $testfile.exp $testfile ${srcfile} ${compile_options}] == -1} {
You can inline "debug pthreads" directly in the proc call.
> + 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"
> +
> +#(gdb)
> +#info thread
> +#&"info thread\n"
> +#~" Id Target Id Frame \n"
> +#~"* 1 Thread 0x7ffff7dbe740 (LWP 411075) \"pr20684\" main () at /home/jv/Projects/gdb/users_jv_patches/gdb/testsuite/gdb.mi/user-selected-context-sync.c:60\n"
> +#~" 2 Thread 0x7ffff7dbd700 (LWP 411079) \"pr20684\" child_sub_function () at /home/jv/Projects/gdb/users_jv_patches/gdb/testsuite/gdb.mi/user-selected-context-sync.c:30\n"
> +#~" 3 Thread 0x7ffff75bc700 (LWP 411080) \"pr20684\" futex_wait (private=0, expected=0, futex_word=0x555555558084 <barrier+4>) at ../sysdeps/nptl/futex-internal.h:144\n"
> +#^done
> +#(gdb)
> +
> +mi_gdb_test "info thread" \
> + [ join { ".*" \
> + "~\" Id Target Id.*\[\r\n\]" \
> + "~\"\\* 1 Thread 0x.*\[\r\n\]" \
> + "~\" 2 Thread 0x.*\[\r\n\]" \
> + "~\" 3 Thread 0x.*\[\r\n\]" \
> + "\\^done" } "" ] \
> + "info thread 1"
I think that would be a good use case for the multi_line proc (defined
in lib/gdb.exp)?
Otherwise, if the sole purpose of "info threads" is to check what is the
current thread, you could perhaps use the "thread" command insstead, and
just look for "Current thread is N".
Simon
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 0/1] PR20684, preserve user selected context when invoking MI commands
2022-01-25 18:59 ` Simon Marchi via Gdb-patches
@ 2022-01-26 13:21 ` Jan Vrany via Gdb-patches
2022-01-26 13:21 ` [PATCH v2 1/1] gdb/mi: PR20684, preserve user selected thread and frame " Jan Vrany via Gdb-patches
1 sibling, 0 replies; 17+ messages in thread
From: Jan Vrany via Gdb-patches @ 2022-01-26 13:21 UTC (permalink / raw)
To: gdb-patches; +Cc: Jan Vrany
This is a next version of the patch addressing Simon's comments
Changes since previous iteration [1]:
* updates to accomodate new C++ification of MI commands pushed in
December last year.
* updates to handle cases pointed out by Simon
* add testcase
Changes since v1:
* address Simon's comments [2]
* add more tests for -stack-select-frame
[1]: https://sourceware.org/pipermail/gdb-patches/2021-February/175897.html
[2]: https://sourceware.org/pipermail/gdb-patches/2022-January/185426.html
Jan Vrany (1):
gdb/mi: PR20684, preserve user selected thread and frame when invoking
MI commands
gdb/mi/mi-cmds.h | 12 ++
gdb/mi/mi-main.c | 13 +-
gdb/testsuite/gdb.mi/mi-cmd-user-context.exp | 155 +++++++++++++++++++
3 files changed, 178 insertions(+), 2 deletions(-)
create mode 100644 gdb/testsuite/gdb.mi/mi-cmd-user-context.exp
--
2.30.2
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 1/1] gdb/mi: PR20684, preserve user selected thread and frame when invoking MI commands
2022-01-25 18:59 ` Simon Marchi via Gdb-patches
2022-01-26 13:21 ` [PATCH v2 0/1] PR20684, preserve user selected context " Jan Vrany via Gdb-patches
@ 2022-01-26 13:21 ` Jan Vrany via Gdb-patches
2022-01-26 15:01 ` Simon Marchi via Gdb-patches
1 sibling, 1 reply; 17+ messages in thread
From: Jan Vrany via Gdb-patches @ 2022-01-26 13:21 UTC (permalink / raw)
To: gdb-patches; +Cc: Jan Vrany
When invoking MI commands with --thread and/or --frame, user selected
thread and frame was not preserved:
(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 was problematic for frontends that provide access to CLI because UI
may silently change the context for CLI commands (as demonstrated above).
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=20684
---
gdb/mi/mi-cmds.h | 12 ++
gdb/mi/mi-main.c | 13 +-
gdb/testsuite/gdb.mi/mi-cmd-user-context.exp | 155 +++++++++++++++++++
3 files changed, 178 insertions(+), 2 deletions(-)
create mode 100644 gdb/testsuite/gdb.mi/mi-cmd-user-context.exp
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..b25e87a1476 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -2037,6 +2037,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 +2048,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 +2062,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.30.2
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/1] gdb/mi: PR20684, preserve user selected thread and frame when invoking MI commands
2022-01-26 13:21 ` [PATCH v2 1/1] gdb/mi: PR20684, preserve user selected thread and frame " Jan Vrany via Gdb-patches
@ 2022-01-26 15:01 ` Simon Marchi via Gdb-patches
2022-01-26 17:08 ` Jan Vrany via Gdb-patches
` (2 more replies)
0 siblings, 3 replies; 17+ messages in thread
From: Simon Marchi via Gdb-patches @ 2022-01-26 15:01 UTC (permalink / raw)
To: Jan Vrany, gdb-patches
Thanks, this is OK.
Simon
On 2022-01-26 08:21, Jan Vrany via Gdb-patches wrote:
> When invoking MI commands with --thread and/or --frame, user selected
> thread and frame was not preserved:
>
> (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 was problematic for frontends that provide access to CLI because UI
> may silently change the context for CLI commands (as demonstrated above).
>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=20684
> ---
> gdb/mi/mi-cmds.h | 12 ++
> gdb/mi/mi-main.c | 13 +-
> gdb/testsuite/gdb.mi/mi-cmd-user-context.exp | 155 +++++++++++++++++++
> 3 files changed, 178 insertions(+), 2 deletions(-)
> create mode 100644 gdb/testsuite/gdb.mi/mi-cmd-user-context.exp
>
> 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..b25e87a1476 100644
> --- a/gdb/mi/mi-main.c
> +++ b/gdb/mi/mi-main.c
> @@ -2037,6 +2037,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 +2048,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 +2062,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"
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/1] gdb/mi: PR20684, preserve user selected thread and frame when invoking MI commands
2022-01-26 15:01 ` Simon Marchi via Gdb-patches
@ 2022-01-26 17:08 ` Jan Vrany via Gdb-patches
2022-01-27 14:50 ` [PATCH v3 0/1] PR20684, preserve user selected context " Jan Vrany via Gdb-patches
2022-01-27 14:50 ` [PATCH v3 1/1] gdb/mi: PR20684, preserve user selected thread and frame " Jan Vrany via Gdb-patches
2 siblings, 0 replies; 17+ messages in thread
From: Jan Vrany via Gdb-patches @ 2022-01-26 17:08 UTC (permalink / raw)
To: Simon Marchi, gdb-patches
Hi,
On Wed, 2022-01-26 at 10:01 -0500, Simon Marchi wrote:
> Thanks, this is OK.
Unfortunately, it is not. I have found a regression when doing final
pre-push testing. I think I have a tentative fix, but needs more
testing. I'll post v3 of this patch tomorrow.
Sorry about that!
Jan
>
> Simon
>
> On 2022-01-26 08:21, Jan Vrany via Gdb-patches wrote:
> > When invoking MI commands with --thread and/or --frame, user selected
> > thread and frame was not preserved:
> >
> > (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 was problematic for frontends that provide access to CLI because UI
> > may silently change the context for CLI commands (as demonstrated above).
> >
> > Bug: https://urldefense.proofpoint.com/v2/url?u=https-3A__sourceware.org_bugzilla_show-5Fbug.cgi-3Fid-3D20684&d=DwICaQ&c=sPZ6DeHLiehUHQWKIrsNwWp3t7snrE-az24ztT0w7Jc&r=WpFFGgYa98Yp-c29WHTCwU1wAGFBvszA6a4RzgpMSqc&m=YXXmP3UHATiIQ8jPJrNxkE33ypHZ5NAYKMAvSdtuERM&s=wWlBE5Q8xGagnhvV57p8mRde5PHtg__CrplJVjeVGIU&e=
> > ---
> > gdb/mi/mi-cmds.h | 12 ++
> > gdb/mi/mi-main.c | 13 +-
> > gdb/testsuite/gdb.mi/mi-cmd-user-context.exp | 155 +++++++++++++++++++
> > 3 files changed, 178 insertions(+), 2 deletions(-)
> > create mode 100644 gdb/testsuite/gdb.mi/mi-cmd-user-context.exp
> >
> > 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..b25e87a1476 100644
> > --- a/gdb/mi/mi-main.c
> > +++ b/gdb/mi/mi-main.c
> > @@ -2037,6 +2037,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 +2048,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 +2062,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 <https://urldefense.proofpoint.com/v2/url?u=http-3A__www.gnu.org_licenses_&d=DwICaQ&c=sPZ6DeHLiehUHQWKIrsNwWp3t7snrE-az24ztT0w7Jc&r=WpFFGgYa98Yp-c29WHTCwU1wAGFBvszA6a4RzgpMSqc&m=YXXmP3UHATiIQ8jPJrNxkE33ypHZ5NAYKMAvSdtuERM&s=e40OHXPdnSaDFlYkl3p8U6MtVrs_pIOj7_bbg84qYT4&e= >.
> > +
> > +# 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"
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v3 0/1] PR20684, preserve user selected context when invoking MI commands
2022-01-26 15:01 ` Simon Marchi via Gdb-patches
2022-01-26 17:08 ` Jan Vrany via Gdb-patches
@ 2022-01-27 14:50 ` Jan Vrany via Gdb-patches
2022-02-07 11:56 ` Jan Vrany via Gdb-patches
2022-01-27 14:50 ` [PATCH v3 1/1] gdb/mi: PR20684, preserve user selected thread and frame " Jan Vrany via Gdb-patches
2 siblings, 1 reply; 17+ messages in thread
From: Jan Vrany via Gdb-patches @ 2022-01-27 14:50 UTC (permalink / raw)
To: gdb-patches; +Cc: Jan Vrany
This is a next version of the patch fixing some regression
Changes since previous iteration [1]:
* updates to accomodate new C++ification of MI commands pushed in
December last year.
* updates to handle cases pointed out by Simon
* add testcase
Changes since v1:
* address Simon's comments [2]
* add more tests for -stack-select-frame
Changes since v2:
Fix regression in mi-nonstop.exp and mi-nsthrexec.exp. This
was caused by extra user selected thread notification being sent
when resuming a thread like:
-exec-continue --thread 2
The culprit was this part of mi_excecute_command ():
else if (inferior_ptid != null_ptid)
{
struct thread_info *ti = inferior_thread ();
report_change = (ti->global_num != command->thread);
}
This "else" is only executed when command specifies --thread.
Imagine currently selected thread is thread 1 and -exec-continue --thread 2
is issued.
BEFORE applying this patch, no change would be reported because mi_cmd_execute ()
would switch the thread to thread 2 so `ti->global_num != command->thread` would
be false.
AFTER appling this patch, mi_cmd_execute () would switch the thread *and then restore*
back so `ti->global_num != command->thread` would be true.
Therefore I removed this whole `else if` branch: since mi_cmd_execute () now restores
the thread if --thread is passed (except few limited commands that notify themselves),
this condition would hold each time thread passed to --thread is different than current
thread. Removing it preserves the behavior.
Removing that "else if" branch allowed me to merge two nested ifs into one to (hopefully)
make the code simpler.
Thanks!
Jan
[1]: https://sourceware.org/pipermail/gdb-patches/2021-February/175897.html
Jan Vrany (1):
gdb/mi: PR20684, preserve user selected thread and frame when invoking
MI commands
gdb/mi/mi-cmds.h | 12 ++
gdb/mi/mi-main.c | 54 ++++---
gdb/testsuite/gdb.mi/mi-cmd-user-context.exp | 155 +++++++++++++++++++
3 files changed, 198 insertions(+), 23 deletions(-)
create mode 100644 gdb/testsuite/gdb.mi/mi-cmd-user-context.exp
--
2.30.2
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v3 1/1] gdb/mi: PR20684, preserve user selected thread and frame when invoking MI commands
2022-01-26 15:01 ` Simon Marchi via Gdb-patches
2022-01-26 17:08 ` Jan Vrany via Gdb-patches
2022-01-27 14:50 ` [PATCH v3 0/1] PR20684, preserve user selected context " Jan Vrany via Gdb-patches
@ 2022-01-27 14:50 ` Jan Vrany via Gdb-patches
2022-02-17 22:24 ` Andrew Burgess via Gdb-patches
2 siblings, 1 reply; 17+ messages in thread
From: Jan Vrany via Gdb-patches @ 2022-01-27 14:50 UTC (permalink / raw)
To: gdb-patches; +Cc: Jan Vrany
When invoking MI commands with --thread and/or --frame, user selected
thread and frame was not preserved:
(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 was problematic for frontends that provide access to CLI because UI
may silently change the context for CLI commands (as demonstrated above).
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=20684
---
gdb/mi/mi-cmds.h | 12 ++
gdb/mi/mi-main.c | 54 ++++---
gdb/testsuite/gdb.mi/mi-cmd-user-context.exp | 155 +++++++++++++++++++
3 files changed, 198 insertions(+), 23 deletions(-)
create mode 100644 gdb/testsuite/gdb.mi/mi-cmd-user-context.exp
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..e112707e4d1 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -1907,6 +1907,16 @@ command_notifies_uscc_observer (struct mi_parse *command)
}
}
+/* 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;
+}
+
void
mi_execute_command (const char *cmd, int from_tty)
{
@@ -1971,28 +1981,17 @@ mi_execute_command (const char *cmd, int from_tty)
&& any_thread_p ()
/* If the command already reports the thread change, no need to do it
again. */
- && !command_notifies_uscc_observer (command.get ()))
+ && !command_notifies_uscc_observer (command.get ())
+ /* Don't report anything if --thread was specified -- the user selected
+ thread is preserved by mi_execute_command and therefore cannot
+ change. */
+ && command->thread == -1
+ /* Don't report anything if the selected thread actually did not change
+ (compared to selected thread before execution the command). */
+ && command_changed_user_selected_thread (previous_ptid, inferior_ptid))
{
- 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);
- }
+ gdb::observers::user_selected_context_changed.notify
+ (USER_SELECTED_THREAD | USER_SELECTED_FRAME);
}
}
}
@@ -2037,6 +2036,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 +2047,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 +2061,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.30.2
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 0/1] PR20684, preserve user selected context when invoking MI commands
2022-01-27 14:50 ` [PATCH v3 0/1] PR20684, preserve user selected context " Jan Vrany via Gdb-patches
@ 2022-02-07 11:56 ` Jan Vrany via Gdb-patches
2022-02-17 16:07 ` [PING] " Jan Vrany via Gdb-patches
0 siblings, 1 reply; 17+ messages in thread
From: Jan Vrany via Gdb-patches @ 2022-02-07 11:56 UTC (permalink / raw)
To: gdb-patches
Polite ping.
Jan
On Thu, 2022-01-27 at 14:50 +0000, Jan Vrany wrote:
> This is a next version of the patch fixing some regression
>
> Changes since previous iteration [1]:
>
> * updates to accomodate new C++ification of MI commands pushed in
> December last year.
> * updates to handle cases pointed out by Simon
> * add testcase
>
> Changes since v1:
>
> * address Simon's comments [2]
> * add more tests for -stack-select-frame
>
> Changes since v2:
>
> Fix regression in mi-nonstop.exp and mi-nsthrexec.exp. This
> was caused by extra user selected thread notification being sent
> when resuming a thread like:
>
> -exec-continue --thread 2
>
> The culprit was this part of mi_excecute_command ():
>
> else if (inferior_ptid != null_ptid)
> {
> struct thread_info *ti = inferior_thread ();
>
> report_change = (ti->global_num != command->thread);
> }
>
> This "else" is only executed when command specifies --thread.
>
> Imagine currently selected thread is thread 1 and -exec-continue --thread 2
> is issued.
>
> BEFORE applying this patch, no change would be reported because mi_cmd_execute ()
> would switch the thread to thread 2 so `ti->global_num != command->thread` would
> be false.
>
> AFTER appling this patch, mi_cmd_execute () would switch the thread *and then restore*
> back so `ti->global_num != command->thread` would be true.
>
> Therefore I removed this whole `else if` branch: since mi_cmd_execute () now restores
> the thread if --thread is passed (except few limited commands that notify themselves),
> this condition would hold each time thread passed to --thread is different than current
> thread. Removing it preserves the behavior.
>
> Removing that "else if" branch allowed me to merge two nested ifs into one to (hopefully)
> make the code simpler.
>
> Thanks!
>
> Jan
>
>
> [1]: https://sourceware.org/pipermail/gdb-patches/2021-February/175897.html
>
>
> Jan Vrany (1):
> gdb/mi: PR20684, preserve user selected thread and frame when invoking
> MI commands
>
> gdb/mi/mi-cmds.h | 12 ++
> gdb/mi/mi-main.c | 54 ++++---
> gdb/testsuite/gdb.mi/mi-cmd-user-context.exp | 155 +++++++++++++++++++
> 3 files changed, 198 insertions(+), 23 deletions(-)
> create mode 100644 gdb/testsuite/gdb.mi/mi-cmd-user-context.exp
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PING] [PATCH v3 0/1] PR20684, preserve user selected context when invoking MI commands
2022-02-07 11:56 ` Jan Vrany via Gdb-patches
@ 2022-02-17 16:07 ` Jan Vrany via Gdb-patches
0 siblings, 0 replies; 17+ messages in thread
From: Jan Vrany via Gdb-patches @ 2022-02-17 16:07 UTC (permalink / raw)
To: gdb-patches
Polite ping.
Jan
On Mon, 2022-02-07 at 11:56 +0000, Jan Vrany wrote:
> Polite ping.
>
> Jan
>
> On Thu, 2022-01-27 at 14:50 +0000, Jan Vrany wrote:
> > This is a next version of the patch fixing some regression
> >
> > Changes since previous iteration [1]:
> >
> > * updates to accomodate new C++ification of MI commands pushed in
> > December last year.
> > * updates to handle cases pointed out by Simon
> > * add testcase
> >
> > Changes since v1:
> >
> > * address Simon's comments [2]
> > * add more tests for -stack-select-frame
> >
> > Changes since v2:
> >
> > Fix regression in mi-nonstop.exp and mi-nsthrexec.exp. This
> > was caused by extra user selected thread notification being sent
> > when resuming a thread like:
> >
> > -exec-continue --thread 2
> >
> > The culprit was this part of mi_excecute_command ():
> >
> > else if (inferior_ptid != null_ptid)
> > {
> > struct thread_info *ti = inferior_thread ();
> >
> > report_change = (ti->global_num != command->thread);
> > }
> >
> > This "else" is only executed when command specifies --thread.
> >
> > Imagine currently selected thread is thread 1 and -exec-continue --thread 2
> > is issued.
> >
> > BEFORE applying this patch, no change would be reported because mi_cmd_execute ()
> > would switch the thread to thread 2 so `ti->global_num != command->thread` would
> > be false.
> >
> > AFTER appling this patch, mi_cmd_execute () would switch the thread *and then restore*
> > back so `ti->global_num != command->thread` would be true.
> >
> > Therefore I removed this whole `else if` branch: since mi_cmd_execute () now restores
> > the thread if --thread is passed (except few limited commands that notify themselves),
> > this condition would hold each time thread passed to --thread is different than current
> > thread. Removing it preserves the behavior.
> >
> > Removing that "else if" branch allowed me to merge two nested ifs into one to (hopefully)
> > make the code simpler.
> >
> > Thanks!
> >
> > Jan
> >
> >
> > [1]: https://sourceware.org/pipermail/gdb-patches/2021-February/175897.html
> >
> >
> > Jan Vrany (1):
> > gdb/mi: PR20684, preserve user selected thread and frame when invoking
> > MI commands
> >
> > gdb/mi/mi-cmds.h | 12 ++
> > gdb/mi/mi-main.c | 54 ++++---
> > gdb/testsuite/gdb.mi/mi-cmd-user-context.exp | 155 +++++++++++++++++++
> > 3 files changed, 198 insertions(+), 23 deletions(-)
> > create mode 100644 gdb/testsuite/gdb.mi/mi-cmd-user-context.exp
> >
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 1/1] gdb/mi: PR20684, preserve user selected thread and frame when invoking MI commands
2022-01-27 14:50 ` [PATCH v3 1/1] gdb/mi: PR20684, preserve user selected thread and frame " Jan Vrany via Gdb-patches
@ 2022-02-17 22:24 ` Andrew Burgess via Gdb-patches
2022-02-18 17:45 ` Jan Vrany via Gdb-patches
0 siblings, 1 reply; 17+ messages in thread
From: Andrew Burgess via Gdb-patches @ 2022-02-17 22:24 UTC (permalink / raw)
To: Jan Vrany; +Cc: gdb-patches
* Jan Vrany via Gdb-patches <gdb-patches@sourceware.org> [2022-01-27 14:50:11 +0000]:
> When invoking MI commands with --thread and/or --frame, user selected
> thread and frame was not preserved:
>
> (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 was problematic for frontends that provide access to CLI because UI
> may silently change the context for CLI commands (as demonstrated above).
>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=20684
> ---
> gdb/mi/mi-cmds.h | 12 ++
> gdb/mi/mi-main.c | 54 ++++---
> gdb/testsuite/gdb.mi/mi-cmd-user-context.exp | 155 +++++++++++++++++++
> 3 files changed, 198 insertions(+), 23 deletions(-)
> create mode 100644 gdb/testsuite/gdb.mi/mi-cmd-user-context.exp
>
> 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..e112707e4d1 100644
> --- a/gdb/mi/mi-main.c
> +++ b/gdb/mi/mi-main.c
> @@ -1907,6 +1907,16 @@ command_notifies_uscc_observer (struct mi_parse *command)
> }
> }
>
> +/* 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;
You need to wrap multi-line conditions like this inside parenthesis.
> +}
> +
> void
> mi_execute_command (const char *cmd, int from_tty)
> {
> @@ -1971,28 +1981,17 @@ mi_execute_command (const char *cmd, int from_tty)
> && any_thread_p ()
> /* If the command already reports the thread change, no need to do it
> again. */
> - && !command_notifies_uscc_observer (command.get ()))
> + && !command_notifies_uscc_observer (command.get ())
> + /* Don't report anything if --thread was specified -- the user selected
> + thread is preserved by mi_execute_command and therefore cannot
> + change. */
> + && command->thread == -1
I think including this check is a bug. Not a bug of your making I
think, as I believe the bug already existed. I'll explain...
If I start GDB, and then spawn a new-ui for the mi interpreter. Start
a multi-threaded inferior, and then stop at a gdb prompt.
Assuming thread 1 is selected, then, from the mi terminal I do:
-thread-select 2
I will get the '^done,new-thread-id="2"....' result on the mi
terminal, but on the cli terminal I'll also get some output telling me
that the thread changed. I think this is what you'd want, right? The
cli thread _did_ just change.
However, now that thread 2 is selected, if I do this from the mi
terminal:
-thread-select --thread 1 1
then I get nothing on the cli terminal. This feels like a bug to me,
and it's caused by the 'command->thread == -1' check above.
I think that check should just be dropped completely. Your scoped
thread/frame restore that you've added will ensure that, when
appropriate, the thread doesn't change. If it does change, then you
want to notify I think, regardless of whether --thread or --frame was
used.
It would be great if we could have a test for this added too. You'll
need to start gdb with a separate mi tty. You can look at
gdb.mi/mi-exec-run.exp for an example, look for passing the
'separate-mi-tty' string to mi_gdb_start, and then later for using
switch_gdb_spawn_id to switch between the main cli tty and the extra
mi tty. If you have any problem, I'll be happy to help.
You still have some white space bugs (tabs vs spaces at the start of
lines), I have this in my .gitconfig:
[core]
whitespace = space-before-tab,indent-with-non-tab,trailing-space
which will have git highlight anywhere I've failed to indent with a
tab when I should have done.
Additionally, some of your lines are over 80 characters, so need to be
wrapped.
Thanks,
Andrew
> + /* Don't report anything if the selected thread actually did not change
> + (compared to selected thread before execution the command). */
> + && command_changed_user_selected_thread (previous_ptid, inferior_ptid))
> {
> - 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);
> - }
> + gdb::observers::user_selected_context_changed.notify
> + (USER_SELECTED_THREAD | USER_SELECTED_FRAME);
> }
> }
> }
> @@ -2037,6 +2036,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 +2047,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 +2061,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.30.2
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 1/1] gdb/mi: PR20684, preserve user selected thread and frame when invoking MI commands
2022-02-17 22:24 ` Andrew Burgess via Gdb-patches
@ 2022-02-18 17:45 ` Jan Vrany via Gdb-patches
2022-03-01 11:59 ` [PING] " Jan Vrany via Gdb-patches
2022-03-02 10:00 ` Andrew Burgess via Gdb-patches
0 siblings, 2 replies; 17+ messages in thread
From: Jan Vrany via Gdb-patches @ 2022-02-18 17:45 UTC (permalink / raw)
To: Andrew Burgess; +Cc: GDB Patches
On Thu, 2022-02-17 at 22:24 +0000, Andrew Burgess wrote:
> * Jan Vrany via Gdb-patches <gdb-patches@sourceware.org> [2022-01-27 14:50:11 +0000]:
>
> > When invoking MI commands with --thread and/or --frame, user selected
> > thread and frame was not preserved:
> >
> > (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 was problematic for frontends that provide access to CLI because UI
> > may silently change the context for CLI commands (as demonstrated above).
> >
> > Bug: https://urldefense.proofpoint.com/v2/url?u=https-3A__sourceware.org_bugzilla_show-5Fbug.cgi-3Fid-3D20684&d=DwIBAg&c=sPZ6DeHLiehUHQWKIrsNwWp3t7snrE-az24ztT0w7Jc&r=WpFFGgYa98Yp-c29WHTCwU1wAGFBvszA6a4RzgpMSqc&m=soXzjKh4zT-FpxAe1wtdv7ka2r_jXNbrkEUR4XdUcOY&s=QkEM5YHXj7mpWI1y1z1AxRbosKxMhhfvfsMXmRnFoQc&e=
> > ---
> > gdb/mi/mi-cmds.h | 12 ++
> > gdb/mi/mi-main.c | 54 ++++---
> > gdb/testsuite/gdb.mi/mi-cmd-user-context.exp | 155 +++++++++++++++++++
> > 3 files changed, 198 insertions(+), 23 deletions(-)
> > create mode 100644 gdb/testsuite/gdb.mi/mi-cmd-user-context.exp
> >
> > 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..e112707e4d1 100644
> > --- a/gdb/mi/mi-main.c
> > +++ b/gdb/mi/mi-main.c
> > @@ -1907,6 +1907,16 @@ command_notifies_uscc_observer (struct mi_parse *command)
> > }
> > }
> >
> > +/* 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;
>
> You need to wrap multi-line conditions like this inside parenthesis.
>
Thanks!
> > +}
> > +
> > void
> > mi_execute_command (const char *cmd, int from_tty)
> > {
> > @@ -1971,28 +1981,17 @@ mi_execute_command (const char *cmd, int from_tty)
> > && any_thread_p ()
> > /* If the command already reports the thread change, no need to do it
> > again. */
> > - && !command_notifies_uscc_observer (command.get ()))
> > + && !command_notifies_uscc_observer (command.get ())
> > + /* Don't report anything if --thread was specified -- the user selected
> > + thread is preserved by mi_execute_command and therefore cannot
> > + change. */
> > + && command->thread == -1
>
> I think including this check is a bug. Not a bug of your making I
> think, as I believe the bug already existed. I'll explain...
>
> If I start GDB, and then spawn a new-ui for the mi interpreter. Start
> a multi-threaded inferior, and then stop at a gdb prompt.
>
> Assuming thread 1 is selected, then, from the mi terminal I do:
>
> -thread-select 2
>
> I will get the '^done,new-thread-id="2"....' result on the mi
> terminal, but on the cli terminal I'll also get some output telling me
> that the thread changed. I think this is what you'd want, right? The
> cli thread _did_ just change.
>
> However, now that thread 2 is selected, if I do this from the mi
> terminal:
>
> -thread-select --thread 1 1
>
> then I get nothing on the cli terminal. This feels like a bug to me,
Yes, this looks like a bug indeed. And I think we just opened
can of worms...
> and it's caused by the 'command->thread == -1' check above.
I do not think so. When the command is -thread-select, then
command_notifies_uscc_observer() return 1, therefore the whole
condition won't hold anyway, right?
>
> I think that check should just be dropped completely. Your scoped
> thread/frame restore that you've added will ensure that, when
> appropriate, the thread doesn't change. If it does change, then you
> want to notify I think, regardless of whether --thread or --frame was
> used.
If I do just drop "&& command->thread == -1" from the condition, it breaks
existing test "--stack-select-frame 3" in mi-cmd-user-context.exp since it
produces change notification the test does not expect. Clearly it should not
produce MI notification, whether it should "Switching to..." CLI output over MI
channel I'm not sure.
But:
Now I think we can get rid of the whole "if" and with that command_notifies_uscc_observer()
and command_changed_user_selected_thread(). There are only two MI commands
that can change context are -thread-select and -stack-select-frame, right?
-thread-select implementation ( in mi_cmd_thread_select () ) does notify observers
on its own and if we add similar logic to -stack-select-frame implementation
( in mi_cmd_stack_select_frame () ) we're fine. Makes sense?
I tried that (see the first patch below) and it seems to pass all MI tests
and the code seems to me much simpler.
Still, this change itself does not solve your original bug. That's because
thread is already switched when we enter mi_cmd_thread_select() so the
condition "inferior_ptid != previous_ptid" won't hold and so no notification
is printed.
I went ahead and tried to address this by lifting the logic up to mi_cmd_execute ()
where --thread option is handled - see the second patch below. With that patch,
all gdb.mi tests pass and your bug seems to be fixed. Still, the patch needs more
work (test + check it works with -stack-frame-select).
That being said, I'd be cautious. It took me quite a bit to make heads and tails
of this and still not sure I got it. My preference would be to fix the original
problem *WITHOUT* fixing "your" bug and once done, submit another patch fixing
"your" bug.
So, if you agree and the first part of my response about dropping the
whole "if" makes any sense, I'll properly resubmit it as v4.
Makes sense?
>
> It would be great if we could have a test for this added too. You'll
> need to start gdb with a separate mi tty. You can look at
> gdb.mi/mi-exec-run.exp for an example, look for passing the
> 'separate-mi-tty' string to mi_gdb_start, and then later for using
> switch_gdb_spawn_id to switch between the main cli tty and the extra
> mi tty. If you have any problem, I'll be happy to help.
>
> You still have some white space bugs (tabs vs spaces at the start of
> lines), I have this in my .gitconfig:
>
> [core]
> whitespace = space-before-tab,indent-with-non-tab,trailing-space
>
> which will have git highlight anywhere I've failed to indent with a
> tab when I should have done.
>
> Additionally, some of your lines are over 80 characters, so need to be
> wrapped.
>
> Thanks,
> Andrew
>
>
> > + /* Don't report anything if the selected thread actually did not change
> > + (compared to selected thread before execution the command). */
> > + && command_changed_user_selected_thread (previous_ptid, inferior_ptid))
> > {
> > - 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);
> > - }
> > + gdb::observers::user_selected_context_changed.notify
> > + (USER_SELECTED_THREAD | USER_SELECTED_FRAME);
> > }
> > }
> > }
> > @@ -2037,6 +2036,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 +2047,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 +2061,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 <https://urldefense.proofpoint.com/v2/url?u=http-3A__www.gnu.org_licenses_&d=DwIBAg&c=sPZ6DeHLiehUHQWKIrsNwWp3t7snrE-az24ztT0w7Jc&r=WpFFGgYa98Yp-c29WHTCwU1wAGFBvszA6a4RzgpMSqc&m=soXzjKh4zT-FpxAe1wtdv7ka2r_jXNbrkEUR4XdUcOY&s=q6IZFROyYXzF9keb4NCS6EL93YiTxbx2HkwQEtqDr8k&e= >.
> > +
> > +# 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.30.2
> >
>
From db229be447707d57980e4c21f394cdba1ec86b4c Mon Sep 17 00:00:00 2001
From: Jan Vrany <jan.vrany@labware.com>
Date: Thu, 27 Jan 2022 13:45:09 +0000
Subject: [PATCH 1/2] gdb/mi: PR20684, preserve user selected thread and frame
when invoking MI commands
When invoking MI commands with --thread and/or --frame, user selected
thread and frame was not preserved:
(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 was problematic for frontends that provide access to CLI because UI
may silently change the context for CLI commands (as demonstrated above).
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
From 18d8dc95fe1a834ecbde319dd6e5960fd4d82a3f Mon Sep 17 00:00:00 2001
From: Jan Vrany <jan.vrany@labware.com>
Date: Fri, 18 Feb 2022 17:04:13 +0000
Subject: [PATCH 2/2] [WIP] gdb/mi: properly notify user when GDB/MI client
uses -thread-select
If we start GDB, and then spawn a new-ui for the mi interpreter. Start
a multi-threaded inferior, and then stop at a gdb prompt.
Assuming thread 1 is selected, then, from the mi terminal I do:
-thread-select 2
We will get the '^done,new-thread-id="2"....' result on the mi
terminal, but on the cli terminal I'll also get some output telling me
that the thread changed.
However, now that thread 2 is selected, if I do this from the mi
terminal:
-thread-select --thread 1 1
then I get nothing on the CLI terminal. This seems to be a bug.
The problem was that when `-thread-select --thread 1 1` 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 this 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:
* add 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 +++++++++++++++++++++++---------
4 files changed, 31 insertions(+), 29 deletions(-)
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 23e2373dcec..608b226f496 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
@@ -1936,6 +1927,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)
{
@@ -1976,6 +1977,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)
{
@@ -2021,7 +2024,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. */
--
2.34.1
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PING] Re: [PATCH v3 1/1] gdb/mi: PR20684, preserve user selected thread and frame when invoking MI commands
2022-02-18 17:45 ` Jan Vrany via Gdb-patches
@ 2022-03-01 11:59 ` Jan Vrany via Gdb-patches
2022-03-02 10:00 ` Andrew Burgess via Gdb-patches
1 sibling, 0 replies; 17+ messages in thread
From: Jan Vrany via Gdb-patches @ 2022-03-01 11:59 UTC (permalink / raw)
To: Andrew Burgess; +Cc: GDB Patches
On Fri, 2022-02-18 at 17:45 +0000, Jan Vrany wrote:
> On Thu, 2022-02-17 at 22:24 +0000, Andrew Burgess wrote:
> > * Jan Vrany via Gdb-patches <gdb-patches@sourceware.org> [2022-01-27 14:50:11 +0000]:
> >
> > > When invoking MI commands with --thread and/or --frame, user selected
> > > thread and frame was not preserved:
> > >
> > > (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 was problematic for frontends that provide access to CLI because UI
> > > may silently change the context for CLI commands (as demonstrated above).
> > >
> > > Bug: https://urldefense.proofpoint.com/v2/url?u=https-3A__sourceware.org_bugzilla_show-5Fbug.cgi-3Fid-3D20684&d=DwIBAg&c=sPZ6DeHLiehUHQWKIrsNwWp3t7snrE-az24ztT0w7Jc&r=WpFFGgYa98Yp-c29WHTCwU1wAGFBvszA6a4RzgpMSqc&m=soXzjKh4zT-FpxAe1wtdv7ka2r_jXNbrkEUR4XdUcOY&s=QkEM5YHXj7mpWI1y1z1AxRbosKxMhhfvfsMXmRnFoQc&e=
> > > ---
> > > gdb/mi/mi-cmds.h | 12 ++
> > > gdb/mi/mi-main.c | 54 ++++---
> > > gdb/testsuite/gdb.mi/mi-cmd-user-context.exp | 155 +++++++++++++++++++
> > > 3 files changed, 198 insertions(+), 23 deletions(-)
> > > create mode 100644 gdb/testsuite/gdb.mi/mi-cmd-user-context.exp
> > >
> > > 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..e112707e4d1 100644
> > > --- a/gdb/mi/mi-main.c
> > > +++ b/gdb/mi/mi-main.c
> > > @@ -1907,6 +1907,16 @@ command_notifies_uscc_observer (struct mi_parse *command)
> > > }
> > > }
> > >
> > > +/* 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;
> >
> > You need to wrap multi-line conditions like this inside parenthesis.
> >
>
> Thanks!
>
> > > +}
> > > +
> > > void
> > > mi_execute_command (const char *cmd, int from_tty)
> > > {
> > > @@ -1971,28 +1981,17 @@ mi_execute_command (const char *cmd, int from_tty)
> > > && any_thread_p ()
> > > /* If the command already reports the thread change, no need to do it
> > > again. */
> > > - && !command_notifies_uscc_observer (command.get ()))
> > > + && !command_notifies_uscc_observer (command.get ())
> > > + /* Don't report anything if --thread was specified -- the user selected
> > > + thread is preserved by mi_execute_command and therefore cannot
> > > + change. */
> > > + && command->thread == -1
> >
> > I think including this check is a bug. Not a bug of your making I
> > think, as I believe the bug already existed. I'll explain...
> >
> > If I start GDB, and then spawn a new-ui for the mi interpreter. Start
> > a multi-threaded inferior, and then stop at a gdb prompt.
> >
> > Assuming thread 1 is selected, then, from the mi terminal I do:
> >
> > -thread-select 2
> >
> > I will get the '^done,new-thread-id="2"....' result on the mi
> > terminal, but on the cli terminal I'll also get some output telling me
> > that the thread changed. I think this is what you'd want, right? The
> > cli thread _did_ just change.
> >
> > However, now that thread 2 is selected, if I do this from the mi
> > terminal:
> >
> > -thread-select --thread 1 1
> >
> > then I get nothing on the cli terminal. This feels like a bug to me,
>
> Yes, this looks like a bug indeed. And I think we just opened
> can of worms...
>
> > and it's caused by the 'command->thread == -1' check above.
>
> I do not think so. When the command is -thread-select, then
> command_notifies_uscc_observer() return 1, therefore the whole
> condition won't hold anyway, right?
>
> >
> > I think that check should just be dropped completely. Your scoped
> > thread/frame restore that you've added will ensure that, when
> > appropriate, the thread doesn't change. If it does change, then you
> > want to notify I think, regardless of whether --thread or --frame was
> > used.
>
> If I do just drop "&& command->thread == -1" from the condition, it breaks
> existing test "--stack-select-frame 3" in mi-cmd-user-context.exp since it
> produces change notification the test does not expect. Clearly it should not
> produce MI notification, whether it should "Switching to..." CLI output over MI
> channel I'm not sure.
>
> But:
>
> Now I think we can get rid of the whole "if" and with that command_notifies_uscc_observer()
> and command_changed_user_selected_thread(). There are only two MI commands
> that can change context are -thread-select and -stack-select-frame, right?
> -thread-select implementation ( in mi_cmd_thread_select () ) does notify observers
> on its own and if we add similar logic to -stack-select-frame implementation
> ( in mi_cmd_stack_select_frame () ) we're fine. Makes sense?
>
> I tried that (see the first patch below) and it seems to pass all MI tests
> and the code seems to me much simpler.
>
> Still, this change itself does not solve your original bug. That's because
> thread is already switched when we enter mi_cmd_thread_select() so the
> condition "inferior_ptid != previous_ptid" won't hold and so no notification
> is printed.
>
> I went ahead and tried to address this by lifting the logic up to mi_cmd_execute ()
> where --thread option is handled - see the second patch below. With that patch,
> all gdb.mi tests pass and your bug seems to be fixed. Still, the patch needs more
> work (test + check it works with -stack-frame-select).
>
> That being said, I'd be cautious. It took me quite a bit to make heads and tails
> of this and still not sure I got it. My preference would be to fix the original
> problem *WITHOUT* fixing "your" bug and once done, submit another patch fixing
> "your" bug.
>
> So, if you agree and the first part of my response about dropping the
> whole "if" makes any sense, I'll properly resubmit it as v4.
>
> Makes sense?
>
> >
> > It would be great if we could have a test for this added too. You'll
> > need to start gdb with a separate mi tty. You can look at
> > gdb.mi/mi-exec-run.exp for an example, look for passing the
> > 'separate-mi-tty' string to mi_gdb_start, and then later for using
> > switch_gdb_spawn_id to switch between the main cli tty and the extra
> > mi tty. If you have any problem, I'll be happy to help.
> >
> > You still have some white space bugs (tabs vs spaces at the start of
> > lines), I have this in my .gitconfig:
> >
> > [core]
> > whitespace = space-before-tab,indent-with-non-tab,trailing-space
> >
> > which will have git highlight anywhere I've failed to indent with a
> > tab when I should have done.
> >
> > Additionally, some of your lines are over 80 characters, so need to be
> > wrapped.
> >
> > Thanks,
> > Andrew
> >
> >
> > > + /* Don't report anything if the selected thread actually did not change
> > > + (compared to selected thread before execution the command). */
> > > + && command_changed_user_selected_thread (previous_ptid, inferior_ptid))
> > > {
> > > - 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);
> > > - }
> > > + gdb::observers::user_selected_context_changed.notify
> > > + (USER_SELECTED_THREAD | USER_SELECTED_FRAME);
> > > }
> > > }
> > > }
> > > @@ -2037,6 +2036,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 +2047,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 +2061,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 <https://urldefense.proofpoint.com/v2/url?u=http-3A__www.gnu.org_licenses_&d=DwIBAg&c=sPZ6DeHLiehUHQWKIrsNwWp3t7snrE-az24ztT0w7Jc&r=WpFFGgYa98Yp-c29WHTCwU1wAGFBvszA6a4RzgpMSqc&m=soXzjKh4zT-FpxAe1wtdv7ka2r_jXNbrkEUR4XdUcOY&s=q6IZFROyYXzF9keb4NCS6EL93YiTxbx2HkwQEtqDr8k&e= >.
> > > +
> > > +# 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.30.2
> > >
> >
>
> From db229be447707d57980e4c21f394cdba1ec86b4c Mon Sep 17 00:00:00 2001
> From: Jan Vrany <jan.vrany@labware.com>
> Date: Thu, 27 Jan 2022 13:45:09 +0000
> Subject: [PATCH 1/2] gdb/mi: PR20684, preserve user selected thread and frame
> when invoking MI commands
>
> When invoking MI commands with --thread and/or --frame, user selected
> thread and frame was not preserved:
>
> (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 was problematic for frontends that provide access to CLI because UI
> may silently change the context for CLI commands (as demonstrated above).
>
> 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
>
>
> From 18d8dc95fe1a834ecbde319dd6e5960fd4d82a3f Mon Sep 17 00:00:00 2001
> From: Jan Vrany <jan.vrany@labware.com>
> Date: Fri, 18 Feb 2022 17:04:13 +0000
> Subject: [PATCH 2/2] [WIP] gdb/mi: properly notify user when GDB/MI client
> uses -thread-select
>
> If we start GDB, and then spawn a new-ui for the mi interpreter. Start
> a multi-threaded inferior, and then stop at a gdb prompt.
>
> Assuming thread 1 is selected, then, from the mi terminal I do:
>
> -thread-select 2
>
> We will get the '^done,new-thread-id="2"....' result on the mi
> terminal, but on the cli terminal I'll also get some output telling me
> that the thread changed.
>
> However, now that thread 2 is selected, if I do this from the mi
> terminal:
>
> -thread-select --thread 1 1
>
> then I get nothing on the CLI terminal. This seems to be a bug.
>
> The problem was that when `-thread-select --thread 1 1` 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 this 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:
> * add 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 +++++++++++++++++++++++---------
> 4 files changed, 31 insertions(+), 29 deletions(-)
>
> 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 23e2373dcec..608b226f496 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
> @@ -1936,6 +1927,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)
> {
> @@ -1976,6 +1977,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)
> {
> @@ -2021,7 +2024,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. */
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 1/1] gdb/mi: PR20684, preserve user selected thread and frame when invoking MI commands
2022-02-18 17:45 ` Jan Vrany via Gdb-patches
2022-03-01 11:59 ` [PING] " Jan Vrany via Gdb-patches
@ 2022-03-02 10:00 ` Andrew Burgess via Gdb-patches
2022-03-02 13:23 ` [PATCH v4] " Jan Vrany via Gdb-patches
1 sibling, 1 reply; 17+ messages in thread
From: Andrew Burgess via Gdb-patches @ 2022-03-02 10:00 UTC (permalink / raw)
To: Jan Vrany; +Cc: GDB Patches
Jan Vrany via Gdb-patches <gdb-patches@sourceware.org> writes:
> On Thu, 2022-02-17 at 22:24 +0000, Andrew Burgess wrote:
>> * Jan Vrany via Gdb-patches <gdb-patches@sourceware.org> [2022-01-27 14:50:11 +0000]:
>>
>> > When invoking MI commands with --thread and/or --frame, user selected
>> > thread and frame was not preserved:
>> >
>> > (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 was problematic for frontends that provide access to CLI because UI
>> > may silently change the context for CLI commands (as demonstrated above).
>> >
>> > Bug: https://urldefense.proofpoint.com/v2/url?u=https-3A__sourceware.org_bugzilla_show-5Fbug.cgi-3Fid-3D20684&d=DwIBAg&c=sPZ6DeHLiehUHQWKIrsNwWp3t7snrE-az24ztT0w7Jc&r=WpFFGgYa98Yp-c29WHTCwU1wAGFBvszA6a4RzgpMSqc&m=soXzjKh4zT-FpxAe1wtdv7ka2r_jXNbrkEUR4XdUcOY&s=QkEM5YHXj7mpWI1y1z1AxRbosKxMhhfvfsMXmRnFoQc&e=
>> > ---
>> > gdb/mi/mi-cmds.h | 12 ++
>> > gdb/mi/mi-main.c | 54 ++++---
>> > gdb/testsuite/gdb.mi/mi-cmd-user-context.exp | 155 +++++++++++++++++++
>> > 3 files changed, 198 insertions(+), 23 deletions(-)
>> > create mode 100644 gdb/testsuite/gdb.mi/mi-cmd-user-context.exp
>> >
>> > 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..e112707e4d1 100644
>> > --- a/gdb/mi/mi-main.c
>> > +++ b/gdb/mi/mi-main.c
>> > @@ -1907,6 +1907,16 @@ command_notifies_uscc_observer (struct mi_parse *command)
>> > }
>> > }
>> >
>> > +/* 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;
>>
>> You need to wrap multi-line conditions like this inside parenthesis.
>>
>
> Thanks!
>
>> > +}
>> > +
>> > void
>> > mi_execute_command (const char *cmd, int from_tty)
>> > {
>> > @@ -1971,28 +1981,17 @@ mi_execute_command (const char *cmd, int from_tty)
>> > && any_thread_p ()
>> > /* If the command already reports the thread change, no need to do it
>> > again. */
>> > - && !command_notifies_uscc_observer (command.get ()))
>> > + && !command_notifies_uscc_observer (command.get ())
>> > + /* Don't report anything if --thread was specified -- the user selected
>> > + thread is preserved by mi_execute_command and therefore cannot
>> > + change. */
>> > + && command->thread == -1
>>
>> I think including this check is a bug. Not a bug of your making I
>> think, as I believe the bug already existed. I'll explain...
>>
>> If I start GDB, and then spawn a new-ui for the mi interpreter. Start
>> a multi-threaded inferior, and then stop at a gdb prompt.
>>
>> Assuming thread 1 is selected, then, from the mi terminal I do:
>>
>> -thread-select 2
>>
>> I will get the '^done,new-thread-id="2"....' result on the mi
>> terminal, but on the cli terminal I'll also get some output telling me
>> that the thread changed. I think this is what you'd want, right? The
>> cli thread _did_ just change.
>>
>> However, now that thread 2 is selected, if I do this from the mi
>> terminal:
>>
>> -thread-select --thread 1 1
>>
>> then I get nothing on the cli terminal. This feels like a bug to me,
>
> Yes, this looks like a bug indeed. And I think we just opened
> can of worms...
>
>> and it's caused by the 'command->thread == -1' check above.
>
> I do not think so. When the command is -thread-select, then
> command_notifies_uscc_observer() return 1, therefore the whole
> condition won't hold anyway, right?
>
>>
>> I think that check should just be dropped completely. Your scoped
>> thread/frame restore that you've added will ensure that, when
>> appropriate, the thread doesn't change. If it does change, then you
>> want to notify I think, regardless of whether --thread or --frame was
>> used.
>
> If I do just drop "&& command->thread == -1" from the condition, it breaks
> existing test "--stack-select-frame 3" in mi-cmd-user-context.exp since it
> produces change notification the test does not expect. Clearly it should not
> produce MI notification, whether it should "Switching to..." CLI output over MI
> channel I'm not sure.
>
> But:
>
> Now I think we can get rid of the whole "if" and with that command_notifies_uscc_observer()
> and command_changed_user_selected_thread(). There are only two MI commands
> that can change context are -thread-select and -stack-select-frame, right?
> -thread-select implementation ( in mi_cmd_thread_select () ) does notify observers
> on its own and if we add similar logic to -stack-select-frame implementation
> ( in mi_cmd_stack_select_frame () ) we're fine. Makes sense?
>
> I tried that (see the first patch below) and it seems to pass all MI tests
> and the code seems to me much simpler.
>
> Still, this change itself does not solve your original bug. That's because
> thread is already switched when we enter mi_cmd_thread_select() so the
> condition "inferior_ptid != previous_ptid" won't hold and so no notification
> is printed.
>
> I went ahead and tried to address this by lifting the logic up to mi_cmd_execute ()
> where --thread option is handled - see the second patch below. With that patch,
> all gdb.mi tests pass and your bug seems to be fixed. Still, the patch needs more
> work (test + check it works with -stack-frame-select).
>
> That being said, I'd be cautious. It took me quite a bit to make heads and tails
> of this and still not sure I got it. My preference would be to fix the original
> problem *WITHOUT* fixing "your" bug and once done, submit another patch fixing
> "your" bug.
>
> So, if you agree and the first part of my response about dropping the
> whole "if" makes any sense, I'll properly resubmit it as v4.
>
> Makes sense?
Sorry for missing this reply.
Yes, what you said makes sense. If you follow up with a v4 based on
your original patch plus minor fixes I suggested that will probably be
fine I think.
Thanks,
Andrew
>
>>
>> It would be great if we could have a test for this added too. You'll
>> need to start gdb with a separate mi tty. You can look at
>> gdb.mi/mi-exec-run.exp for an example, look for passing the
>> 'separate-mi-tty' string to mi_gdb_start, and then later for using
>> switch_gdb_spawn_id to switch between the main cli tty and the extra
>> mi tty. If you have any problem, I'll be happy to help.
>>
>> You still have some white space bugs (tabs vs spaces at the start of
>> lines), I have this in my .gitconfig:
>>
>> [core]
>> whitespace = space-before-tab,indent-with-non-tab,trailing-space
>>
>> which will have git highlight anywhere I've failed to indent with a
>> tab when I should have done.
>>
>> Additionally, some of your lines are over 80 characters, so need to be
>> wrapped.
>>
>> Thanks,
>> Andrew
>>
>>
>> > + /* Don't report anything if the selected thread actually did not change
>> > + (compared to selected thread before execution the command). */
>> > + && command_changed_user_selected_thread (previous_ptid, inferior_ptid))
>> > {
>> > - 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);
>> > - }
>> > + gdb::observers::user_selected_context_changed.notify
>> > + (USER_SELECTED_THREAD | USER_SELECTED_FRAME);
>> > }
>> > }
>> > }
>> > @@ -2037,6 +2036,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 +2047,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 +2061,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 <https://urldefense.proofpoint.com/v2/url?u=http-3A__www.gnu.org_licenses_&d=DwIBAg&c=sPZ6DeHLiehUHQWKIrsNwWp3t7snrE-az24ztT0w7Jc&r=WpFFGgYa98Yp-c29WHTCwU1wAGFBvszA6a4RzgpMSqc&m=soXzjKh4zT-FpxAe1wtdv7ka2r_jXNbrkEUR4XdUcOY&s=q6IZFROyYXzF9keb4NCS6EL93YiTxbx2HkwQEtqDr8k&e= >.
>> > +
>> > +# 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.30.2
>> >
>>
>
> From db229be447707d57980e4c21f394cdba1ec86b4c Mon Sep 17 00:00:00 2001
> From: Jan Vrany <jan.vrany@labware.com>
> Date: Thu, 27 Jan 2022 13:45:09 +0000
> Subject: [PATCH 1/2] gdb/mi: PR20684, preserve user selected thread and frame
> when invoking MI commands
>
> When invoking MI commands with --thread and/or --frame, user selected
> thread and frame was not preserved:
>
> (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 was problematic for frontends that provide access to CLI because UI
> may silently change the context for CLI commands (as demonstrated above).
>
> 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
>
>
> From 18d8dc95fe1a834ecbde319dd6e5960fd4d82a3f Mon Sep 17 00:00:00 2001
> From: Jan Vrany <jan.vrany@labware.com>
> Date: Fri, 18 Feb 2022 17:04:13 +0000
> Subject: [PATCH 2/2] [WIP] gdb/mi: properly notify user when GDB/MI client
> uses -thread-select
>
> If we start GDB, and then spawn a new-ui for the mi interpreter. Start
> a multi-threaded inferior, and then stop at a gdb prompt.
>
> Assuming thread 1 is selected, then, from the mi terminal I do:
>
> -thread-select 2
>
> We will get the '^done,new-thread-id="2"....' result on the mi
> terminal, but on the cli terminal I'll also get some output telling me
> that the thread changed.
>
> However, now that thread 2 is selected, if I do this from the mi
> terminal:
>
> -thread-select --thread 1 1
>
> then I get nothing on the CLI terminal. This seems to be a bug.
>
> The problem was that when `-thread-select --thread 1 1` 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 this 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:
> * add 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 +++++++++++++++++++++++---------
> 4 files changed, 31 insertions(+), 29 deletions(-)
>
> 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 23e2373dcec..608b226f496 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
> @@ -1936,6 +1927,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)
> {
> @@ -1976,6 +1977,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)
> {
> @@ -2021,7 +2024,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. */
> --
> 2.34.1
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v4] gdb/mi: PR20684, preserve user selected thread and frame when invoking MI commands
2022-03-02 10:00 ` Andrew Burgess via Gdb-patches
@ 2022-03-02 13:23 ` Jan Vrany via Gdb-patches
2022-03-08 16:58 ` Andrew Burgess via Gdb-patches
0 siblings, 1 reply; 17+ messages in thread
From: Jan Vrany via Gdb-patches @ 2022-03-02 13:23 UTC (permalink / raw)
To: gdb-patches; +Cc: Jan Vrany
When invoking MI commands with --thread and/or --frame, user selected
thread and frame was not preserved:
(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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4] gdb/mi: PR20684, preserve user selected thread and frame when invoking MI commands
2022-03-02 13:23 ` [PATCH v4] " Jan Vrany via Gdb-patches
@ 2022-03-08 16:58 ` Andrew Burgess via Gdb-patches
0 siblings, 0 replies; 17+ messages in thread
From: Andrew Burgess via Gdb-patches @ 2022-03-08 16:58 UTC (permalink / raw)
To: Jan Vrany via Gdb-patches, gdb-patches; +Cc: Jan Vrany
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
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2022-03-08 16:58 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-25 14:51 [PATCH 0/1] PR20684, preserve user selected context when invoking MI commands Jan Vrany via Gdb-patches
2022-01-25 14:51 ` [PATCH 1/1] gdb/mi: PR20684, preserve user selected thread and frame " Jan Vrany via Gdb-patches
2022-01-25 18:59 ` Simon Marchi via Gdb-patches
2022-01-26 13:21 ` [PATCH v2 0/1] PR20684, preserve user selected context " Jan Vrany via Gdb-patches
2022-01-26 13:21 ` [PATCH v2 1/1] gdb/mi: PR20684, preserve user selected thread and frame " Jan Vrany via Gdb-patches
2022-01-26 15:01 ` Simon Marchi via Gdb-patches
2022-01-26 17:08 ` Jan Vrany via Gdb-patches
2022-01-27 14:50 ` [PATCH v3 0/1] PR20684, preserve user selected context " Jan Vrany via Gdb-patches
2022-02-07 11:56 ` Jan Vrany via Gdb-patches
2022-02-17 16:07 ` [PING] " Jan Vrany via Gdb-patches
2022-01-27 14:50 ` [PATCH v3 1/1] gdb/mi: PR20684, preserve user selected thread and frame " Jan Vrany via Gdb-patches
2022-02-17 22:24 ` Andrew Burgess via Gdb-patches
2022-02-18 17:45 ` Jan Vrany via Gdb-patches
2022-03-01 11:59 ` [PING] " Jan Vrany via Gdb-patches
2022-03-02 10:00 ` Andrew Burgess via Gdb-patches
2022-03-02 13:23 ` [PATCH v4] " Jan Vrany via Gdb-patches
2022-03-08 16:58 ` Andrew Burgess via Gdb-patches
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox