[3/6] gdb: fix display of thread condition for multi-location breakpoints

Message ID 685ac9c3cc26e43c47b1a5fd3a59f7e97c5eca2b.1669634536.git.aburgess@redhat.com
State New
Headers
Series Inferior specific breakpoints |

Commit Message

Andrew Burgess Nov. 28, 2022, 11:25 a.m. UTC
  If a breakpoint with multiple locations has a thread condition, then
the 'info breakpoints' output is a little messed up, here's an example
of the current output:

  (gdb) break foo thread 1
  Breakpoint 2 at 0x401114: foo. (3 locations)
  (gdb) break bar thread 1
  Breakpoint 3 at 0x40110a: file /tmp/src/gdb/testsuite/gdb.base/thread-bp-multi-loc.c, line 32.
  (gdb) info breakpoints
  Num     Type           Disp Enb Address            What
  2       breakpoint     keep y   <MULTIPLE>          thread 1
          stop only in thread 1
  2.1                         y   0x0000000000401114 in foo at /tmp/src/gdb/testsuite/gdb.base/thread-bp-multi-loc.c:25
  2.2                         y   0x0000000000401146 in foo at /tmp/src/gdb/testsuite/gdb.base/thread-bp-multi-loc.c:25
  2.3                         y   0x0000000000401168 in foo at /tmp/src/gdb/testsuite/gdb.base/thread-bp-multi-loc.c:25
  3       breakpoint     keep y   0x000000000040110a in bar at /tmp/src/gdb/testsuite/gdb.base/thread-bp-multi-loc.c:32 thread 1
          stop only in thread 1

Notice that, at the end of the location for breakpoint 3, the 'thread
1' condition is printed.

In contrast, for breakpoint 2, the 'thread 1' appears randomly, in the
"What" column, though slightly offset.

I believe this is a bug in GDB, in the function
print_one_breakpoint_location, due to checking the local variable
part_of_multiple, instead of header_of_multiple.

If I fix this oversight, then the output is now:

  (gdb) break foo thread 1
  Breakpoint 2 at 0x401114: foo. (3 locations)
  (gdb) break bar thread 1
  Breakpoint 3 at 0x40110a: file /tmp/src/gdb/testsuite/gdb.base/thread-bp-multi-loc.c, line 32.
  (gdb) info breakpoints
  Num     Type           Disp Enb Address            What
  2       breakpoint     keep y   <MULTIPLE>
          stop only in thread 1
  2.1                         y   0x0000000000401114 in foo at /tmp/src/gdb/testsuite/gdb.base/thread-bp-multi-loc.c:25 thread 1
  2.2                         y   0x0000000000401146 in foo at /tmp/src/gdb/testsuite/gdb.base/thread-bp-multi-loc.c:25 thread 1
  2.3                         y   0x0000000000401168 in foo at /tmp/src/gdb/testsuite/gdb.base/thread-bp-multi-loc.c:25 thread 1
  3       breakpoint     keep y   0x000000000040110a in bar at /tmp/src/gdb/testsuite/gdb.base/thread-bp-multi-loc.c:32 thread 1
          stop only in thread 1

The 'thread 1' condition is now displayed at the end of each location,
which makes the output the same for single location breakpoints and
multi-location breakpoints.
---
 gdb/breakpoint.c                              |  2 +-
 gdb/testsuite/gdb.base/thread-bp-multi-loc.c  | 44 ++++++++++
 .../gdb.base/thread-bp-multi-loc.exp          | 88 +++++++++++++++++++
 3 files changed, 133 insertions(+), 1 deletion(-)
 create mode 100644 gdb/testsuite/gdb.base/thread-bp-multi-loc.c
 create mode 100644 gdb/testsuite/gdb.base/thread-bp-multi-loc.exp
  

Comments

Peikes, Wendy via Gdb-patches Dec. 23, 2022, 8:37 a.m. UTC | #1
On Monday, November 28, 2022 12:26 PM, Andrew Burgess wrote:
> If a breakpoint with multiple locations has a thread condition, then
> the 'info breakpoints' output is a little messed up, here's an example
> of the current output:
> 
>   (gdb) break foo thread 1
>   Breakpoint 2 at 0x401114: foo. (3 locations)
>   (gdb) break bar thread 1
>   Breakpoint 3 at 0x40110a: file /tmp/src/gdb/testsuite/gdb.base/thread-bp-multi-loc.c, line
> 32.
>   (gdb) info breakpoints
>   Num     Type           Disp Enb Address            What
>   2       breakpoint     keep y   <MULTIPLE>          thread 1
>           stop only in thread 1
>   2.1                         y   0x0000000000401114 in foo at
> /tmp/src/gdb/testsuite/gdb.base/thread-bp-multi-loc.c:25
>   2.2                         y   0x0000000000401146 in foo at
> /tmp/src/gdb/testsuite/gdb.base/thread-bp-multi-loc.c:25
>   2.3                         y   0x0000000000401168 in foo at
> /tmp/src/gdb/testsuite/gdb.base/thread-bp-multi-loc.c:25
>   3       breakpoint     keep y   0x000000000040110a in bar at
> /tmp/src/gdb/testsuite/gdb.base/thread-bp-multi-loc.c:32 thread 1
>           stop only in thread 1
> 
> Notice that, at the end of the location for breakpoint 3, the 'thread
> 1' condition is printed.
> 
> In contrast, for breakpoint 2, the 'thread 1' appears randomly, in the
> "What" column, though slightly offset.
> 
> I believe this is a bug in GDB, in the function
> print_one_breakpoint_location, due to checking the local variable
> part_of_multiple, instead of header_of_multiple.
> 
> If I fix this oversight, then the output is now:
> 
>   (gdb) break foo thread 1
>   Breakpoint 2 at 0x401114: foo. (3 locations)
>   (gdb) break bar thread 1
>   Breakpoint 3 at 0x40110a: file /tmp/src/gdb/testsuite/gdb.base/thread-bp-multi-loc.c, line
> 32.
>   (gdb) info breakpoints
>   Num     Type           Disp Enb Address            What
>   2       breakpoint     keep y   <MULTIPLE>
>           stop only in thread 1
>   2.1                         y   0x0000000000401114 in foo at
> /tmp/src/gdb/testsuite/gdb.base/thread-bp-multi-loc.c:25 thread 1
>   2.2                         y   0x0000000000401146 in foo at
> /tmp/src/gdb/testsuite/gdb.base/thread-bp-multi-loc.c:25 thread 1
>   2.3                         y   0x0000000000401168 in foo at
> /tmp/src/gdb/testsuite/gdb.base/thread-bp-multi-loc.c:25 thread 1
>   3       breakpoint     keep y   0x000000000040110a in bar at
> /tmp/src/gdb/testsuite/gdb.base/thread-bp-multi-loc.c:32 thread 1
>           stop only in thread 1

We already state the condition in "stop only in thread 1".
Do we need to repeat it also as part of the location?

> The 'thread 1' condition is now displayed at the end of each location,
> which makes the output the same for single location breakpoints and
> multi-location breakpoints.
> ---
>  gdb/breakpoint.c                              |  2 +-
>  gdb/testsuite/gdb.base/thread-bp-multi-loc.c  | 44 ++++++++++
>  .../gdb.base/thread-bp-multi-loc.exp          | 88 +++++++++++++++++++
>  3 files changed, 133 insertions(+), 1 deletion(-)
>  create mode 100644 gdb/testsuite/gdb.base/thread-bp-multi-loc.c
>  create mode 100644 gdb/testsuite/gdb.base/thread-bp-multi-loc.exp
> 
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index f0276a963c0..2ec8ca364b6 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -6467,7 +6467,7 @@ print_one_breakpoint_location (struct breakpoint *b,
>        output_thread_groups (uiout, "thread-groups", inf_nums, mi_only);
>      }
> 
> -  if (!part_of_multiple)
> +  if (!header_of_multiple)
>      {
>        if (b->thread != -1)
>  	{
> diff --git a/gdb/testsuite/gdb.base/thread-bp-multi-loc.c b/gdb/testsuite/gdb.base/thread-
> bp-multi-loc.c
> new file mode 100644
> index 00000000000..cab009c39ec
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/thread-bp-multi-loc.c
> @@ -0,0 +1,44 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2022 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/>.  */
> +
> +volatile int global_var = 0;
> +
> +__attribute__((__always_inline__)) static inline void
> +foo (void)
> +{
> +  int i;
> +
> +  for (i = 0; i < 10; ++i)
> +    global_var = i;
> +}
> +
> +static void
> +bar (void)
> +{
> +  global_var = 0;
> +  foo ();
> +}
> +
> +int
> +main (void)
> +{
> +  global_var = 0;
> +  foo ();
> +  bar ();
> +  foo ();
> +  return 0;
> +}
> diff --git a/gdb/testsuite/gdb.base/thread-bp-multi-loc.exp b/gdb/testsuite/gdb.base/thread-
> bp-multi-loc.exp
> new file mode 100644
> index 00000000000..9abff5866d9
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/thread-bp-multi-loc.exp
> @@ -0,0 +1,88 @@
> +# Copyright 2022 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/>.
> +
> +# Create a multi-location breakpoint with a thread condition, then check the
> +# output of 'info breakpoints' to ensure that the thread condition is
> +# displayed correctly.
> +
> +standard_testfile
> +
> +if { [prepare_for_testing "failed to prepare" $testfile $srcfile] } {
> +    return
> +}
> +
> +if ![runto_main] then {

Maybe use {} here, too, for consistency.

> +    return

The testsuite is unfortunately highly inconsistent about what to do here.
There is "return 0", "return 1", "return -1", and "return".
If we cannot run to the main function, "return -1" seems a better choice
to me.  What do you think?

> +}
> +
> +gdb_test_no_output "with confirm off -- delete breakpoints"

Why not use delete_breakpoints?

> +
> +set bp_number ""
> +gdb_test_multiple "break foo thread 1" "" {
> +    -re "^break foo thread 1\r\n" {
> +	exp_continue
> +    }
> +    -re "^Breakpoint ($decimal) at $hex: foo\[^\r\n\]+3 locations\[^\r\n\]+\r\n" {
> +	set bp_number $expect_out(1,string)
> +	exp_continue
> +    }
> +    -re "^$gdb_prompt $" {
> +	gdb_assert { ![string eq $bp_number ""] }
> +    }
> +}
> +
> +if { $bp_number == "" } {
> +    unresolved "breakpoint not placed correctly"
> +    return
> +}

An alternative could be to use "gdb_breakpoint" followed by

set bp_number [get_integer_valueof "\$bpnum" 0]


> +
> +set saw_header false
> +set saw_cond false
> +set loc_count 0
> +gdb_test_multiple "info breakpoints" "" {
> +    -re "^info breakpoints\r\n" {
> +	exp_continue
> +    }
> +
> +    -re "^Num\\s+\[^\r\n\]+\r\n" {

If we remove "^" at the beginning, we can also get rid of the 
first regexp above for a bit shorter code.

Thanks
-Baris

> +	exp_continue
> +    }
> +
> +    -re "^$bp_number\\s+breakpoint\\s+keep\\s+y\\s+<MULTIPLE>\\s*\r\n" {
> +	set saw_header true
> +	exp_continue
> +    }
> +
> +    -re "^\\s+stop only in thread 1\r\n" {
> +	set saw_cond true
> +	exp_continue
> +    }
> +
> +    -re "^$bp_number\\.\[123\]\\s+\[^\r\n\]+ thread 1\r\n" {
> +	incr loc_count
> +	exp_continue
> +    }
> +
> +    -re "^$gdb_prompt $" {
> +	with_test_prefix $gdb_test_name {
> +	    gdb_assert { $saw_header } \
> +		"saw header line"
> +	    gdb_assert { $saw_cond } \
> +		"saw b/p condition line"
> +	    gdb_assert { $loc_count == 3 } \
> +		"saw all three locations"
> +	}
> +    }
> +}
> --
> 2.25.4

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
  

Patch

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index f0276a963c0..2ec8ca364b6 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -6467,7 +6467,7 @@  print_one_breakpoint_location (struct breakpoint *b,
       output_thread_groups (uiout, "thread-groups", inf_nums, mi_only);
     }
 
-  if (!part_of_multiple)
+  if (!header_of_multiple)
     {
       if (b->thread != -1)
 	{
diff --git a/gdb/testsuite/gdb.base/thread-bp-multi-loc.c b/gdb/testsuite/gdb.base/thread-bp-multi-loc.c
new file mode 100644
index 00000000000..cab009c39ec
--- /dev/null
+++ b/gdb/testsuite/gdb.base/thread-bp-multi-loc.c
@@ -0,0 +1,44 @@ 
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2022 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/>.  */
+
+volatile int global_var = 0;
+
+__attribute__((__always_inline__)) static inline void
+foo (void)
+{
+  int i;
+
+  for (i = 0; i < 10; ++i)
+    global_var = i;
+}
+
+static void
+bar (void)
+{
+  global_var = 0;
+  foo ();
+}
+
+int
+main (void)
+{
+  global_var = 0;
+  foo ();
+  bar ();
+  foo ();
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/thread-bp-multi-loc.exp b/gdb/testsuite/gdb.base/thread-bp-multi-loc.exp
new file mode 100644
index 00000000000..9abff5866d9
--- /dev/null
+++ b/gdb/testsuite/gdb.base/thread-bp-multi-loc.exp
@@ -0,0 +1,88 @@ 
+# Copyright 2022 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/>.
+
+# Create a multi-location breakpoint with a thread condition, then check the
+# output of 'info breakpoints' to ensure that the thread condition is
+# displayed correctly.
+
+standard_testfile
+
+if { [prepare_for_testing "failed to prepare" $testfile $srcfile] } {
+    return
+}
+
+if ![runto_main] then {
+    return
+}
+
+gdb_test_no_output "with confirm off -- delete breakpoints"
+
+set bp_number ""
+gdb_test_multiple "break foo thread 1" "" {
+    -re "^break foo thread 1\r\n" {
+	exp_continue
+    }
+    -re "^Breakpoint ($decimal) at $hex: foo\[^\r\n\]+3 locations\[^\r\n\]+\r\n" {
+	set bp_number $expect_out(1,string)
+	exp_continue
+    }
+    -re "^$gdb_prompt $" {
+	gdb_assert { ![string eq $bp_number ""] }
+    }
+}
+
+if { $bp_number == "" } {
+    unresolved "breakpoint not placed correctly"
+    return
+}
+
+set saw_header false
+set saw_cond false
+set loc_count 0
+gdb_test_multiple "info breakpoints" "" {
+    -re "^info breakpoints\r\n" {
+	exp_continue
+    }
+
+    -re "^Num\\s+\[^\r\n\]+\r\n" {
+	exp_continue
+    }
+
+    -re "^$bp_number\\s+breakpoint\\s+keep\\s+y\\s+<MULTIPLE>\\s*\r\n" {
+	set saw_header true
+	exp_continue
+    }
+
+    -re "^\\s+stop only in thread 1\r\n" {
+	set saw_cond true
+	exp_continue
+    }
+
+    -re "^$bp_number\\.\[123\]\\s+\[^\r\n\]+ thread 1\r\n" {
+	incr loc_count
+	exp_continue
+    }
+
+    -re "^$gdb_prompt $" {
+	with_test_prefix $gdb_test_name {
+	    gdb_assert { $saw_header } \
+		"saw header line"
+	    gdb_assert { $saw_cond } \
+		"saw b/p condition line"
+	    gdb_assert { $loc_count == 3 } \
+		"saw all three locations"
+	}
+    }
+}