* [PATCH 0/3] Avoid printing global thread-id in CLI command output @ 2023-02-08 15:23 Andrew Burgess 2023-02-08 15:23 ` [PATCH 1/3] gdb: don't print global thread-id to CLI in describe_other_breakpoints Andrew Burgess ` (2 more replies) 0 siblings, 3 replies; 15+ messages in thread From: Andrew Burgess @ 2023-02-08 15:23 UTC (permalink / raw) To: gdb-patches; +Cc: 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 1/3] gdb: don't print global thread-id to CLI in describe_other_breakpoints 2023-02-08 15:23 [PATCH 0/3] Avoid printing global thread-id in CLI command output Andrew Burgess @ 2023-02-08 15:23 ` Andrew Burgess 2023-02-08 17:55 ` Pedro Alves 2023-02-08 15:23 ` [PATCH 2/3] gdb: show task number " Andrew Burgess 2023-02-08 15:23 ` [PATCH 3/3] gdb: don't use the global thread-id in the saved breakpoints file Andrew Burgess 2 siblings, 1 reply; 15+ messages in thread From: Andrew Burgess @ 2023-02-08 15:23 UTC (permalink / raw) To: gdb-patches; +Cc: 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] gdb: don't print global thread-id to CLI in describe_other_breakpoints 2023-02-08 15:23 ` [PATCH 1/3] gdb: don't print global thread-id to CLI in describe_other_breakpoints Andrew Burgess @ 2023-02-08 17:55 ` Pedro Alves 2023-02-11 17:41 ` Andrew Burgess 0 siblings, 1 reply; 15+ messages in thread From: Pedro Alves @ 2023-02-08 17:55 UTC (permalink / raw) To: Andrew Burgess, gdb-patches 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> ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] gdb: don't print global thread-id to CLI in describe_other_breakpoints 2023-02-08 17:55 ` Pedro Alves @ 2023-02-11 17:41 ` Andrew Burgess 0 siblings, 0 replies; 15+ messages in thread From: Andrew Burgess @ 2023-02-11 17:41 UTC (permalink / raw) To: Pedro Alves, gdb-patches 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 2/3] gdb: show task number in describe_other_breakpoints 2023-02-08 15:23 [PATCH 0/3] Avoid printing global thread-id in CLI command output Andrew Burgess 2023-02-08 15:23 ` [PATCH 1/3] gdb: don't print global thread-id to CLI in describe_other_breakpoints Andrew Burgess @ 2023-02-08 15:23 ` Andrew Burgess 2023-02-08 17:55 ` Pedro Alves 2023-02-08 15:23 ` [PATCH 3/3] gdb: don't use the global thread-id in the saved breakpoints file Andrew Burgess 2 siblings, 1 reply; 15+ messages in thread From: Andrew Burgess @ 2023-02-08 15:23 UTC (permalink / raw) To: gdb-patches; +Cc: 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/3] gdb: show task number in describe_other_breakpoints 2023-02-08 15:23 ` [PATCH 2/3] gdb: show task number " Andrew Burgess @ 2023-02-08 17:55 ` Pedro Alves 2023-02-11 17:42 ` Andrew Burgess 0 siblings, 1 reply; 15+ messages in thread From: Pedro Alves @ 2023-02-08 17:55 UTC (permalink / raw) To: Andrew Burgess, gdb-patches 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" > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/3] gdb: show task number in describe_other_breakpoints 2023-02-08 17:55 ` Pedro Alves @ 2023-02-11 17:42 ` Andrew Burgess 0 siblings, 0 replies; 15+ messages in thread From: Andrew Burgess @ 2023-02-11 17:42 UTC (permalink / raw) To: Pedro Alves, gdb-patches 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 3/3] gdb: don't use the global thread-id in the saved breakpoints file 2023-02-08 15:23 [PATCH 0/3] Avoid printing global thread-id in CLI command output Andrew Burgess 2023-02-08 15:23 ` [PATCH 1/3] gdb: don't print global thread-id to CLI in describe_other_breakpoints Andrew Burgess 2023-02-08 15:23 ` [PATCH 2/3] gdb: show task number " Andrew Burgess @ 2023-02-08 15:23 ` Andrew Burgess 2023-02-08 17:55 ` Pedro Alves 2 siblings, 1 reply; 15+ messages in thread From: Andrew Burgess @ 2023-02-08 15:23 UTC (permalink / raw) To: gdb-patches; +Cc: 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/3] gdb: don't use the global thread-id in the saved breakpoints file 2023-02-08 15:23 ` [PATCH 3/3] gdb: don't use the global thread-id in the saved breakpoints file Andrew Burgess @ 2023-02-08 17:55 ` Pedro Alves 2023-02-10 19:22 ` Andrew Burgess 0 siblings, 1 reply; 15+ messages in thread From: Pedro Alves @ 2023-02-08 17:55 UTC (permalink / raw) To: Andrew Burgess, gdb-patches 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> ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/3] gdb: don't use the global thread-id in the saved breakpoints file 2023-02-08 17:55 ` Pedro Alves @ 2023-02-10 19:22 ` Andrew Burgess 2023-02-17 17:49 ` Pedro Alves 0 siblings, 1 reply; 15+ messages in thread From: Andrew Burgess @ 2023-02-10 19:22 UTC (permalink / raw) To: Pedro Alves, gdb-patches 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> ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/3] gdb: don't use the global thread-id in the saved breakpoints file 2023-02-10 19:22 ` Andrew Burgess @ 2023-02-17 17:49 ` Pedro Alves 2023-02-27 19:45 ` Andrew Burgess 0 siblings, 1 reply; 15+ messages in thread From: Pedro Alves @ 2023-02-17 17:49 UTC (permalink / raw) To: Andrew Burgess, gdb-patches 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> > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/3] gdb: don't use the global thread-id in the saved breakpoints file 2023-02-17 17:49 ` Pedro Alves @ 2023-02-27 19:45 ` Andrew Burgess 2023-03-16 17:06 ` Andrew Burgess 0 siblings, 1 reply; 15+ messages in thread From: Andrew Burgess @ 2023-02-27 19:45 UTC (permalink / raw) To: Pedro Alves, gdb-patches 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; } ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/3] gdb: don't use the global thread-id in the saved breakpoints file 2023-02-27 19:45 ` Andrew Burgess @ 2023-03-16 17:06 ` Andrew Burgess 2023-03-17 18:01 ` Pedro Alves 0 siblings, 1 reply; 15+ messages in thread From: Andrew Burgess @ 2023-03-16 17:06 UTC (permalink / raw) To: Pedro Alves, gdb-patches 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; > } > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/3] gdb: don't use the global thread-id in the saved breakpoints file 2023-03-16 17:06 ` Andrew Burgess @ 2023-03-17 18:01 ` Pedro Alves 2023-03-20 10:38 ` Andrew Burgess 0 siblings, 1 reply; 15+ messages in thread From: Pedro Alves @ 2023-03-17 18:01 UTC (permalink / raw) To: Andrew Burgess, gdb-patches 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. > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/3] gdb: don't use the global thread-id in the saved breakpoints file 2023-03-17 18:01 ` Pedro Alves @ 2023-03-20 10:38 ` Andrew Burgess 0 siblings, 0 replies; 15+ messages in thread From: Andrew Burgess @ 2023-03-20 10:38 UTC (permalink / raw) To: Pedro Alves, gdb-patches 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2023-03-20 10:38 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-02-08 15:23 [PATCH 0/3] Avoid printing global thread-id in CLI command output Andrew Burgess 2023-02-08 15:23 ` [PATCH 1/3] gdb: don't print global thread-id to CLI in describe_other_breakpoints Andrew Burgess 2023-02-08 17:55 ` Pedro Alves 2023-02-11 17:41 ` Andrew Burgess 2023-02-08 15:23 ` [PATCH 2/3] gdb: show task number " Andrew Burgess 2023-02-08 17:55 ` Pedro Alves 2023-02-11 17:42 ` Andrew Burgess 2023-02-08 15:23 ` [PATCH 3/3] gdb: don't use the global thread-id in the saved breakpoints file Andrew Burgess 2023-02-08 17:55 ` Pedro Alves 2023-02-10 19:22 ` Andrew Burgess 2023-02-17 17:49 ` Pedro Alves 2023-02-27 19:45 ` Andrew Burgess 2023-03-16 17:06 ` Andrew Burgess 2023-03-17 18:01 ` Pedro Alves 2023-03-20 10:38 ` Andrew Burgess
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).