Re: [PATCHv2] gdb: notify of inferior switch when needed from 'thread' command

[Andrew Burgess <aburgess@redhat.com>]

Guinevere Larsen <guinevere@redhat.com> writes:

> On 1/26/26 10:58 AM, Andrew Burgess wrote:
>> In v2:
>>
>>    - Rebased and retested.
>>
>>    - Reworked commit message to try and address Pedro's concerns.
>>
>> Thanks,
>> Andrew
>>
>> ---
>
> Hi Andrew!
>
> I've taken a look at this patch and it looks pretty good, and I think it 
> is a good change.
>
> I know you're waiting for Pedro's feedback, but hopefully this message 
> acts as a ping for him as well as a review for you
>
> Reviewed-By: Guinevere Larsen <guinevere@redhat.com>

Thanks Guinevere, I appreciate the feedback.

I've been wondering what to do about this patch.  Usually I would have
self-approved it and applied it by now, but I'm very reluctant to do
that given Pedro's previous comments.

I've added Pedro to the 'To:' list,  I should have done that with the
original v2 patch.  I'll give this a little longer yet, my hope is to
land this for GDB 18.

Thanks,
Andrew


>
>>
>> While working on the commit:
>>
>>    commit 9959019545d8d6d71d927f20f088efba944b1e9c
>>    Date:   Sun Sep 28 16:16:53 2025 +0100
>>
>>        gdb: fix for 'set suppress-cli-notifications on' missed case
>>
>> I spotted this message in the gdb.mi/user-selected-context-sync.exp
>> test script:
>>
>>    # Idea for the future: selecting a thread in a different inferior.  For now,
>>    # GDB doesn't show an inferior switch, but if it did, it would be a nice
>>    # place to test it.
>>
>> What this message is talking about is this behaviour:
>>
>>    (gdb) info threads
>>      Id   Target Id                              Frame
>>      1.1  Thread 0xf7dbc700 (LWP 818430) "thr" 0xf7eb2888 in clone () from /lib/libc.so.6
>>      1.2  Thread 0xf7dbbb40 (LWP 818433) "thr" 0xf7fd0579 in __kernel_vsyscall ()
>>      1.3  Thread 0xf73ffb40 (LWP 818434) "thr" breakpt () at thr.c:19
>>      2.1  Thread 0xf7dbc700 (LWP 818456) "thr" 0xf7eb2888 in clone () from /lib/libc.so.6
>>      2.2  Thread 0xf7dbbb40 (LWP 818457) "thr" breakpt () at thr.c:19
>>    * 2.3  Thread 0xf73ffb40 (LWP 818458) "thr" breakpt () at thr.c:19
>>    (gdb) inferior 1
>>    [Switching to inferior 1 [process 818430] (/home/andrew/tmp/thr)]
>>    [Switching to thread 1.1 (Thread 0xf7dbc700 (LWP 818430))]
>>    #0  0xf7eb2888 in clone () from /lib/libc.so.6
>>    (gdb) thread 2.2
>>    [Switching to thread 2.2 (Thread 0xf7dbbb40 (LWP 818457))]
>>    #0  breakpt () at thr.c:19
>>    19	  while (stop)
>>    (gdb)
>>
>> Notice that when we switch from thread 2.3 to 1.1 using the 'inferior
>> 1' command, GDB tells us that the inferior has changed, and that the
>> thread has changed (and also that the frame has changed).
>>
>> But, when we switch from 1.1 to 2.2 using the 'thread 2.2' command, we
>> are only told about the thread change.
>>
>> The 'Switching to inferior ...' line includes some useful information,
>> the process PID and the executable name, and I think it is a shame
>> that these are not presented when using the 'thread' command to switch
>> inferior.
>>
>> So, this commit addresses this issue.
>>
>> A question that came up during review, and which I'm clarifying here:
>> this change only effects the output of GDB when the thread command is
>> also used to switch inferiors.  I am (in effect) arguing that the
>> command 'thread 2.2' should be treated as a shorthand for 'inferior 2;
>> thread 2', and should display all of the associated output.  If the
>> user is only switching threads within a single inferior then it is not
>> necessary to re-display the inferior information.
>>
>> I acknowledge that this does mean the output of the 'thread' command
>> will now be different depending on whether the user changes inferior
>> or not.  However, I think this is better than the alternative, having
>> the 'thread' command always re-print the inferior information.  I
>> think this would introduce excess noise that is not useful.
>>
>> There are changes in basically two areas.  The easy part is in
>> thread_command (thread.c).  Here we spot when the inferior has changed
>> as a result of the 'thread' command, and included
>> USER_SELECTED_INFERIOR in the set of state passed to the
>> notify_user_selected_context_changed function.
>>
>> The change in mi/mi-main.c is a little more involved.  In the
>> mi_cmd_execute function we use an instance of user_selected_context to
>> spot if any inferior state (frame, thread, or inferior) changes after
>> an MI command, this is then used to decide if there should be a call
>> to notify_user_selected_context_changed.
>>
>> Currently, in mi_cmd_execute, notify_user_selected_context_changed is
>> always passed 'USER_SELECTED_THREAD | USER_SELECTED_FRAME'.  This
>> makes sense, the MI doesn't allow "switching inferiors" as a command,
>> instead, an MI frontend must switch threads, and the inferior is
>> switched as a consequence.  But this does mean that if a user has a
>> CLI and MI interpreter running, and the MI switches threads, the CLI
>> will only receive the thread switch style notifications, that is,
>> there will be no "Switching to inferior ..." line.
>>
>> What I've done is rename user_selected_context::has_changed to
>> user_selected_context::what_changed, this function is now responsible
>> for returning the set of USER_SELECTED_* flags that indicate what
>> changed.
>>
>> If anything has changed then we always return USER_SELECTED_THREAD |
>> USER_SELECTED_FRAME as a minimum.  This retains the existing
>> behaviour, but is possibly more aggressive that we need to be; the
>> -stack-select-frame command can only change the frame, so maybe in
>> this case we should only return USER_SELECTED_FRAME?  I've left that
>> for the future though.
>>
>> However, the important change is that in ::what_changed, I now spot
>> when the inferior has changed and include USER_SELECTED_INFERIOR in
>> the set of flags that are returned.
>>
>> In mi_cmd_execute we now call the new what_changed function, and use
>> the set of flags returned when calling
>> notify_user_selected_context_changed.  This means that the CLI will
>> now receive inferior changed notifications when appropriate.
>>
>> The gdb.mi/user-selected-context-sync.exp script has been updated,
>> replacing the comment I quoted above with an actual test that the
>> inferior change is announced correctly.
>> ---
>>   gdb/mi/mi-main.c                              | 44 ++++++++++++---
>>   .../gdb.mi/user-selected-context-sync.exp     | 56 ++++++++++++++++---
>>   gdb/thread.c                                  | 13 ++++-
>>   3 files changed, 93 insertions(+), 20 deletions(-)
>>
>> diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
>> index 77e56bc9fc8..a18209366ff 100644
>> --- a/gdb/mi/mi-main.c
>> +++ b/gdb/mi/mi-main.c
>> @@ -538,6 +538,11 @@ mi_cmd_thread_select (const char *command, const char *const *argv, int argc)
>>   
>>     thread_select (argv[0], thr);
>>   
>> +  /* We don't call print_selected_inferior here as this never prints
>> +     anything when the output is MI like (as it is now).  MI consumers are
>> +     expected to derive the inferior change from the global thread-id
>> +     included in the print_selected_thread_frame output.  */
>> +
>>     print_selected_thread_frame (current_uiout,
>>   			       USER_SELECTED_THREAD | USER_SELECTED_FRAME);
>>   }
>> @@ -1968,12 +1973,30 @@ struct user_selected_context
>>   
>>     /* Return true if the user selected context has changed since this object
>>        was created.  */
>> -  bool has_changed () const
>> +  user_selected_what what_changed () const
>>     {
>> +    /* If anything changed then we report both the thread and frame at a
>> +       minimum.  We optionally add the inferior if we know that it
>> +       changed.
>> +
>> +       This means that for pure frame changes, e.g. -stack-select-frame, we
>> +       still report both a thread and a frame, which isn't ideal, but
>> +       there's also the cases where -thread-select is used to re-select the
>> +       current thread, in this case we'd still like to see the thread
>> +       reported, at least, that's what we have historically done.  */
>> +    user_selected_what state
>> +      = USER_SELECTED_THREAD | USER_SELECTED_FRAME;
>> +
>>       /* Did the selected thread change?  */
>>       if (m_previous_ptid != null_ptid && inferior_ptid != null_ptid
>>   	&& m_previous_ptid != inferior_ptid)
>> -      return true;
>> +      {
>> +	/* Did the inferior change too?  */
>> +	if (m_previous_ptid.pid () != inferior_ptid.pid ())
>> +	  state |= USER_SELECTED_INFERIOR;
>> +
>> +	return state;
>> +      }
>>   
>>       /* Grab details of the currently selected frame, for comparison.  */
>>       frame_id current_frame_id;
>> @@ -1982,7 +2005,7 @@ struct user_selected_context
>>   
>>       /* Did the selected frame level change?  */
>>       if (current_frame_level != m_previous_frame_level)
>> -      return true;
>> +      return state;
>>   
>>       /* Did the selected frame id change?  If the innermost frame is
>>          selected then the level will be -1, and the frame-id will be
>> @@ -1991,10 +2014,10 @@ struct user_selected_context
>>          other than the innermost frame selected.  */
>>       if (current_frame_level != -1
>>   	&& current_frame_id != m_previous_frame_id)
>> -      return true;
>> +      return state;
>>   
>>       /* Nothing changed!  */
>> -    return false;
>> +    return 0;
>>     }
>>   private:
>>     /* The previously selected thread.  This might be null_ptid if there was
>> @@ -2098,10 +2121,13 @@ mi_cmd_execute (struct mi_parse *parse)
>>   
>>     parse->cmd->invoke (parse);
>>   
>> -  if (!parse->cmd->preserve_user_selected_context ()
>> -      && current_user_selected_context.has_changed ())
>> -    interps_notify_user_selected_context_changed
>> -      (USER_SELECTED_THREAD | USER_SELECTED_FRAME);
>> +  if (!parse->cmd->preserve_user_selected_context ())
>> +    {
>> +      user_selected_what what
>> +	= current_user_selected_context.what_changed ();
>> +      if (what != 0)
>> +	notify_user_selected_context_changed (what);
>> +    }
>>   }
>>   
>>   /* See mi-main.h.  */
>> diff --git a/gdb/testsuite/gdb.mi/user-selected-context-sync.exp b/gdb/testsuite/gdb.mi/user-selected-context-sync.exp
>> index 3434ffa2a46..4f02b950f6e 100644
>> --- a/gdb/testsuite/gdb.mi/user-selected-context-sync.exp
>> +++ b/gdb/testsuite/gdb.mi/user-selected-context-sync.exp
>> @@ -698,9 +698,21 @@ proc_with_prefix test_cli_thread { mode } {
>>   	}
>>       }
>>   
>> -    # Idea for the future: selecting a thread in a different inferior.  For now,
>> -    # GDB doesn't show an inferior switch, but if it did, it would be a nice
>> -    # place to test it.
>> +    with_test_prefix "thread 2.2" {
>> +	# Select a thread in a different inferior.  This should trigger an
>> +	# 'inferior changed' and 'thread changed' notification on the CLI,
>> +	# and a single MI async notification.
>> +	set mi_re [make_mi_re $mode 5 0 event]
>> +	set cli_re [make_cli_re $mode 2 2.2 0]
>> +
>> +	with_spawn_id $gdb_main_spawn_id {
>> +	    gdb_test "thread 2.2" $cli_re "select thread"
>> +	}
>> +
>> +	with_spawn_id $mi_spawn_id {
>> +	    match_re_or_ensure_no_output $mi_re "select thread, event on MI"
>> +	}
>> +    }
>>   }
>>   
>>   # Test frame selection from CLI.
>> @@ -995,9 +1007,22 @@ proc_with_prefix test_mi_thread_select { mode } {
>>   	}
>>       }
>>   
>> -    # Idea for the future: selecting a thread in a different inferior.  For now,
>> -    # GDB doesn't show an inferior switch, but if it did, it would be a nice
>> -    # place to test it.
>> +    with_test_prefix "thread 2.2" {
>> +	# Select a thread in a different inferior.  This should trigger an
>> +	# 'inferior changed' and 'thread changed' notification on the CLI,
>> +	# and a single MI async notification.
>> +	set mi_re [make_mi_re $mode 5 0 response]
>> +	set cli_re [make_cli_re $mode 2 2.2 0]
>> +
>> +	with_spawn_id $mi_spawn_id {
>> +	    mi_gdb_test "-thread-select 5" $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"
>> +	}
>> +
>> +    }
>>   }
>>   
>>   proc_with_prefix test_mi_stack_select_frame { mode } {
>> @@ -1290,9 +1315,22 @@ proc_with_prefix test_cli_in_mi_thread { mode cli_in_mi_mode } {
>>   	}
>>       }
>>   
>> -    # Idea for the future: selecting a thread in a different inferior.  For now,
>> -    # GDB doesn't show an inferior switch, but if it did, it would be a nice
>> -    # place to test it.
>> +    with_test_prefix "thread 2.2" {
>> +	# Select a thread in a different inferior.  This should trigger an
>> +	# 'inferior changed' and 'thread changed' notification on the CLI,
>> +	# and a single MI async notification.
>> +	set command [make_cli_in_mi_command $cli_in_mi_mode "thread 2.2"]
>> +	set mi_re [make_cli_in_mi_re $command $cli_in_mi_mode $mode 1 2 2.2 5 0]
>> +	set cli_re [make_cli_re $mode 2 2.2 0]
>> +
>> +	with_spawn_id $mi_spawn_id {
>> +	    mi_gdb_test $command $mi_re "select thread"
>> +	}
>> +
>> +	with_spawn_id $gdb_main_spawn_id {
>> +	    match_re_or_ensure_no_output "$cli_re\r\n" "select thread, event on CLI"
>> +	}
>> +    }
>>   }
>>   
>>   # Test selecting the frame using a CLI command in the MI channel.
>> diff --git a/gdb/thread.c b/gdb/thread.c
>> index 0788bea235a..9ca1a8d973a 100644
>> --- a/gdb/thread.c
>> +++ b/gdb/thread.c
>> @@ -1983,10 +1983,19 @@ thread_command (const char *tidstr, int from_tty)
>>       }
>>     else
>>       {
>> +      inferior *previous_inferior = current_inferior ();
>> +
>>         thread_select (tidstr, parse_thread_id (tidstr, NULL));
>>   
>> -      notify_user_selected_context_changed
>> -	(USER_SELECTED_THREAD | USER_SELECTED_FRAME);
>> +      user_selected_what selection = (USER_SELECTED_THREAD
>> +				      | USER_SELECTED_FRAME);
>> +
>> +      /* If the inferior changed as a consequence of the thread change,
>> +	 then let the user know.  */
>> +      if (previous_inferior != current_inferior ())
>> +	selection |= USER_SELECTED_INFERIOR;
>> +
>> +      notify_user_selected_context_changed (selection);
>>       }
>>   }
>>   
>>
>> base-commit: 449035c35f2169e0c690d83f28306275ab7f7463
>
>
> -- 
> Cheers,
> Guinevere Larsen
> It/she