From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id F30533858C20 for ; Tue, 8 Mar 2022 16:58:18 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org F30533858C20 Received: from mail-wr1-f70.google.com (mail-wr1-f70.google.com [209.85.221.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-635-VGBE5miyOl2O5d5kA8dO6Q-1; Tue, 08 Mar 2022 11:58:15 -0500 X-MC-Unique: VGBE5miyOl2O5d5kA8dO6Q-1 Received: by mail-wr1-f70.google.com with SMTP id t15-20020adfdc0f000000b001ef93643476so5658981wri.2 for ; Tue, 08 Mar 2022 08:58:15 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:from:to:cc:subject:in-reply-to:references:date :message-id:mime-version; bh=/UvT/l324O8ad/hQEmbgwdKpW7yJU2ZcmCi+3Y52tVA=; b=jVSHTqHlKaI/swVT0Z5BTOp6Vfk0Stwc/Tr49hqGeM6xj6ldjOlpGdXUC5Y+F2M+tZ HvaoDvL1AOkcUoZVQzjjQCv1+rPNfi2PskTcMwoFRKUluKlH28IOK0M9g2YmDvxn8Svk DnrW4MeEbdGcmV9Kq/ygg08+JhBvXw8qX80LQqLySWUAUe1u36tr3BLycDV0QdHEfQbC ojc5Yewa0Cwpx3ZRnxGvsTqlXp/INQ0dmXZ4liG/leE1okto20KS7iZOv5Y0xBljUlSc EB+NWxuobxN99PAyZ5P2t3PvraG4+K076rYRt2ojlE/J5bWGaCKXPy83fvSW0Ff8CSsm dOpQ== X-Gm-Message-State: AOAM533JzUEbhAC4FOp1TxlpRmxXVG67lB76UTAPwEw1fBnsmnQXwfcm 7ayJjpcGwWB9dvx+CH5lGcJZzRR66at6Wwon1nB+ELetjOFfkYP1bsyqc/GI7QOAGcfmI2LDVuE 66afcnIqgf2k0yAOlVMvdy941N91oVPO9K95JnYN3GISjURLQxVJ/ZuWm96l9n9Jej5BStADSgQ == X-Received: by 2002:a05:6000:1a89:b0:203:706f:7c67 with SMTP id f9-20020a0560001a8900b00203706f7c67mr2503032wry.471.1646758694096; Tue, 08 Mar 2022 08:58:14 -0800 (PST) X-Google-Smtp-Source: ABdhPJwIzWZGp07AL1OP22GSJBz0frhRFRQtF1Bq6+KNyUed14fNS2GEqN+Ok9I+feY46frCf6Qp2w== X-Received: by 2002:a05:6000:1a89:b0:203:706f:7c67 with SMTP id f9-20020a0560001a8900b00203706f7c67mr2503011wry.471.1646758693600; Tue, 08 Mar 2022 08:58:13 -0800 (PST) Received: from localhost (host86-134-151-205.range86-134.btcentralplus.com. [86.134.151.205]) by smtp.gmail.com with ESMTPSA id h36-20020a05600c49a400b00382aa0b1619sm2642072wmp.45.2022.03.08.08.58.12 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 08 Mar 2022 08:58:13 -0800 (PST) From: Andrew Burgess To: Jan Vrany via Gdb-patches , gdb-patches@sourceware.org Cc: Jan Vrany Subject: Re: [PATCH v4] gdb/mi: PR20684, preserve user selected thread and frame when invoking MI commands In-Reply-To: <20220302132330.1083078-1-jan.vrany@labware.com> References: <87a6e8znbi.fsf@redhat.com> <20220302132330.1083078-1-jan.vrany@labware.com> Date: Tue, 08 Mar 2022 16:58:11 +0000 Message-ID: <871qzcwfdo.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-Spam-Status: No, score=-11.4 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_ASCII_DIVIDERS, KAM_SHORT, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H5, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 08 Mar 2022 16:58:21 -0000 Jan Vrany via Gdb-patches 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 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 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 . > + > +# 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