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

[Andrew Burgess <aburgess@redhat.com>]

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