[binutils-gdb] gdb: don't use the global thread-id in the saved breakpoints file

[binutils-gdb] gdb: don't use the global thread-id in the saved breakpoints file

[Andrew Burgess]

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=442716d400655e252f3faf93ae17ec410e73869d

commit 442716d400655e252f3faf93ae17ec410e73869d
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:
---
 gdb/breakpoint.c                               |  5 ++-
 gdb/gdbthread.h                                |  4 +++
 gdb/testsuite/gdb.base/save-bp.exp             | 16 +++++++++
 gdb/testsuite/gdb.multi/bp-thread-specific.exp | 46 ++++++++++++++++++++++++++
 gdb/thread.c                                   | 19 ++++++++---
 5 files changed, 85 insertions(+), 5 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 51bea46256f..7228acfd8fe 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -14142,7 +14142,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 e4d98ce966a..3d4cba32303 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -1431,14 +1431,25 @@ 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 ();
+
   gdb_assert (thr != nullptr);
+  xsnprintf (s, PRINT_CELL_SIZE, "%d", thr->per_inf_num);
+  return s;
+}
 
+/* See gdbthread.h.  */
+
+const char *
+print_full_thread_id (struct thread_info *thr)
+{
   char *s = get_print_cell ();
 
-  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);
+  gdb_assert (thr != nullptr);
+  xsnprintf (s, PRINT_CELL_SIZE, "%d.%d", thr->inf->num, thr->per_inf_num);
   return s;
 }