From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ed1-f45.google.com (mail-ed1-f45.google.com [209.85.208.45]) by sourceware.org (Postfix) with ESMTPS id 020DE3968C39 for ; Wed, 14 Apr 2021 15:27:49 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 020DE3968C39 Received: by mail-ed1-f45.google.com with SMTP id ba6so24212776edb.1 for ; Wed, 14 Apr 2021 08:27:48 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:message-id:subject:from:to:cc:date:in-reply-to :references:user-agent:mime-version:content-transfer-encoding; bh=3wJo3KS1ZVsz6zPt/0D9cVMVcIyl3Qjud9wYj8Jarm4=; b=bX8lDN6R5U1EeywNmPQF5izuY3lMZdxQk9PlGjmVPkopDAbGq69iq6TTlNzgdOX3TG regOxwRHTbegABIZRIl4/S5lKnZ8hXm95Il0QmjIMr8En63v0FO6kQylGKANaV3Oxcp8 rxtAhG/G0bo55FNSVtOCknGfFj6l6gbvaFNONjoSUVs3Ce7X/FyWQgiBu83LXDJXFGIp WEiXkLlc8+96v6YsymiQPu9oISDwh46w+i22kgRW9lZBn7oFX6OFhgvB6mFHS200KJJK tyXx+J+fp3Q9lGakHfWU1/chCSnfZ4YgrbXQfCS0C4KGHldlfKMdN0W81zFcBnkUrVSk r2hw== X-Gm-Message-State: AOAM533eRsa+kr03kqnHA0vCgkTd05B76OUmo3gsi0dLn5zgewesXz5P EKbAoUzlm68UpsMNx1ZiwKlpoU0v7RE= X-Google-Smtp-Source: ABdhPJyGaCNke01HvmIwfocBGYZ9nSKkYiXSbsR9eZ3W8GKvKVEFBbz+Gy/MrvTAx74vx9OGePUyig== X-Received: by 2002:a05:6402:2746:: with SMTP id z6mr42360596edd.146.1618414068027; Wed, 14 Apr 2021 08:27:48 -0700 (PDT) Received: from sao.lan ([83.219.56.252]) by smtp.gmail.com with ESMTPSA id k16sm9137920ejv.37.2021.04.14.08.27.46 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 14 Apr 2021 08:27:46 -0700 (PDT) Message-ID: <7901c56cd5e8d71893038e66b71f7f6d56205b96.camel@labware.com> Subject: Re: [PATCH] MI: PR20684, preserve user selected thread and frame when invoking MI commands From: Jan Vrany To: "gdb-patches@sourceware.org" Date: Wed, 14 Apr 2021 16:27:45 +0100 In-Reply-To: References: <20210209100813.710754-1-jan.vrany@labware.com> Content-Type: text/plain; charset="UTF-8" User-Agent: Evolution 3.38.3-1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-11.0 required=5.0 tests=BAYES_00, BODY_8BITS, FREEMAIL_ENVFROM_END_DIGIT, FREEMAIL_FORGED_FROMDOMAIN, FREEMAIL_FROM, GIT_PATCH_0, HEADER_FROM_DIFFERENT_DOMAINS, KAM_DMARC_STATUS, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, 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: Wed, 14 Apr 2021 15:27:50 -0000 On Thu, 2021-03-25 at 14:25 -0400, Simon Marchi wrote: > > > > > > 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. Hi Simon, Sorry for not getting at this sooner, this time I was away. I have written some tests, though I do not test notifications when having separate CLI and MI channel. I tried to look at user-selected-context-sync.exp but failed to understand how  it works. > > 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. Good catch, thanks! It did not occur to me someone might do that. Similar thing may happen for frame and -frame-select. The only way fixing it I can think of now is to special-case it like: @@ -2034,7 +2034,12 @@ mi_cmd_execute (struct mi_parse *parse) if (tp->state == THREAD_EXITED) error (_("Thread id: %d has terminated"), parse->thread); - thread_saver.emplace (); + /* The -thread-select and -select-frame are the only commands + that change the currently selected thread, so do not restore + thread after executing any of them. */ + if (parse->cmd->argv_func != mi_cmd_thread_select + && parse->cmd->argv_func != mi_cmd_stack_select_frame) + thread_saver.emplace (); switch_to_thread (tp); } This fixes your usecase, but is there a better way? I need to  think of it. > > > 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. I actually wrote this patch a long time ago and using it since then, but I believe the reason for removing  else if (inferior_ptid != null_ptid) { ... } branch was that if --thread is specified, we do not want any notifications that context has changed. Imagine the currently selected thread is 1, frontend passes --thread 2 to a command. Then  report_change = (ti->global_num != command->thread); will be true (because 1 != 2) and CLI channel (if any) would get notification which is not what we want [1] [1]: https://sourceware.org/legacy-ml/gdb/2019-06/msg00057.html Thanks!  Jan