[3/3] 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".

This commit updates breakpoint::print_recreate_thread to use the
print_thread_id function, and adds a test for this change.
---
 gdb/breakpoint.c                              |  5 +-
 .../gdb.multi/bp-thread-specific.exp          | 46 +++++++++++++++++++
 2 files changed, 50 insertions(+), 1 deletion(-)
  

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.

Otherwise,

 Approved-By: Pedro Alves <pedro@palves.net>
  

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.

Thanks,
Andrew

>
> Otherwise,
>
>  Approved-By: Pedro Alves <pedro@palves.net>
  

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.

Pedro Alves

> Thanks,
> Andrew
> 
>>
>> Otherwise,
>>
>>  Approved-By: Pedro Alves <pedro@palves.net>
>
  

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.

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;
 }
  

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;
>  }
>
  

On 2023-03-16 5:06 p.m., Andrew Burgess wrote:
> Andrew Burgess <aburgess@redhat.com> writes:
> 
>> Pedro Alves <pedro@palves.net> writes:
>>>
>>> On 2023-02-10 7:22 p.m., Andrew Burgess wrote:
>>>> Pedro Alves <pedro@palves.net> writes:

>>> 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?

Sorry for the delay.  (I simply forgot to reply.)  It looks good to me.

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

Pedro Alves <pedro@palves.net> writes:

> On 2023-03-16 5:06 p.m., Andrew Burgess wrote:
>> Andrew Burgess <aburgess@redhat.com> writes:
>> 
>>> Pedro Alves <pedro@palves.net> writes:
>>>>
>>>> On 2023-02-10 7:22 p.m., Andrew Burgess wrote:
>>>>> Pedro Alves <pedro@palves.net> writes:
>
>>>> 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?
>
> Sorry for the delay.  (I simply forgot to reply.)  It looks good to me.
>

Not a problem.  Now pushed.

Thanks,
Andrew