[PATCHv6,05/10] gdb: don't display inferior list for pending breakpoints
Checks
Context |
Check |
Description |
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 |
success
|
Testing passed
|
linaro-tcwg-bot/tcwg_gdb_build--master-arm |
success
|
Testing passed
|
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 |
success
|
Testing passed
|
Commit Message
I noticed that in the 'info breakpoints' output, GDB sometimes prints
the inferior list for pending breakpoints, this doesn't seem right to
me. A pending breakpoint has no locations (at least, as far as we
display things in the 'info breakpoints' output), so including an
inferior list seems odd.
Here's what I see right now:
(gdb) info breakpoint 5
Num Type Disp Enb Address What
5 breakpoint keep y <PENDING> foo inf 1
(gdb)
It's the 'inf 1' at the end of the line that I'm objecting too.
To trigger this behaviour we need to be in a multi-inferior debug
session. The breakpoint must have been non-pending at some point in
the past, and so have a location assigned to it.
The breakpoint becomes pending again as a result of a shared library
being unloaded. When this happens the location itself is marked
pending (via bp_location::shlib_disabled).
In print_one_breakpoint_location, in order to print the inferior list
we check that the breakpoint has a location, and that we have multiple
inferiors, but we don't check if the location itself is pending.
This commit adds that check, which means the output is now:
(gdb) info breakpoint 5
Num Type Disp Enb Address What
5 breakpoint keep y <PENDING> foo
(gdb)
Which I think makes more sense -- indeed, the format without the
inferior list is what we display for a pending breakpoint that has
never had any locations assigned, so I think this change in behaviour
makes GDB more consistent.
---
gdb/breakpoint.c | 2 +-
gdb/testsuite/gdb.multi/pending-bp-lib.c | 22 ++++
gdb/testsuite/gdb.multi/pending-bp.c | 66 ++++++++++++
gdb/testsuite/gdb.multi/pending-bp.exp | 130 +++++++++++++++++++++++
4 files changed, 219 insertions(+), 1 deletion(-)
create mode 100644 gdb/testsuite/gdb.multi/pending-bp-lib.c
create mode 100644 gdb/testsuite/gdb.multi/pending-bp.c
create mode 100644 gdb/testsuite/gdb.multi/pending-bp.exp
Comments
On Saturday, December 2, 2023 11:42 AM, Andrew Burgess wrote:
> I noticed that in the 'info breakpoints' output, GDB sometimes prints
> the inferior list for pending breakpoints, this doesn't seem right to
> me. A pending breakpoint has no locations (at least, as far as we
> display things in the 'info breakpoints' output), so including an
> inferior list seems odd.
>
> Here's what I see right now:
>
> (gdb) info breakpoint 5
> Num Type Disp Enb Address What
> 5 breakpoint keep y <PENDING> foo inf 1
> (gdb)
>
> It's the 'inf 1' at the end of the line that I'm objecting too.
>
> To trigger this behaviour we need to be in a multi-inferior debug
> session. The breakpoint must have been non-pending at some point in
> the past, and so have a location assigned to it.
>
> The breakpoint becomes pending again as a result of a shared library
> being unloaded. When this happens the location itself is marked
> pending (via bp_location::shlib_disabled).
>
> In print_one_breakpoint_location, in order to print the inferior list
> we check that the breakpoint has a location, and that we have multiple
> inferiors, but we don't check if the location itself is pending.
>
> This commit adds that check, which means the output is now:
>
> (gdb) info breakpoint 5
> Num Type Disp Enb Address What
> 5 breakpoint keep y <PENDING> foo
> (gdb)
>
> Which I think makes more sense -- indeed, the format without the
> inferior list is what we display for a pending breakpoint that has
> never had any locations assigned, so I think this change in behaviour
> makes GDB more consistent.
> ---
> gdb/breakpoint.c | 2 +-
> gdb/testsuite/gdb.multi/pending-bp-lib.c | 22 ++++
> gdb/testsuite/gdb.multi/pending-bp.c | 66 ++++++++++++
> gdb/testsuite/gdb.multi/pending-bp.exp | 130 +++++++++++++++++++++++
> 4 files changed, 219 insertions(+), 1 deletion(-)
> create mode 100644 gdb/testsuite/gdb.multi/pending-bp-lib.c
> create mode 100644 gdb/testsuite/gdb.multi/pending-bp.c
> create mode 100644 gdb/testsuite/gdb.multi/pending-bp.exp
>
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index c1440a6921f..3ee23af83d6 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -6597,7 +6597,7 @@ print_one_breakpoint_location (struct breakpoint *b,
> }
> }
>
> - if (loc != NULL && !header_of_multiple)
> + if (loc != NULL && !header_of_multiple && !loc->shlib_disabled)
Since you're touching the line, would you consider turning NULL
into nullptr?
> {
> std::vector<int> inf_nums;
> int mi_only = 1;
> diff --git a/gdb/testsuite/gdb.multi/pending-bp-lib.c b/gdb/testsuite/gdb.multi/pending-bp-
> lib.c
> new file mode 100644
> index 00000000000..15d1b9833dd
> --- /dev/null
> +++ b/gdb/testsuite/gdb.multi/pending-bp-lib.c
> @@ -0,0 +1,22 @@
> +/* 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 global_var = 0;
> +
> +void
> +foo (int arg)
> +{
> + global_var = arg;
> +}
> diff --git a/gdb/testsuite/gdb.multi/pending-bp.c b/gdb/testsuite/gdb.multi/pending-bp.c
> new file mode 100644
> index 00000000000..ca8a3c20b72
> --- /dev/null
> +++ b/gdb/testsuite/gdb.multi/pending-bp.c
> @@ -0,0 +1,66 @@
> +/* 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/>. */
> +
> +#include <dlfcn.h>
> +#include <stdlib.h>
> +
> +void
> +breakpt ()
> +{
> + /* Nothing. */
> +}
> +
> +volatile int global_counter = 0;
> +
> +volatile int call_count = 1;
> +
> +int
> +main (void)
> +{
> + void *handle;
> + void (*func)(int);
> +
> + /* Some filler work so that we don't initially stop on the breakpt call
> + below. */
> + ++global_counter;
> +
> + breakpt (); /* Break before open. */
> +
> + /* Now load the shared library. */
> + handle = dlopen (SHLIB_NAME, RTLD_LAZY);
> + if (handle == NULL)
> + abort ();
> +
> + breakpt (); /* Break after open. */
> +
> + /* Find the function symbol. */
> + func = (void (*)(int)) dlsym (handle, "foo");
> +
> + for (; call_count > 0; --call_count)
> + {
> + /* Call the library function. */
> + func (1);
> + }
> +
> + breakpt (); /* Break before close. */
> +
> + /* Unload the shared library. */
> + if (dlclose (handle) != 0)
> + abort ();
> +
> + breakpt (); /* Break after close. */
> +
> + return 0;
> +}
> diff --git a/gdb/testsuite/gdb.multi/pending-bp.exp b/gdb/testsuite/gdb.multi/pending-bp.exp
> new file mode 100644
> index 00000000000..e4f0aa2e38a
> --- /dev/null
> +++ b/gdb/testsuite/gdb.multi/pending-bp.exp
> @@ -0,0 +1,130 @@
> +# 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/>.
> +
> +# Tests related to pending breakpoints in a multi-inferior environment.
> +
> +require allow_shlib_tests !use_gdb_stub
> +
> +standard_testfile
> +
> +set libname $testfile-lib
> +set srcfile_lib $srcdir/$subdir/$libname.c
> +set binfile_lib [standard_output_file $libname.so]
> +
> +if { [gdb_compile_shlib $srcfile_lib $binfile_lib {}] != "" } {
> + untested "failed to compile shared library 1"
> + return -1
> +}
> +
> +set binfile_lib_target [gdb_download_shlib $binfile_lib]
> +
> +if { [build_executable "failed to prepare" $testfile $srcfile \
> + [list debug \
> + additional_flags=-DSHLIB_NAME=\"$binfile_lib_target\" \
> + shlib_load pthreads]] } {
Is pthreads needed?
> + return -1
> +}
> +
> +# Used to override delete_breakpoints when we don't want breakpoints
> +# deleted.
> +proc do_nothing {} {}
> +
> +# Start two inferiors, both running the same test binary. The arguments
> +# INF_1_STOP and INF_2_STOP are source code patterns that are passed to
> +# gdb_get_line_number to figure out where each inferior should be stopped.
> +#
> +# This proc does a clean_restart and leaves inferior 2 selected. Also the
> +# 'breakpoint pending' flag is enabled, so pending breakpoints can be created
> +# without GDB prompting the user.
> +proc do_test_setup { inf_1_stop inf_2_stop } {
> + clean_restart ${::binfile}
> +
> + gdb_locate_shlib $::binfile_lib
> +
> + if ![runto_main] {
Braces can be added for consistency with the other if-statements.
> + return false
> + }
> +
> + gdb_breakpoint [gdb_get_line_number ${inf_1_stop}] temporary
> + gdb_continue_to_breakpoint "move inferior 1 into position"
> +
> + gdb_test "add-inferior -exec ${::binfile}" \
> + "Added inferior 2.*" "add inferior 2"
> + gdb_test "inferior 2" "Switching to inferior 2 .*" "switch to inferior 2"
> +
> + with_override delete_breakpoints do_nothing {
I'm not sure why this is needed. The BP at inf_1_stop was already
defined temporary. Is there any breakpoint left that we want to retain?
> + if {![runto_main]} {
> + return false
> + }
> + }
> +
> + gdb_breakpoint [gdb_get_line_number ${inf_2_stop}] temporary
> + gdb_continue_to_breakpoint "move inferior 2 into position"
> +
> + gdb_test_no_output "set breakpoint pending on"
> +
> + return true
> +}
> +
> +# Check that when a breakpoint is in the pending state, but that breakpoint
> +# does have some locations (those locations themselves are pending), GDB
> +# doesn't display the inferior list in the 'info breakpoints' output.
> +proc_with_prefix test_no_inf_display {} {
> + do_test_setup "Break before open" "Break before open"
> +
> + # Create a breakpoint on 'foo'. As the shared library (that contains
> + # foo) has not been loaded into any inferior yet, then there will be no
> + # locations and the breakpoint will be created pending.
> + gdb_breakpoint "foo" allow-pending
We already did "set breakpoint pending on" in setup. Do we need "allow-pending"?
> + set bpnum [get_integer_valueof "\$bpnum" "*INVALID*" \
> + "get foo breakpoint number"]
> +
> + # Check the 'info breakpoints' output; the breakpoint is pending with
> + # not 'inf X' appearing at the end of the line.
"not" -> "no"
Regards
-Baris
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
@@ -6597,7 +6597,7 @@ print_one_breakpoint_location (struct breakpoint *b,
}
}
- if (loc != NULL && !header_of_multiple)
+ if (loc != NULL && !header_of_multiple && !loc->shlib_disabled)
{
std::vector<int> inf_nums;
int mi_only = 1;
new file mode 100644
@@ -0,0 +1,22 @@
+/* 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 global_var = 0;
+
+void
+foo (int arg)
+{
+ global_var = arg;
+}
new file mode 100644
@@ -0,0 +1,66 @@
+/* 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/>. */
+
+#include <dlfcn.h>
+#include <stdlib.h>
+
+void
+breakpt ()
+{
+ /* Nothing. */
+}
+
+volatile int global_counter = 0;
+
+volatile int call_count = 1;
+
+int
+main (void)
+{
+ void *handle;
+ void (*func)(int);
+
+ /* Some filler work so that we don't initially stop on the breakpt call
+ below. */
+ ++global_counter;
+
+ breakpt (); /* Break before open. */
+
+ /* Now load the shared library. */
+ handle = dlopen (SHLIB_NAME, RTLD_LAZY);
+ if (handle == NULL)
+ abort ();
+
+ breakpt (); /* Break after open. */
+
+ /* Find the function symbol. */
+ func = (void (*)(int)) dlsym (handle, "foo");
+
+ for (; call_count > 0; --call_count)
+ {
+ /* Call the library function. */
+ func (1);
+ }
+
+ breakpt (); /* Break before close. */
+
+ /* Unload the shared library. */
+ if (dlclose (handle) != 0)
+ abort ();
+
+ breakpt (); /* Break after close. */
+
+ return 0;
+}
new file mode 100644
@@ -0,0 +1,130 @@
+# 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/>.
+
+# Tests related to pending breakpoints in a multi-inferior environment.
+
+require allow_shlib_tests !use_gdb_stub
+
+standard_testfile
+
+set libname $testfile-lib
+set srcfile_lib $srcdir/$subdir/$libname.c
+set binfile_lib [standard_output_file $libname.so]
+
+if { [gdb_compile_shlib $srcfile_lib $binfile_lib {}] != "" } {
+ untested "failed to compile shared library 1"
+ return -1
+}
+
+set binfile_lib_target [gdb_download_shlib $binfile_lib]
+
+if { [build_executable "failed to prepare" $testfile $srcfile \
+ [list debug \
+ additional_flags=-DSHLIB_NAME=\"$binfile_lib_target\" \
+ shlib_load pthreads]] } {
+ return -1
+}
+
+# Used to override delete_breakpoints when we don't want breakpoints
+# deleted.
+proc do_nothing {} {}
+
+# Start two inferiors, both running the same test binary. The arguments
+# INF_1_STOP and INF_2_STOP are source code patterns that are passed to
+# gdb_get_line_number to figure out where each inferior should be stopped.
+#
+# This proc does a clean_restart and leaves inferior 2 selected. Also the
+# 'breakpoint pending' flag is enabled, so pending breakpoints can be created
+# without GDB prompting the user.
+proc do_test_setup { inf_1_stop inf_2_stop } {
+ clean_restart ${::binfile}
+
+ gdb_locate_shlib $::binfile_lib
+
+ if ![runto_main] {
+ return false
+ }
+
+ gdb_breakpoint [gdb_get_line_number ${inf_1_stop}] temporary
+ gdb_continue_to_breakpoint "move inferior 1 into position"
+
+ gdb_test "add-inferior -exec ${::binfile}" \
+ "Added inferior 2.*" "add inferior 2"
+ gdb_test "inferior 2" "Switching to inferior 2 .*" "switch to inferior 2"
+
+ with_override delete_breakpoints do_nothing {
+ if {![runto_main]} {
+ return false
+ }
+ }
+
+ gdb_breakpoint [gdb_get_line_number ${inf_2_stop}] temporary
+ gdb_continue_to_breakpoint "move inferior 2 into position"
+
+ gdb_test_no_output "set breakpoint pending on"
+
+ return true
+}
+
+# Check that when a breakpoint is in the pending state, but that breakpoint
+# does have some locations (those locations themselves are pending), GDB
+# doesn't display the inferior list in the 'info breakpoints' output.
+proc_with_prefix test_no_inf_display {} {
+ do_test_setup "Break before open" "Break before open"
+
+ # Create a breakpoint on 'foo'. As the shared library (that contains
+ # foo) has not been loaded into any inferior yet, then there will be no
+ # locations and the breakpoint will be created pending.
+ gdb_breakpoint "foo" allow-pending
+ set bpnum [get_integer_valueof "\$bpnum" "*INVALID*" \
+ "get foo breakpoint number"]
+
+ # Check the 'info breakpoints' output; the breakpoint is pending with
+ # not 'inf X' appearing at the end of the line.
+ gdb_test "info breakpoint $bpnum" \
+ "$bpnum\\s+breakpoint\\s+keep\\s+y\\s+<PENDING>\\s+foo" \
+ "check info bp before locations have been created"
+
+ # Now select inferior 1 and allow the inferior to run forward to the
+ # point where a breakpoint location for foo will have been created.
+ gdb_test "inferior 1" "Switching to inferior 1 .*"
+ gdb_breakpoint [gdb_get_line_number "Break after open"] temporary
+ gdb_continue_to_breakpoint \
+ "move inferior 1 until a location has been added"
+
+ # Check the 'info breakpoints' output. Notice we display the inferior
+ # list at the end of the breakpoint line.
+ gdb_test "info breakpoint $bpnum" \
+ "$bpnum\\s+breakpoint\\s+keep\\s+y\\s+$::hex\\s+<foo\[^>\]*>\\s+inf 1" \
+ "check info breakpoints while breakpoint is inserted"
+
+ # Continue inferior 1 until the shared library has been unloaded. The
+ # breakpoint on 'foo' will return to the pending state. We will need to
+ # 'continue' twice as the first time will hit the 'foo' breakpoint.
+ gdb_breakpoint [gdb_get_line_number "Break after close"] temporary
+ gdb_continue_to_breakpoint "hit the breakpoint in foo"
+ gdb_continue_to_breakpoint "after close library"
+
+ # Check the 'info breakpoints' output, check there is no 'inf 1' at the
+ # end of the breakpoint line.
+ gdb_test "info breakpoint $bpnum" \
+ [multi_line \
+ "$bpnum\\s+breakpoint\\s+keep\\s+y\\s+<PENDING>\\s+foo" \
+ "\\s+breakpoint already hit 1 time"] \
+ "check info breakpoints while breakpoint is pending"
+}
+
+# Run all the tests.
+test_no_inf_display