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.133.124]) by sourceware.org (Postfix) with ESMTPS id 21C93385AC19 for ; Thu, 17 Feb 2022 22:24:33 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 21C93385AC19 Received: from mail-wr1-f71.google.com (mail-wr1-f71.google.com [209.85.221.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-182-_xctu92lNnK1KOF2UgqmNw-1; Thu, 17 Feb 2022 17:24:27 -0500 X-MC-Unique: _xctu92lNnK1KOF2UgqmNw-1 Received: by mail-wr1-f71.google.com with SMTP id g11-20020adfa48b000000b001e57dfb3c38so2886208wrb.2 for ; Thu, 17 Feb 2022 14:24:27 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=cs5W6HyJ7tkZQPU6HWDkxKTOrRr3sx8mFRAe+4JHuk0=; b=KtcOOQ739zkOe5/HAEag3qwfQNiF8qarfbkxp3W9SROIN8pZzWSjyZQOloeHlMfrYW XsHLJJ69n2IZyIE2mzoCsAcY7fXF822+dJ5RNrUk+LOwEaVEbooEceY0kjhAkKVhlZYB yuMhIP6B1c7k19znZDCUg5GerMzHVqguqDBYPxGob+3VuLatZTc4TNZQDkCFFWvAzNdn 61v7UH9igonoMQtOUPm1KQOs/rcLr1CLNiQj31MZsydSnV8njuKo6VuLCFRonCVk+9Dy KRnPbENMN2McBRyxT/T7W9MWZVD8XSQjPgTGboTrvUJ2sD3hgVzkWlADvRuqb1nqs89p eM+w== X-Gm-Message-State: AOAM533q6rGn/WzoW62+2Fx6Ru3+pALghICcS7lC+d2CPbQGqbmbauZ8 WXzGpFW9dU0OPX2lzD1Haugp2/w+XKh/VGAuXgUOQk8bT+o5N3kIidz4uF5QbRJi0+51MYRvEL+ /UtSIcxKSVQVf7cQvLhk1/w== X-Received: by 2002:a05:6000:1ace:b0:1e8:cbe4:9920 with SMTP id i14-20020a0560001ace00b001e8cbe49920mr3512060wry.121.1645136666146; Thu, 17 Feb 2022 14:24:26 -0800 (PST) X-Google-Smtp-Source: ABdhPJwaxo5O2PgWr3I1YjBzPH2ptW7Fc8b7yRAACUkUM3VtvKOIedULbdB1VWGWvsQDdcVDtaJf2w== X-Received: by 2002:a05:6000:1ace:b0:1e8:cbe4:9920 with SMTP id i14-20020a0560001ace00b001e8cbe49920mr3512044wry.121.1645136665809; Thu, 17 Feb 2022 14:24:25 -0800 (PST) Received: from localhost (host86-134-151-224.range86-134.btcentralplus.com. [86.134.151.224]) by smtp.gmail.com with ESMTPSA id f8sm2783062wmq.19.2022.02.17.14.24.25 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 17 Feb 2022 14:24:25 -0800 (PST) Date: Thu, 17 Feb 2022 22:24:24 +0000 From: Andrew Burgess To: Jan Vrany Cc: gdb-patches@sourceware.org Subject: Re: [PATCH v3 1/1] gdb/mi: PR20684, preserve user selected thread and frame when invoking MI commands Message-ID: <20220217222424.GQ2571@redhat.com> References: <74e5b388-9f7a-dd0a-befe-3b069fba54b7@polymtl.ca> <20220127145011.1153142-2-jan.vrany@labware.com> MIME-Version: 1.0 In-Reply-To: <20220127145011.1153142-2-jan.vrany@labware.com> X-Operating-System: Linux/5.8.18-100.fc31.x86_64 (x86_64) X-Uptime: 21:30:17 up 6 days, 11:09, X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii Content-Disposition: inline X-Spam-Status: No, score=-10.1 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_BARRACUDACENTRAL, RCVD_IN_DNSWL_LOW, 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: Thu, 17 Feb 2022 22:24:36 -0000 * Jan Vrany via Gdb-patches [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 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 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 . > + > +# 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 >