[PATCH 0/3] Avoid printing global thread-id in CLI command output

[PATCH 0/3] Avoid printing global thread-id in CLI command output

[Andrew Burgess]

After fixing PR gdb/30087 I took a look through the rest of
breakpoint.c looking for places where we might be printing the global
thread-id.  I found two places (patches #1 and #3).

While working on patch #1 I spotted that we handle the thread-id, but
not the task-id, so patch #2 fixes that.

---

Andrew Burgess (3):
  gdb: don't print global thread-id to CLI in describe_other_breakpoints
  gdb: show task number in describe_other_breakpoints
  gdb: don't use the global thread-id in the saved breakpoints file

 gdb/breakpoint.c                              |  12 +-
 gdb/testsuite/gdb.ada/tasks.exp               |  17 ++-
 gdb/testsuite/gdb.multi/bp-thread-specific.c  |  28 +++++
 .../gdb.multi/bp-thread-specific.exp          | 107 ++++++++++++++++++
 4 files changed, 156 insertions(+), 8 deletions(-)
 create mode 100644 gdb/testsuite/gdb.multi/bp-thread-specific.c
 create mode 100644 gdb/testsuite/gdb.multi/bp-thread-specific.exp


base-commit: 1947a4a4bb7651a4656edceb1f9b246f96f89ebd
-- 
2.25.4

[PATCH 1/3] gdb: don't print global thread-id to CLI in describe_other_breakpoints

[Andrew Burgess]

I noticed that describe_other_breakpoints was printing the global
thread-id to the CLI.  For CLI output we should be printing the
inferior local thread-id (e.g. "2.1").  This can be seen in the
following GDB session:

  (gdb) info threads
    Id   Target Id                                Frame
    1.1  Thread 4065742.4065742 "bp-thread-speci" main () at /tmp/bp-thread-specific.c:27
  * 2.1  Thread 4065743.4065743 "bp-thread-speci" main () at /tmp/bp-thread-specific.c:27
  (gdb) break foo thread 2.1
  Breakpoint 3 at 0x40110a: foo. (2 locations)
  (gdb) break foo thread 1.1
  Note: breakpoint 3 (thread 2) also set at pc 0x40110a.
  Note: breakpoint 3 (thread 2) also set at pc 0x40110a.
  Breakpoint 4 at 0x40110a: foo. (2 locations)

Notice that GDB says:

  Note: breakpoint 3 (thread 2) also set at pc 0x40110a.

The 'thread 2' in here is using the global thread-id, we should
instead say 'thread 2.1' which corresponds to how the user specified
the breakpoint.

This commit fixes this issue and adds a test.
---
 gdb/breakpoint.c                              |  5 +-
 gdb/testsuite/gdb.multi/bp-thread-specific.c  | 28 +++++++++
 .../gdb.multi/bp-thread-specific.exp          | 61 +++++++++++++++++++
 3 files changed, 93 insertions(+), 1 deletion(-)
 create mode 100644 gdb/testsuite/gdb.multi/bp-thread-specific.c
 create mode 100644 gdb/testsuite/gdb.multi/bp-thread-specific.exp

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index adf38e7d722..701555a060e 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -7045,7 +7045,10 @@ describe_other_breakpoints (struct gdbarch *gdbarch,
 	    if (b->thread == -1 && thread != -1)
 	      gdb_printf (" (all threads)");
 	    else if (b->thread != -1)
-	      gdb_printf (" (thread %d)", b->thread);
+	      {
+		struct thread_info *thr = find_thread_global_id (b->thread);
+		gdb_printf (" (thread %s)", print_thread_id (thr));
+	      }
 	    gdb_printf ("%s%s ",
 			((b->enable_state == bp_disabled
 			  || b->enable_state == bp_call_disabled)
diff --git a/gdb/testsuite/gdb.multi/bp-thread-specific.c b/gdb/testsuite/gdb.multi/bp-thread-specific.c
new file mode 100644
index 00000000000..51b6492085f
--- /dev/null
+++ b/gdb/testsuite/gdb.multi/bp-thread-specific.c
@@ -0,0 +1,28 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2023 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+int
+foo ()
+{
+  return 0;
+}
+
+int
+main ()
+{
+  return foo ();
+}
diff --git a/gdb/testsuite/gdb.multi/bp-thread-specific.exp b/gdb/testsuite/gdb.multi/bp-thread-specific.exp
new file mode 100644
index 00000000000..777fcf85ab0
--- /dev/null
+++ b/gdb/testsuite/gdb.multi/bp-thread-specific.exp
@@ -0,0 +1,61 @@
+# Copyright 2023 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Check that GDB uses the correct thread-id when describing multiple
+# thread specific breakpoints at the same location.
+
+# The plain remote target can't do multiple inferiors.
+require !use_gdb_stub
+
+standard_testfile
+
+if {[prepare_for_testing "failed to prepare" $testfile $srcfile]} {
+    return -1
+}
+
+if {![runto_main]} {
+    return -1
+}
+
+gdb_test "add-inferior -exec ${binfile}" "Added inferior 2.*" "add inferior 2"
+gdb_test "inferior 2"
+
+if {![runto_main]} {
+    return -1
+}
+
+gdb_test "info threads" \
+    [multi_line \
+	 "  Id\\s+Target Id\\s+Frame\\s*" \
+	 "  1\\.1\\s+\[^\r\n\]+" \
+	 "\\* 2\\.1\\s+\[^\r\n\]+"] \
+    "check we have the expected threads"
+
+# Set the first breakpoint.  Currently this is going to insert at two
+# locations ('foo' in both inferiors) even though only one of those
+# locations will ever trigger ('foo' in inferior 2).
+gdb_test "break foo thread 2.1" \
+    "Breakpoint $decimal at $hex: foo\\. \\(2 locations\\)"
+
+set bpnum [get_integer_valueof "\$bpnum" "INVALID"]
+
+# Now set another breakpoint that will be at the same location as the
+# earlier breakpoint.  Check that the thread-id used when describing
+# the earlier breakpoints is correct.
+gdb_test "break foo thread 1.1" \
+    [multi_line \
+	 "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\\)"]
-- 
2.25.4

Re: [PATCH 1/3] gdb: don't print global thread-id to CLI in describe_other_breakpoints

[Pedro Alves]

On 2023-02-08 3:23 p.m., Andrew Burgess via Gdb-patches wrote:
> I noticed that describe_other_breakpoints was printing the global
> thread-id to the CLI.  For CLI output we should be printing the
> inferior local thread-id (e.g. "2.1").  This can be seen in the
> following GDB session:
> 
>   (gdb) info threads
>     Id   Target Id                                Frame
>     1.1  Thread 4065742.4065742 "bp-thread-speci" main () at /tmp/bp-thread-specific.c:27
>   * 2.1  Thread 4065743.4065743 "bp-thread-speci" main () at /tmp/bp-thread-specific.c:27
>   (gdb) break foo thread 2.1
>   Breakpoint 3 at 0x40110a: foo. (2 locations)
>   (gdb) break foo thread 1.1
>   Note: breakpoint 3 (thread 2) also set at pc 0x40110a.
>   Note: breakpoint 3 (thread 2) also set at pc 0x40110a.
>   Breakpoint 4 at 0x40110a: foo. (2 locations)
> 
> Notice that GDB says:
> 
>   Note: breakpoint 3 (thread 2) also set at pc 0x40110a.
> 
> The 'thread 2' in here is using the global thread-id, we should
> instead say 'thread 2.1' which corresponds to how the user specified
> the breakpoint.
> 
> This commit fixes this issue and adds a test.

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

Re: [PATCH 1/3] gdb: don't print global thread-id to CLI in describe_other_breakpoints

[Andrew Burgess]

Pedro Alves <pedro@palves.net> writes:

> On 2023-02-08 3:23 p.m., Andrew Burgess via Gdb-patches wrote:
>> I noticed that describe_other_breakpoints was printing the global
>> thread-id to the CLI.  For CLI output we should be printing the
>> inferior local thread-id (e.g. "2.1").  This can be seen in the
>> following GDB session:
>> 
>>   (gdb) info threads
>>     Id   Target Id                                Frame
>>     1.1  Thread 4065742.4065742 "bp-thread-speci" main () at /tmp/bp-thread-specific.c:27
>>   * 2.1  Thread 4065743.4065743 "bp-thread-speci" main () at /tmp/bp-thread-specific.c:27
>>   (gdb) break foo thread 2.1
>>   Breakpoint 3 at 0x40110a: foo. (2 locations)
>>   (gdb) break foo thread 1.1
>>   Note: breakpoint 3 (thread 2) also set at pc 0x40110a.
>>   Note: breakpoint 3 (thread 2) also set at pc 0x40110a.
>>   Breakpoint 4 at 0x40110a: foo. (2 locations)
>> 
>> Notice that GDB says:
>> 
>>   Note: breakpoint 3 (thread 2) also set at pc 0x40110a.
>> 
>> The 'thread 2' in here is using the global thread-id, we should
>> instead say 'thread 2.1' which corresponds to how the user specified
>> the breakpoint.
>> 
>> This commit fixes this issue and adds a test.
>
> Approved-By: Pedro Alves <pedro@palves.net>

Pushed.

Thanks,
Andrew

[PATCH 2/3] gdb: show task number in describe_other_breakpoints

[Andrew Burgess]

I noticed that describe_other_breakpoints doesn't show the task
number, but does show the thread-id.  I can't see any reason why we'd
want to not show the task number in this situation, so this commit
adds this missing information, and extends gdb.ada/tasks.exp to check
this case.
---
 gdb/breakpoint.c                |  2 ++
 gdb/testsuite/gdb.ada/tasks.exp | 17 +++++++++++------
 2 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 701555a060e..6b576859592 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -7049,6 +7049,8 @@ describe_other_breakpoints (struct gdbarch *gdbarch,
 		struct thread_info *thr = find_thread_global_id (b->thread);
 		gdb_printf (" (thread %s)", print_thread_id (thr));
 	      }
+	    else if (b->task != 0)
+	      gdb_printf (" (task %d)", b->task);
 	    gdb_printf ("%s%s ",
 			((b->enable_state == bp_disabled
 			  || b->enable_state == bp_call_disabled)
diff --git a/gdb/testsuite/gdb.ada/tasks.exp b/gdb/testsuite/gdb.ada/tasks.exp
index 88ef123865b..83692054e4f 100644
--- a/gdb/testsuite/gdb.ada/tasks.exp
+++ b/gdb/testsuite/gdb.ada/tasks.exp
@@ -50,16 +50,21 @@ gdb_test "watch j task 1 task 3" "You can specify only one task\\."
 # breakpoint in the list that matched the triggered-breakpoint's
 # address, no matter which task it was specific to.
 gdb_test "break break_me task 1" "Breakpoint .* at .*"
+set bp_number [get_integer_valueof "\$bpnum" "INVALID" \
+		   "get number of breakpoint for task 1"]
 gdb_test "info breakpoints" "foo.adb:${decimal}\r\n\\s+stop only in task 1" \
     "check info breakpoints for task 1 breakpoint"
 
 # Now, insert a breakpoint that should stop only if task 3 stops, and
-# extract its number.
-gdb_breakpoint "break_me task 3" message
-set bp_number [get_integer_valueof "\$bpnum" -1]
-if {$bp_number < 0} {
-    return
-}
+# extract its number.  Use gdb_test here so that we can validate that
+# the 'Breakpoint ... also set at' line correctly includes the task
+# number of the prevoius breakpoint.
+gdb_test "break break_me task 3" \
+    [multi_line \
+	 "Note: breakpoint $bp_number \\(task 1\\) also set at pc $hex\\." \
+	 "Breakpoint $decimal at $hex: \[^\r\n\]+"]
+set bp_number [get_integer_valueof "\$bpnum" "INVALID" \
+		   "get number of breakpoint for task 3"]
 gdb_test "info breakpoints" "foo.adb:${decimal}\r\n\\s+stop only in task 3" \
     "check info breakpoints for task 3 breakpoint"
 
-- 
2.25.4

Re: [PATCH 2/3] gdb: show task number in describe_other_breakpoints

[Pedro Alves]

On 2023-02-08 3:23 p.m., Andrew Burgess via Gdb-patches wrote:

>  # Now, insert a breakpoint that should stop only if task 3 stops, and
> -# extract its number.
> -gdb_breakpoint "break_me task 3" message
> -set bp_number [get_integer_valueof "\$bpnum" -1]
> -if {$bp_number < 0} {
> -    return
> -}
> +# extract its number.  Use gdb_test here so that we can validate that
> +# the 'Breakpoint ... also set at' line correctly includes the task
> +# number of the prevoius breakpoint.

prevoius -> previous

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

Thanks,
Pedro Alves

> +gdb_test "break break_me task 3" \
> +    [multi_line \
> +	 "Note: breakpoint $bp_number \\(task 1\\) also set at pc $hex\\." \
> +	 "Breakpoint $decimal at $hex: \[^\r\n\]+"]
> +set bp_number [get_integer_valueof "\$bpnum" "INVALID" \
> +		   "get number of breakpoint for task 3"]
>  gdb_test "info breakpoints" "foo.adb:${decimal}\r\n\\s+stop only in task 3" \
>      "check info breakpoints for task 3 breakpoint"
>  
> 

Re: [PATCH 2/3] gdb: show task number in describe_other_breakpoints

[Andrew Burgess]

Pedro Alves <pedro@palves.net> writes:

> On 2023-02-08 3:23 p.m., Andrew Burgess via Gdb-patches wrote:
>
>>  # Now, insert a breakpoint that should stop only if task 3 stops, and
>> -# extract its number.
>> -gdb_breakpoint "break_me task 3" message
>> -set bp_number [get_integer_valueof "\$bpnum" -1]
>> -if {$bp_number < 0} {
>> -    return
>> -}
>> +# extract its number.  Use gdb_test here so that we can validate that
>> +# the 'Breakpoint ... also set at' line correctly includes the task
>> +# number of the prevoius breakpoint.
>
> prevoius -> previous
>
> Otherwise,
>  
>  Approved-By: Pedro Alves <pedro@palves.net>

Fixed the typo and pushed.

Thanks,
Andrew

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

[Andrew Burgess]

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(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 6b576859592..6e3c76305e7 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -14120,7 +14120,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_thread_id (thr));
+    }
 
   if (task != 0)
     gdb_printf (fp, " task %d", task);
diff --git a/gdb/testsuite/gdb.multi/bp-thread-specific.exp b/gdb/testsuite/gdb.multi/bp-thread-specific.exp
index 777fcf85ab0..6a7cd044af4 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 breakpoint $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'"
+}
-- 
2.25.4

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

[Pedro Alves]

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>

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

[Andrew Burgess]

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>

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

[Pedro Alves]

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

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

[Andrew Burgess]

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

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

[Andrew Burgess]

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

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

[Pedro Alves]

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

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

[Andrew Burgess]

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