Re: [PATCH 3/3] gdb: don't use the global thread-id in the saved breakpoints file

[Andrew Burgess <aburgess@redhat.com>]

Andrew Burgess <aburgess@redhat.com> writes:

> Pedro Alves <pedro@palves.net> writes:
>
>> Hi!
>>
>> On 2023-02-10 7:22 p.m., Andrew Burgess wrote:
>>> Pedro Alves <pedro@palves.net> writes:
>>> 
>>>> On 2023-02-08 3:23 p.m., Andrew Burgess via Gdb-patches wrote:
>>>>
>>>>>  breakpoint::print_recreate_thread (struct ui_file *fp) const
>>>>>  {
>>>>>    if (thread != -1)
>>>>> -    gdb_printf (fp, " thread %d", thread);
>>>>> +    {
>>>>> +      struct thread_info *thr = find_thread_global_id (thread);
>>>>> +      gdb_printf (fp, " thread %s", print_thread_id (thr));
>>>>
>>>> print_thread_id only prints the inferior-qualified thread id if
>>>> there are multiple inferiors.  I am wondering whether the save breakpoints
>>>> file should _always_ end up with inferior-qualified thread ids, so that
>>>> reloading the saved file works the same if you meanwhile add another
>>>> inferior.
>>> 
>>> As a counter argument; if the user has a single inferior and places
>>> breakpoints on a particular thread, we'll have a save like:
>>> 
>>>   break foo thread 2
>>> 
>>> Then if the user sets up two inferiors, they can select which inferior
>>> the breakpoints should apply to - source the saves from inferior 2, and
>>> the b/p will apply to inferior 2 threads, source from inferior 1, and
>>> the b/p will apply to inferior 1 threads.
>>> 
>>> If the user has changed the inferior setup when sourcing the breakpoint
>>> save file, I think they have to take some responsibility for knowing
>>> what they want ... maybe?
>>> 
>>> If you feel strongly then it's easy enough to print the qualified
>>> thread-id, just let me know and I'll get it done.
>>> 
>>
>> My thinking is that internally, the thread is really inferior-qualified.
>> It is just a presentation detail in the CLI that we don't print the
>> inferior when there's only one inferior, for backwards compatibility.
>> That may even change in the future.  An MI frontend / GUI may be presenting
>> the qualified ID, for instance.
>>
>> It seems to be that there are two valid approaches:
>>
>> #1 - we consider what the user typed when the breakpoint was created as canonical,
>>      and thus we save the breakpoint using the same breakpoint spec string that
>>      user typed originally, meaning, if the user typed:
>>
>>        "break foo thread 1"
>>
>>      then that's what we'd save, even if the user added a second
>>      inferior after creating the breakpoint.
>>
>>      Of course, it follows then that if the breakpoint is created with
>>
>>        "break foo thread 1.1"
>>
>>      then that's what we save.  So the user would have the option.
>>
>>      I'm really not sure whether this is an option that we should be giving
>>      users, though.  What if the breakpoint was created via Python, or via the
>>      MI with --thread ?  Then the concept of original "thread" may not even exists,
>>      even though we save such a breakpoint too.
>>
>> #2 - we consider that the thread that the breakpoint ended up bound to is what
>>      is canonical and thus we always print the qualified id to the file.
>>
>> The approach in your patch is neither of the above -- it prints the qualified
>> or non-qualified thread id depending on a CLI presentation detail, which seems
>> brittle to me.
>>
>> Option #2 seems the simplest to explain, document, and implement, to me,
>> but I could be convinced to go with #1 too.
>
> Thanks for the explanation.  I've implemented #2 in the patch below,
> what are your thoughts?
>
> Thanks,
> Andrew
>
> ---
>
> commit 868a074345bb6d20d9a64470936d699c8a123894
> Author: Andrew Burgess <aburgess@redhat.com>
> Date:   Wed Feb 8 13:56:22 2023 +0000
>
>     gdb: don't use the global thread-id in the saved breakpoints file
>     
>     I noticed that breakpoint::print_recreate_thread was printing the
>     global thread-id.  This function is used to implement the 'save
>     breakpoints' command, and should be writing out suitable CLI commands
>     for recreating the current breakpoints.  The CLI does not use global
>     thread-ids, but instead uses the inferior specific thread-ids,
>     e.g. "2.1".
>     
>     After some discussion on the mailing list it was suggested that the
>     most consistent solution would be for the saved breakpoints file to
>     always contain the inferior-qualified thread-id, so the file would
>     include "thread 1.1" instead of just "thread 1", even when there is
>     only a single inferior.
>     
>     So, this commit adds print_full_thread_id, which is just like the
>     existing print_thread_id, only it always prints the inferior-qualified
>     thread-id.
>     
>     I then update the existing print_thread_id to make use of this new
>     function, and finally, I update  breakpoint::print_recreate_thread to
>     also use this new function.
>     
>     There's a multi-inferior test that confirms the saved breakpoints file
>     correctly includes the fully-qualified thread-id, and I've also
>     updated the single inferior test gdb.base/save-bp.exp to have it
>     validate that the saved breakpoints file includes the
>     inferior-qualified thread-id, even for this single inferior case.

I'm planning to push this patch some time next week unless someone
objects.

Thanks,
Andrew

>
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index 0db3adaf916..6b616be547a 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -14141,7 +14141,10 @@ void
>  breakpoint::print_recreate_thread (struct ui_file *fp) const
>  {
>    if (thread != -1)
> -    gdb_printf (fp, " thread %d", thread);
> +    {
> +      struct thread_info *thr = find_thread_global_id (thread);
> +      gdb_printf (fp, " thread %s", print_full_thread_id (thr));
> +    }
>  
>    if (task != -1)
>      gdb_printf (fp, " task %d", task);
> diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
> index c0f27a8a66e..848daa94410 100644
> --- a/gdb/gdbthread.h
> +++ b/gdb/gdbthread.h
> @@ -661,6 +661,10 @@ extern int show_inferior_qualified_tids (void);
>     circular static buffer, NUMCELLS deep.  */
>  const char *print_thread_id (struct thread_info *thr);
>  
> +/* Like print_thread_id, but always prints the inferior-qualified form,
> +   even when there is only a single inferior.  */
> +const char *print_full_thread_id (struct thread_info *thr);
> +
>  /* Boolean test for an already-known ptid.  */
>  extern bool in_thread_list (process_stratum_target *targ, ptid_t ptid);
>  
> diff --git a/gdb/testsuite/gdb.base/save-bp.exp b/gdb/testsuite/gdb.base/save-bp.exp
> index 41d71837fb6..68933d36427 100644
> --- a/gdb/testsuite/gdb.base/save-bp.exp
> +++ b/gdb/testsuite/gdb.base/save-bp.exp
> @@ -89,3 +89,19 @@ gdb_test_sequence "info break" "info break" [list				\
>    "\[\r\n\]+\[ \t\]+printf"							\
>    "\[\r\n\]+$disabled_row_start main at \[^\r\n\]*$srcfile:$loc_bp8"		\
>  ]
> +
> +# Copy the saved breakpoints file to the local machine (if necessary),
> +# and then check its contents.
> +if {[is_remote host]} {
> +    set bps [remote_upload host bps [standard_output_file bps]]
> +}
> +set fh [open $bps]
> +set lines [split [read $fh] "\n"]
> +close $fh
> +
> +with_test_prefix "in bps file" {
> +    gdb_assert {[lsearch -regexp $lines "break ${srcfile}:${loc_bp2}$"] != -1} \
> +	"check for general breakpoint"
> +    gdb_assert {[lsearch -regexp $lines "break ${srcfile}:${loc_bp3} thread 1\\.1"] != -1} \
> +	"check for thread-specific breakpoint"
> +}
> diff --git a/gdb/testsuite/gdb.multi/bp-thread-specific.exp b/gdb/testsuite/gdb.multi/bp-thread-specific.exp
> index 777fcf85ab0..85c08f44a2c 100644
> --- a/gdb/testsuite/gdb.multi/bp-thread-specific.exp
> +++ b/gdb/testsuite/gdb.multi/bp-thread-specific.exp
> @@ -15,6 +15,9 @@
>  
>  # Check that GDB uses the correct thread-id when describing multiple
>  # thread specific breakpoints at the same location.
> +#
> +# Also check that the correct thread-ids are used in the saved
> +# breakpoints file.
>  
>  # The plain remote target can't do multiple inferiors.
>  require !use_gdb_stub
> @@ -59,3 +62,46 @@ gdb_test "break foo thread 1.1" \
>  	 "Note: breakpoint $bpnum \\(thread 2.1\\) also set at pc $hex\\." \
>  	 "Note: breakpoint $bpnum \\(thread 2.1\\) also set at pc $hex\\." \
>  	 "Breakpoint $decimal at $hex: foo\\. \\(2 locations\\)"]
> +
> +# Save the breakpoints into a file.
> +if {[is_remote host]} {
> +    set bps bps
> +} else {
> +    set bps [standard_output_file bps]
> +}
> +
> +remote_file host delete "$bps"
> +gdb_test "save breakpoints $bps" "" "save breakpoint to bps"
> +
> +if {[is_remote host]} {
> +    set bps [remote_upload host bps [standard_output_file bps]]
> +}
> +
> +# Now dig through the saved breakpoints file and check that the
> +# thread-ids were written out correctly.  First open the saved
> +# breakpoints and read them into a list.
> +set fh [open $bps]
> +set lines [split [read $fh] "\n"]
> +close $fh
> +
> +# Except the list created from the saved breakpoints file will have a
> +# blank line entry at the end, so remove it now.
> +gdb_assert {[string equal [lindex $lines end] ""]} \
> +    "check last item was an empty line"
> +set lines [lrange $lines 0 end-1]
> +
> +# These are the lines we expect in the saved breakpoints file, in the
> +# order that we expect them.  These are strings, not regexps.
> +set expected_results \
> +    [list \
> +	 "break -qualified main" \
> +	 "break foo thread 2.1" \
> +	 "break foo thread 1.1"]
> +
> +# Now check that the files contents (in LINES) matches the
> +# EXPECTED_RESULTS.
> +gdb_assert {[llength $lines] == [llength $expected_results]} \
> +    "correct number of lines in saved breakpoints file"
> +foreach a $lines b $expected_results {
> +    gdb_assert {[string equal $a $b]} "line '$b'"
> +}
> diff --git a/gdb/thread.c b/gdb/thread.c
> index 1a852f70206..9ba383d9bee 100644
> --- a/gdb/thread.c
> +++ b/gdb/thread.c
> @@ -1431,12 +1431,22 @@ show_inferior_qualified_tids (void)
>  const char *
>  print_thread_id (struct thread_info *thr)
>  {
> +  if (show_inferior_qualified_tids ())
> +    return print_full_thread_id (thr);
> +
>    char *s = get_print_cell ();
> +  xsnprintf (s, PRINT_CELL_SIZE, "%d", thr->per_inf_num);
> +  return s;
> +}
>  
> -  if (show_inferior_qualified_tids ())
> -    xsnprintf (s, PRINT_CELL_SIZE, "%d.%d", thr->inf->num, thr->per_inf_num);
> -  else
> -    xsnprintf (s, PRINT_CELL_SIZE, "%d", thr->per_inf_num);
> +/* See gdbthread.h.  */
> +
> +const char *
> +print_full_thread_id (struct thread_info *thr)
> +{
> +  char *s = get_print_cell ();
> +
> +  xsnprintf (s, PRINT_CELL_SIZE, "%d.%d", thr->inf->num, thr->per_inf_num);
>    return s;
>  }
>