From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.polymtl.ca (smtp.polymtl.ca [132.207.4.11]) by sourceware.org (Postfix) with ESMTPS id 5505E3858001 for ; Thu, 25 Mar 2021 18:26:05 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 5505E3858001 Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id 12PIPrOE005161 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 25 Mar 2021 14:25:58 -0400 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 12PIPrOE005161 Received: from [10.0.0.11] (192-222-157-6.qc.cable.ebox.net [192.222.157.6]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 8705B1EF5D; Thu, 25 Mar 2021 14:25:53 -0400 (EDT) Subject: Re: [PATCH] MI: PR20684, preserve user selected thread and frame when invoking MI commands To: Jan Vrany , gdb-patches@sourceware.org References: <20210209100813.710754-1-jan.vrany@labware.com> From: Simon Marchi Message-ID: Date: Thu, 25 Mar 2021 14:25:53 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.7.1 MIME-Version: 1.0 In-Reply-To: <20210209100813.710754-1-jan.vrany@labware.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Thu, 25 Mar 2021 18:25:53 +0000 X-Spam-Status: No, score=-10.3 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_PASS, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) 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: Thu, 25 Mar 2021 18:26:06 -0000 On 2021-02-09 5:08 a.m., 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). Hi Jan, Sorry for not getting at this sooner, I was away for a while. We'll definitely want a test for this, because it's something that can easily break unexpectedly. One behavior change with this patch is: assuming thread 1 is selected, if you do: -thread-select --thread 2 3 Before: thread 3 becomes selected After: thread 1 stays selected Even though it looks a bit silly, I think we should keep the "before" behavior. An existing frontend may add `--thread X` to every command, including -thread-select. So let's say you click on thread 4 in the interface, it sends to GDB: -thread-select --thread 4 4 With this patch, the selection change wouldn't work. > gdb/Changelog > > PR 20684 > * gdb/mi/mi-main.c (mi_cmd_execute): Preserve user selected thread Remove leading "gdb/". > and frame when invoking MI commands. > --- > gdb/ChangeLog | 6 ++++++ > gdb/mi/mi-main.c | 36 +++++++++++++++--------------------- > 2 files changed, 21 insertions(+), 21 deletions(-) > > diff --git a/gdb/ChangeLog b/gdb/ChangeLog > index e67668d315c..ea232ff1b3f 100644 > --- a/gdb/ChangeLog > +++ b/gdb/ChangeLog > @@ -1,3 +1,9 @@ > +2021-02-09 Jan Vrany > + > + PR 20684 > + * gdb/mi/mi-main.c (mi_cmd_execute): Preserve user selected thread > + and frame when invoking MI commands. > + > 2021-02-08 Shahab Vahedi > > PR tdep/27369 > diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c > index 9a14d78e1e2..c5103800314 100644 > --- a/gdb/mi/mi-main.c > +++ b/gdb/mi/mi-main.c > @@ -1971,25 +1971,13 @@ mi_execute_command (const char *cmd, int from_tty) > 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); > + if (command->thread == -1 > + && previous_ptid != null_ptid > + && inferior_ptid != previous_ptid > + && inferior_ptid != null_ptid) > + { > + gdb::observers::user_selected_context_changed.notify > + (USER_SELECTED_THREAD | USER_SELECTED_FRAME); Can you explain the what and why of this change? It's not obvious to me. Simon