From: Andrew Burgess <aburgess@redhat.com>
To: Jan Vrany <jan.vrany@labware.com>, gdb-patches@sourceware.org
Cc: Jan Vrany <jan.vrany@labware.com>
Subject: Re: [PATCH v2] gdb/mi: consistently notify user when GDB/MI client uses -thread-select
Date: Wed, 16 Mar 2022 14:35:00 +0000 [thread overview]
Message-ID: <87mthqt17v.fsf@redhat.com> (raw)
In-Reply-To: <20220316140316.1020010-1-jan.vrany@labware.com>
Jan Vrany <jan.vrany@labware.com> writes:
> Changes since v1:
>
> * all improvements and tests by Andrew - thanks a lot!
> * merged mi_command::invoke() and mi_command::do_invoke() into
> single method (mi_command::invoke()). See the commit message
> for rationale.
>
> -- >8 --
> GDB notifies users about user selected thread changes somewhat
> inconsistently as mentioned on gdb-patches mailing list here:
>
> https://sourceware.org/pipermail/gdb-patches/2022-February/185989.html
>
> Consider GDB debugging a multi-threaded inferior with both CLI and GDB/MI
> interfaces connected to separate terminals.
>
> Assuming inferior is stopped and thread 1 is selected, when a thread
> 2 is selected using '-thread-select 2' command on GDB/MI terminal:
>
> -thread-select 2
> ^done,new-thread-id="2",frame={level="0",addr="0x00005555555551cd",func="child_sub_function",args=[],file="/home/jv/Projects/gdb/users_jv_patches/gdb/testsuite/gdb.mi/user-selected-context-sync.c",fullname="/home/uuu/gdb/gdb/testsuite/gdb.mi/user-selected-context-sync.c",line="30",arch="i386:x86-64"}
> (gdb)
>
> and on CLI terminal we get the notification (as expected):
>
> [Switching to thread 2 (Thread 0x7ffff7daa640 (LWP 389659))]
> #0 child_sub_function () at /home/uuu/gdb/gdb/testsuite/gdb.mi/user-selected-context-sync.c:30
> 30 volatile int dummy = 0;
>
> However, now that thread 2 is selected, if thread 1 is selected
> using 'thread-select --thread 1 1' command on GDB/MI terminal
> terminal:
>
> -thread-select --thread 1 1
> ^done,new-thread-id="1",frame={level="0",addr="0x0000555555555294",func="main",args=[],file="/home/jv/Projects/gdb/users_jv_patches/gdb/testsuite/gdb.mi/user-selected-context-sync.c",fullname="/home/jv/Projects/gdb/users_jv_patches/gdb/testsuite/gdb.mi/user-selected-context-sync.c",line="66",arch="i386:x86-64"}
> (gdb)
>
> but no notification is printed on CLI terminal, despite the fact
> that user selected thread has changed.
>
> The problem is that when `-thread-select --thread 1 1` is executed
> then thread is switched to thread 1 before mi_cmd_thread_select () is
> called, therefore the condition "inferior_ptid != previous_ptid"
> there does not hold.
>
> To address this problem, we have to move notification logic up to
> mi_cmd_execute () where --thread option is processed and notify
> user selected contents observers there if context changes.
>
> However, this in itself breaks GDB/MI because it would cause context
> notification to be sent on MI channel. This is because by the time
> we notify, MI notification suppression is already restored (done in
> mi_command::invoke(). Therefore we had to lift notification suppression
> logic also up to mi_cmd_execute (). This change in made distinction
> between mi_command::invoke() and mi_command::do_invoke() unnecessary
> as all mi_command::invoke() did (after the change) was to call
> do_invoke(). So this patches removes do_invoke() and moves the command
> execution logic directly to invoke().
>
> With this change, all gdb.mi tests pass, tested on x86_64-linux.
>
> Co-authored-by: Andrew Burgess <aburgess@redhat.com>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=20631
> ---
> gdb/mi/mi-cmd-stack.c | 12 +---
> gdb/mi/mi-cmds.c | 18 +-----
> gdb/mi/mi-cmds.h | 17 ++----
> gdb/mi/mi-main.c | 51 +++++++++++++---
> gdb/python/py-micmd.c | 5 +-
> gdb/stack.c | 8 ---
> gdb/stack.h | 6 --
> .../gdb.mi/user-selected-context-sync.exp | 58 +++++++++++++++++--
> 8 files changed, 106 insertions(+), 69 deletions(-)
Thanks this looks great. Please go ahead and push this.
Thanks,
Andrew
>
> diff --git a/gdb/mi/mi-cmd-stack.c b/gdb/mi/mi-cmd-stack.c
> index 1be8aa81c3d..e894411765a 100644
> --- a/gdb/mi/mi-cmd-stack.c
> +++ b/gdb/mi/mi-cmd-stack.c
> @@ -757,17 +757,7 @@ mi_cmd_stack_select_frame (const char *command, char **argv, int argc)
> {
> if (argc == 0 || argc > 1)
> error (_("-stack-select-frame: Usage: FRAME_SPEC"));
> -
> - ptid_t previous_ptid = inferior_ptid;
> -
> - select_frame_for_mi (parse_frame_specification (argv[0]));
> -
> - /* Notify if the thread has effectively changed. */
> - if (inferior_ptid != previous_ptid)
> - {
> - gdb::observers::user_selected_context_changed.notify
> - (USER_SELECTED_THREAD | USER_SELECTED_FRAME);
> - }
> + select_frame (parse_frame_specification (argv[0]));
> }
>
> void
> diff --git a/gdb/mi/mi-cmds.c b/gdb/mi/mi-cmds.c
> index 38fbe0d8a32..60fec0a0b85 100644
> --- a/gdb/mi/mi-cmds.c
> +++ b/gdb/mi/mi-cmds.c
> @@ -45,11 +45,9 @@ struct mi_command_mi : public mi_command
> gdb_assert (func != nullptr);
> }
>
> -protected:
> -
> /* Called when this MI command has been invoked, calls m_argv_function
> with arguments contained within PARSE. */
> - void do_invoke (struct mi_parse *parse) const override
> + void invoke (struct mi_parse *parse) const override
> {
> mi_parse_argv (parse->args, parse);
>
> @@ -83,13 +81,11 @@ struct mi_command_cli : public mi_command
> m_args_p (args_p)
> { /* Nothing. */ }
>
> -protected:
> -
> /* Called when this MI command has been invoked, calls the m_cli_name
> CLI function. In m_args_p is true then the argument string from
> within PARSE is passed through to the CLI function, otherwise nullptr
> is passed through to the CLI function as its argument string. */
> - void do_invoke (struct mi_parse *parse) const override
> + void invoke (struct mi_parse *parse) const override
> {
> const char *args = m_args_p ? parse->args : nullptr;
> mi_execute_cli_command (m_cli_name, m_args_p, args);
> @@ -173,16 +169,6 @@ mi_command::mi_command (const char *name, int *suppress_notification)
>
> /* See mi-cmds.h. */
>
> -void
> -mi_command::invoke (struct mi_parse *parse) const
> -{
> - gdb::optional<scoped_restore_tmpl<int>> restore
> - = do_suppress_notification ();
> - this->do_invoke (parse);
> -}
> -
> -/* See mi-cmds.h. */
> -
> gdb::optional<scoped_restore_tmpl<int>>
> mi_command::do_suppress_notification () const
> {
> diff --git a/gdb/mi/mi-cmds.h b/gdb/mi/mi-cmds.h
> index 47b90a26064..05b702f83ec 100644
> --- a/gdb/mi/mi-cmds.h
> +++ b/gdb/mi/mi-cmds.h
> @@ -160,9 +160,10 @@ struct mi_command
> const char *name () const
> { return m_name; }
>
> - /* Execute the MI command. Can throw an exception if something goes
> - wrong. */
> - void invoke (struct mi_parse *parse) const;
> + /* Execute the MI command. this needs to be overridden in each
> + base class. PARSE is the parsed command line from the user.
> + Can throw an exception if something goes wrong. */
> + virtual void invoke (struct mi_parse *parse) const = 0;
>
> /* Return whether this command preserves user selected context (thread
> and frame). */
> @@ -175,14 +176,6 @@ struct mi_command
> return m_suppress_notification != &mi_suppress_notification.user_selected_context;
> }
>
> -protected:
> -
> - /* The core of command invocation, this needs to be overridden in each
> - base class. PARSE is the parsed command line from the user. */
> - virtual void do_invoke (struct mi_parse *parse) const = 0;
> -
> -private:
> -
> /* If this command was created with a suppress notifications pointer,
> then this function will set the suppress flag and return a
> gdb::optional with its value set to an object that will restore the
> @@ -192,6 +185,8 @@ struct mi_command
> then this function returns an empty gdb::optional. */
> gdb::optional<scoped_restore_tmpl<int>> do_suppress_notification () const;
>
> +private:
> +
> /* The name of the command. */
> const char *m_name;
>
> diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
> index 73380f5e668..abd033b22ae 100644
> --- a/gdb/mi/mi-main.c
> +++ b/gdb/mi/mi-main.c
> @@ -556,19 +556,10 @@ mi_cmd_thread_select (const char *command, char **argv, int argc)
> if (thr == NULL)
> error (_("Thread ID %d not known."), num);
>
> - ptid_t previous_ptid = inferior_ptid;
> -
> thread_select (argv[0], thr);
>
> print_selected_thread_frame (current_uiout,
> USER_SELECTED_THREAD | USER_SELECTED_FRAME);
> -
> - /* Notify if the thread has effectively changed. */
> - if (inferior_ptid != previous_ptid)
> - {
> - gdb::observers::user_selected_context_changed.notify
> - (USER_SELECTED_THREAD | USER_SELECTED_FRAME);
> - }
> }
>
> void
> @@ -1975,6 +1966,37 @@ mi_execute_command (const char *cmd, int from_tty)
> }
> }
>
> +/* Captures the current user selected context state, that is the current
> + thread and frame. Later we can then check if the user selected context
> + has changed at all. */
> +
> +struct user_selected_context
> +{
> + /* Constructor. */
> + user_selected_context ()
> + : m_previous_ptid (inferior_ptid),
> + m_previous_frame (deprecated_safe_get_selected_frame ())
> + { /* Nothing. */ }
> +
> + /* Return true if the user selected context has changed since this object
> + was created. */
> + bool has_changed () const
> + {
> + return ((m_previous_ptid != null_ptid
> + && inferior_ptid != null_ptid
> + && m_previous_ptid != inferior_ptid)
> + || m_previous_frame != deprecated_safe_get_selected_frame ());
> + }
> +private:
> + /* The previously selected thread. This might be null_ptid if there was
> + no previously selected thread. */
> + ptid_t m_previous_ptid;
> +
> + /* The previously selected frame. This might be nullptr if there was no
> + previously selected frame. */
> + frame_info *m_previous_frame;
> +};
> +
> static void
> mi_cmd_execute (struct mi_parse *parse)
> {
> @@ -2015,6 +2037,8 @@ mi_cmd_execute (struct mi_parse *parse)
> set_current_program_space (inf->pspace);
> }
>
> + user_selected_context current_user_selected_context;
> +
> gdb::optional<scoped_restore_current_thread> thread_saver;
> if (parse->thread != -1)
> {
> @@ -2060,7 +2084,16 @@ mi_cmd_execute (struct mi_parse *parse)
> current_context = parse;
>
> gdb_assert (parse->cmd != nullptr);
> +
> + gdb::optional<scoped_restore_tmpl<int>> restore_suppress_notification
> + = parse->cmd->do_suppress_notification ();
> +
> parse->cmd->invoke (parse);
> +
> + if (!parse->cmd->preserve_user_selected_context ()
> + && current_user_selected_context.has_changed ())
> + gdb::observers::user_selected_context_changed.notify
> + (USER_SELECTED_THREAD | USER_SELECTED_FRAME);
> }
>
> /* See mi-main.h. */
> diff --git a/gdb/python/py-micmd.c b/gdb/python/py-micmd.c
> index 4665fcc75c7..399e89b0298 100644
> --- a/gdb/python/py-micmd.c
> +++ b/gdb/python/py-micmd.c
> @@ -134,9 +134,8 @@ struct mi_command_py : public mi_command
> m_pyobj = new_pyobj;
> }
>
> -protected:
> /* Called when the MI command is invoked. */
> - virtual void do_invoke(struct mi_parse *parse) const override;
> + virtual void invoke(struct mi_parse *parse) const override;
>
> private:
> /* The Python object representing this MI command. */
> @@ -311,7 +310,7 @@ serialize_mi_result (PyObject *result)
> command line arguments from the user. */
>
> void
> -mi_command_py::do_invoke (struct mi_parse *parse) const
> +mi_command_py::invoke (struct mi_parse *parse) const
> {
> PYMICMD_SCOPED_DEBUG_ENTER_EXIT;
>
> diff --git a/gdb/stack.c b/gdb/stack.c
> index 8855ed8fe70..10da88b88e5 100644
> --- a/gdb/stack.c
> +++ b/gdb/stack.c
> @@ -1841,14 +1841,6 @@ select_frame_command_core (struct frame_info *fi, bool ignored)
> gdb::observers::user_selected_context_changed.notify (USER_SELECTED_FRAME);
> }
>
> -/* See stack.h. */
> -
> -void
> -select_frame_for_mi (struct frame_info *fi)
> -{
> - select_frame_command_core (fi, false /* Ignored. */);
> -}
> -
> /* The core of all the "frame" sub-commands. Select frame FI, and if this
> means we change frame send out a change notification (otherwise, just
> reprint the current frame summary). */
> diff --git a/gdb/stack.h b/gdb/stack.h
> index c49d2e2cbef..f78aedf1c85 100644
> --- a/gdb/stack.h
> +++ b/gdb/stack.h
> @@ -20,12 +20,6 @@
> #ifndef STACK_H
> #define STACK_H
>
> -/* Access method used by the MI -stack-select-frame command to switch to
> - frame FI. This differs from SELECT_FRAME in that the observers for a
> - user selected context change will be triggered. */
> -
> -void select_frame_for_mi (struct frame_info *fi);
> -
> gdb::unique_xmalloc_ptr<char> find_frame_funname (struct frame_info *frame,
> enum language *funlang,
> struct symbol **funcp);
> diff --git a/gdb/testsuite/gdb.mi/user-selected-context-sync.exp b/gdb/testsuite/gdb.mi/user-selected-context-sync.exp
> index 3b4ff0bf2e4..9444ca5acf4 100644
> --- a/gdb/testsuite/gdb.mi/user-selected-context-sync.exp
> +++ b/gdb/testsuite/gdb.mi/user-selected-context-sync.exp
> @@ -916,7 +916,7 @@ proc_with_prefix test_mi_thread_select { mode } {
> }
> }
>
> - with_test_prefix "thread 1.2 with --thread" {
> + with_test_prefix "thread 1.2 with --thread 2" {
> # Test selecting a thread from MI with a --thread option. This test
> # verifies that even if the thread GDB would switch to is the same as
> # the thread specified with --thread, an event is still sent to CLI.
> @@ -930,10 +930,28 @@ proc_with_prefix test_mi_thread_select { mode } {
> }
>
> with_spawn_id $gdb_main_spawn_id {
> - # This doesn't work as of now, no event is sent on CLI. It is
> - # commented out so we don't have to wait for the timeout every time.
> - # match_re_or_ensure_no_output "$cli_re\r\n" "-thread-select, event on cli"
> - kfail "gdb/20631" "thread-select, event on cli"
> + match_re_or_ensure_no_output "$cli_re\r\n" "-thread-select, event on cli"
> + }
> + }
> +
> + with_test_prefix "thread 1.2 with --thread 3" {
> + # Test selecting a thread from MI with a --thread option.
> + # This test verifies that when different thread numbers are
> + # passed to the --thread option and the underlying
> + # -thread-select command, the correct thread is selected.
> + # In this case this is thread 1.2
> +
> + reset_selection "1.1"
> +
> + set mi_re [make_mi_re $mode 2 0 response]
> + set cli_re [make_cli_re $mode -1 1.2 0]
> +
> + with_spawn_id $mi_spawn_id {
> + mi_gdb_test "-thread-select --thread 3 2" $mi_re "-thread-select"
> + }
> +
> + with_spawn_id $gdb_main_spawn_id {
> + match_re_or_ensure_no_output "$cli_re\r\n" "-thread-select, event on cli"
> }
> }
>
> @@ -974,6 +992,36 @@ proc_with_prefix test_mi_stack_select_frame { mode } {
> with_spawn_id $gdb_main_spawn_id {
> match_re_or_ensure_no_output $cli_re "-stack-select-frame again, event on CLI"
> }
> +
> + # Now use the '-stack-select-frame' command with the --frame
> + # option, this verifies that even when the frame GDB would
> + # swith to is the same as the frame specified with --frame, an
> + # event is still sent to the CLI.
> +
> + set cli_re [make_cli_re $mode -1 -1 0]
> +
> + with_spawn_id $mi_spawn_id {
> + mi_gdb_test "-stack-select-frame --thread 2 --frame 0 0" $mi_re
> + }
> +
> + with_spawn_id $gdb_main_spawn_id {
> + match_re_or_ensure_no_output "$cli_re\r\n" "-stack-select-frame with --frame 0, event on CLI"
> + }
> +
> + # Now use the '-stack-select-frame' command with the --frame
> + # option, this verifies that the correct event is sent to the
> + # CLI when the frame specified with --frame is different to
> + # the actual frame selected.
> +
> + set cli_re [make_cli_re $mode -1 -1 1]
> +
> + with_spawn_id $mi_spawn_id {
> + mi_gdb_test "-stack-select-frame --thread 2 --frame 2 1" $mi_re
> + }
> +
> + with_spawn_id $gdb_main_spawn_id {
> + match_re_or_ensure_no_output "$cli_re\r\n" "-stack-select-frame with --frame 2, event on CLI"
> + }
> }
>
> with_test_prefix "thread 1.3" {
> --
> 2.35.1
prev parent reply other threads:[~2022-03-16 14:35 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-03-14 11:57 [RFC PATCH] " Jan Vrany
2022-03-15 17:10 ` Andrew Burgess
2022-03-16 14:03 ` [PATCH v2] " Jan Vrany
2022-03-16 14:35 ` Andrew Burgess [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87mthqt17v.fsf@redhat.com \
--to=aburgess@redhat.com \
--cc=gdb-patches@sourceware.org \
--cc=jan.vrany@labware.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).