* [PATCH] gdb: make "start" breakpoint inferior-specific
@ 2022-08-04 17:40 Simon Marchi
2022-08-17 17:56 ` Simon Marchi
` (3 more replies)
0 siblings, 4 replies; 28+ messages in thread
From: Simon Marchi @ 2022-08-04 17:40 UTC (permalink / raw)
To: gdb-patches; +Cc: Simon Marchi
I saw this failure on a CI:
(gdb) add-inferior
[New inferior 2]
Added inferior 2
(gdb) PASS: gdb.threads/vfork-multi-inferior.exp: method=non-stop: add-inferior
inferior 2
[Switching to inferior 2 [<null>] (<noexec>)]
(gdb) PASS: gdb.threads/vfork-multi-inferior.exp: method=non-stop: inferior 2
kill
The program is not being run.
(gdb) file /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/tmp/tmp.GYATAXR8Ku/gdb/testsuite/outputs/gdb.threads/vfork-multi-inferior/vfork-multi-inferior-sleep
Reading symbols from /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/tmp/tmp.GYATAXR8Ku/gdb/testsuite/outputs/gdb.threads/vfork-multi-inferior/vfork-multi-inferior-sleep...
(gdb) run &
Starting program: /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/tmp/tmp.GYATAXR8Ku/gdb/testsuite/outputs/gdb.threads/vfork-multi-inferior/vfork-multi-inferior-sleep
(gdb) PASS: gdb.threads/vfork-multi-inferior.exp: method=non-stop: run inferior 2
inferior 1
[Switching to inferior 1 [<null>] (<noexec>)]
(gdb) PASS: gdb.threads/vfork-multi-inferior.exp: method=non-stop: inferior 1
kill
The program is not being run.
(gdb) file /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/tmp/tmp.GYATAXR8Ku/gdb/testsuite/outputs/gdb.threads/vfork-multi-inferior/vfork-multi-inferior
Reading symbols from /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/tmp/tmp.GYATAXR8Ku/gdb/testsuite/outputs/gdb.threads/vfork-multi-inferior/vfork-multi-inferior...
(gdb) break should_break_here
Breakpoint 1 at 0x11b1: file /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/src/binutils-gdb/gdb/testsuite/gdb.threads/vfork-multi-inferior.c, line 25.
(gdb) PASS: gdb.threads/vfork-multi-inferior.exp: method=non-stop: break should_break_here
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
start
Temporary breakpoint 2 at 0x11c0: -qualified main. (2 locations)
Starting program: /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/tmp/tmp.GYATAXR8Ku/gdb/testsuite/outputs/gdb.threads/vfork-multi-inferior/vfork-multi-inferior
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
Thread 2.1 "vfork-multi-inf" hit Temporary breakpoint 2, main () at /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/src/binutils-gdb/gdb/testsuite/gdb.threads/vfork-multi-inferior-sleep.c:23
23 sleep (30);
(gdb) FAIL: gdb.threads/vfork-multi-inferior.exp: method=non-stop: start inferior 1
What happens is:
1. We start inferior 2 with "run&", it runs very slowly, takes time to
get to main
2. We switch to inferior 1, and run "start"
3. The temporary breakpoint inserted by "start" applies to all inferiors
4. Inferior 2 hits that breakpoint and GDB reports that hit
To avoid this, breakpoints inserted by "start" should be
inferior-specific. However, we don't have a nice way to make
inferior-specific breakpoints yet. It's possible to make
pspace-specific breakpoints (for example how the internal_breakpoint
constructor does) by creating a symtab_and_line manually. However,
inferiors can share program spaces (usually on particular embedded
targets), so we could have a situation where two inferiors run the same
code in the same program space. In that case, it would just not be
possible to insert a breakpoint in one inferior but not the other.
A simple solution that should work all the time is to add a condition to
the breakpoint inserted by "start", to check the inferior reporting the
hit is the expected one. This is what this patch implements.
Add a test that does:
- start a background inferior 1 that calls main in a loop
- start aninferior 2 with the "start" command
- validate that we hit the breakpoint in inferior 2
Without the fix, we hit the breakpoint in inferior 1 pretty much all the
time.
Change-Id: Ib0148498a476bfa634ed62353c95f163623c686a
---
gdb/infcmd.c | 3 +-
.../gdb.multi/start-inferior-specific-other.c | 30 ++++++++++
.../gdb.multi/start-inferior-specific.c | 22 +++++++
.../gdb.multi/start-inferior-specific.exp | 58 +++++++++++++++++++
4 files changed, 112 insertions(+), 1 deletion(-)
create mode 100644 gdb/testsuite/gdb.multi/start-inferior-specific-other.c
create mode 100644 gdb/testsuite/gdb.multi/start-inferior-specific.c
create mode 100644 gdb/testsuite/gdb.multi/start-inferior-specific.exp
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 69a7896b560..e9107cef520 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -423,7 +423,8 @@ run_command_1 (const char *args, int from_tty, enum run_how run_how)
/* Insert temporary breakpoint in main function if requested. */
if (run_how == RUN_STOP_AT_MAIN)
{
- std::string arg = string_printf ("-qualified %s", main_name ());
+ std::string arg = string_printf ("-qualified %s if $_inferior == %d", main_name (),
+ current_inferior ()->num);
tbreak_command (arg.c_str (), 0);
}
diff --git a/gdb/testsuite/gdb.multi/start-inferior-specific-other.c b/gdb/testsuite/gdb.multi/start-inferior-specific-other.c
new file mode 100644
index 00000000000..b593c3aa068
--- /dev/null
+++ b/gdb/testsuite/gdb.multi/start-inferior-specific-other.c
@@ -0,0 +1,30 @@
+/* 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/>. */
+
+#include <stdlib.h>
+
+int
+main (int argc, const char **argv)
+{
+ if (argc == 999)
+ return 0;
+
+ for (;;)
+ main (999, NULL);
+
+ return 0;
+}
diff --git a/gdb/testsuite/gdb.multi/start-inferior-specific.c b/gdb/testsuite/gdb.multi/start-inferior-specific.c
new file mode 100644
index 00000000000..b69e218962a
--- /dev/null
+++ b/gdb/testsuite/gdb.multi/start-inferior-specific.c
@@ -0,0 +1,22 @@
+/* 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/>. */
+
+int
+main ()
+{
+ return 0;
+}
diff --git a/gdb/testsuite/gdb.multi/start-inferior-specific.exp b/gdb/testsuite/gdb.multi/start-inferior-specific.exp
new file mode 100644
index 00000000000..5a5572f591a
--- /dev/null
+++ b/gdb/testsuite/gdb.multi/start-inferior-specific.exp
@@ -0,0 +1,58 @@
+# 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/>.
+
+# Test that "start"ing an inferior does not inadvertently stop in another
+# inferior.
+#
+# To achieve this, we have an inferior (the "other") running in background
+# constantly re-calling main. We then "start" a second inferior. A buggy GDB
+# would report a breakpoint hit in "other".
+
+standard_testfile .c -other.c
+
+if { [use_gdb_stub] } {
+ return
+}
+
+set srcfile_other ${srcfile2}
+set binfile_other ${binfile}-other
+
+if { [build_executable ${testfile}.exp ${binfile} "${srcfile}" {debug}] != 0 } {
+ return -1
+}
+
+if { [build_executable ${testfile}.exp ${binfile_other} "${srcfile_other}" {debug}] != 0 } {
+ return -1
+}
+
+proc do_test {} {
+ # With remote, to be able to start an inferior while another one is
+ # running, we need to use the non-stop variant of the protocol.
+ save_vars { ::GDBFLAGS } {
+ if { [target_info gdb_protocol] == "extended-remote"} {
+ append ::GDBFLAGS " -ex \"maintenance set target-non-stop on\""
+ }
+
+ clean_restart ${::binfile_other}
+ }
+
+ gdb_test "run&" "Starting program: .*" "start background inferior"
+ gdb_test "add-inferior" "Added inferior 2.*"
+ gdb_test "inferior 2" "Switching to inferior 2.*"
+ gdb_file_cmd ${::binfile}
+ gdb_test "start" "Thread 2.1 .* hit Temporary breakpoint .*"
+}
+
+do_test
--
2.37.1
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] gdb: make "start" breakpoint inferior-specific
2022-08-04 17:40 [PATCH] gdb: make "start" breakpoint inferior-specific Simon Marchi
@ 2022-08-17 17:56 ` Simon Marchi
2022-08-31 14:03 ` Bruno Larsen
` (2 subsequent siblings)
3 siblings, 0 replies; 28+ messages in thread
From: Simon Marchi @ 2022-08-17 17:56 UTC (permalink / raw)
To: Simon Marchi, gdb-patches
Ping.
On 2022-08-04 13:40, Simon Marchi via Gdb-patches wrote:
> I saw this failure on a CI:
>
> (gdb) add-inferior
> [New inferior 2]
> Added inferior 2
> (gdb) PASS: gdb.threads/vfork-multi-inferior.exp: method=non-stop: add-inferior
> inferior 2
> [Switching to inferior 2 [<null>] (<noexec>)]
> (gdb) PASS: gdb.threads/vfork-multi-inferior.exp: method=non-stop: inferior 2
> kill
> The program is not being run.
> (gdb) file /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/tmp/tmp.GYATAXR8Ku/gdb/testsuite/outputs/gdb.threads/vfork-multi-inferior/vfork-multi-inferior-sleep
> Reading symbols from /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/tmp/tmp.GYATAXR8Ku/gdb/testsuite/outputs/gdb.threads/vfork-multi-inferior/vfork-multi-inferior-sleep...
> (gdb) run &
> Starting program: /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/tmp/tmp.GYATAXR8Ku/gdb/testsuite/outputs/gdb.threads/vfork-multi-inferior/vfork-multi-inferior-sleep
> (gdb) PASS: gdb.threads/vfork-multi-inferior.exp: method=non-stop: run inferior 2
> inferior 1
> [Switching to inferior 1 [<null>] (<noexec>)]
> (gdb) PASS: gdb.threads/vfork-multi-inferior.exp: method=non-stop: inferior 1
> kill
> The program is not being run.
> (gdb) file /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/tmp/tmp.GYATAXR8Ku/gdb/testsuite/outputs/gdb.threads/vfork-multi-inferior/vfork-multi-inferior
> Reading symbols from /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/tmp/tmp.GYATAXR8Ku/gdb/testsuite/outputs/gdb.threads/vfork-multi-inferior/vfork-multi-inferior...
> (gdb) break should_break_here
> Breakpoint 1 at 0x11b1: file /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/src/binutils-gdb/gdb/testsuite/gdb.threads/vfork-multi-inferior.c, line 25.
> (gdb) PASS: gdb.threads/vfork-multi-inferior.exp: method=non-stop: break should_break_here
> [Thread debugging using libthread_db enabled]
> Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
> start
> Temporary breakpoint 2 at 0x11c0: -qualified main. (2 locations)
> Starting program: /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/tmp/tmp.GYATAXR8Ku/gdb/testsuite/outputs/gdb.threads/vfork-multi-inferior/vfork-multi-inferior
> [Thread debugging using libthread_db enabled]
> Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
>
> Thread 2.1 "vfork-multi-inf" hit Temporary breakpoint 2, main () at /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/src/binutils-gdb/gdb/testsuite/gdb.threads/vfork-multi-inferior-sleep.c:23
> 23 sleep (30);
> (gdb) FAIL: gdb.threads/vfork-multi-inferior.exp: method=non-stop: start inferior 1
>
> What happens is:
>
> 1. We start inferior 2 with "run&", it runs very slowly, takes time to
> get to main
> 2. We switch to inferior 1, and run "start"
> 3. The temporary breakpoint inserted by "start" applies to all inferiors
> 4. Inferior 2 hits that breakpoint and GDB reports that hit
>
> To avoid this, breakpoints inserted by "start" should be
> inferior-specific. However, we don't have a nice way to make
> inferior-specific breakpoints yet. It's possible to make
> pspace-specific breakpoints (for example how the internal_breakpoint
> constructor does) by creating a symtab_and_line manually. However,
> inferiors can share program spaces (usually on particular embedded
> targets), so we could have a situation where two inferiors run the same
> code in the same program space. In that case, it would just not be
> possible to insert a breakpoint in one inferior but not the other.
>
> A simple solution that should work all the time is to add a condition to
> the breakpoint inserted by "start", to check the inferior reporting the
> hit is the expected one. This is what this patch implements.
>
> Add a test that does:
>
> - start a background inferior 1 that calls main in a loop
> - start aninferior 2 with the "start" command
> - validate that we hit the breakpoint in inferior 2
>
> Without the fix, we hit the breakpoint in inferior 1 pretty much all the
> time.
>
> Change-Id: Ib0148498a476bfa634ed62353c95f163623c686a
> ---
> gdb/infcmd.c | 3 +-
> .../gdb.multi/start-inferior-specific-other.c | 30 ++++++++++
> .../gdb.multi/start-inferior-specific.c | 22 +++++++
> .../gdb.multi/start-inferior-specific.exp | 58 +++++++++++++++++++
> 4 files changed, 112 insertions(+), 1 deletion(-)
> create mode 100644 gdb/testsuite/gdb.multi/start-inferior-specific-other.c
> create mode 100644 gdb/testsuite/gdb.multi/start-inferior-specific.c
> create mode 100644 gdb/testsuite/gdb.multi/start-inferior-specific.exp
>
> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
> index 69a7896b560..e9107cef520 100644
> --- a/gdb/infcmd.c
> +++ b/gdb/infcmd.c
> @@ -423,7 +423,8 @@ run_command_1 (const char *args, int from_tty, enum run_how run_how)
> /* Insert temporary breakpoint in main function if requested. */
> if (run_how == RUN_STOP_AT_MAIN)
> {
> - std::string arg = string_printf ("-qualified %s", main_name ());
> + std::string arg = string_printf ("-qualified %s if $_inferior == %d", main_name (),
> + current_inferior ()->num);
> tbreak_command (arg.c_str (), 0);
> }
>
> diff --git a/gdb/testsuite/gdb.multi/start-inferior-specific-other.c b/gdb/testsuite/gdb.multi/start-inferior-specific-other.c
> new file mode 100644
> index 00000000000..b593c3aa068
> --- /dev/null
> +++ b/gdb/testsuite/gdb.multi/start-inferior-specific-other.c
> @@ -0,0 +1,30 @@
> +/* 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/>. */
> +
> +#include <stdlib.h>
> +
> +int
> +main (int argc, const char **argv)
> +{
> + if (argc == 999)
> + return 0;
> +
> + for (;;)
> + main (999, NULL);
> +
> + return 0;
> +}
> diff --git a/gdb/testsuite/gdb.multi/start-inferior-specific.c b/gdb/testsuite/gdb.multi/start-inferior-specific.c
> new file mode 100644
> index 00000000000..b69e218962a
> --- /dev/null
> +++ b/gdb/testsuite/gdb.multi/start-inferior-specific.c
> @@ -0,0 +1,22 @@
> +/* 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/>. */
> +
> +int
> +main ()
> +{
> + return 0;
> +}
> diff --git a/gdb/testsuite/gdb.multi/start-inferior-specific.exp b/gdb/testsuite/gdb.multi/start-inferior-specific.exp
> new file mode 100644
> index 00000000000..5a5572f591a
> --- /dev/null
> +++ b/gdb/testsuite/gdb.multi/start-inferior-specific.exp
> @@ -0,0 +1,58 @@
> +# 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/>.
> +
> +# Test that "start"ing an inferior does not inadvertently stop in another
> +# inferior.
> +#
> +# To achieve this, we have an inferior (the "other") running in background
> +# constantly re-calling main. We then "start" a second inferior. A buggy GDB
> +# would report a breakpoint hit in "other".
> +
> +standard_testfile .c -other.c
> +
> +if { [use_gdb_stub] } {
> + return
> +}
> +
> +set srcfile_other ${srcfile2}
> +set binfile_other ${binfile}-other
> +
> +if { [build_executable ${testfile}.exp ${binfile} "${srcfile}" {debug}] != 0 } {
> + return -1
> +}
> +
> +if { [build_executable ${testfile}.exp ${binfile_other} "${srcfile_other}" {debug}] != 0 } {
> + return -1
> +}
> +
> +proc do_test {} {
> + # With remote, to be able to start an inferior while another one is
> + # running, we need to use the non-stop variant of the protocol.
> + save_vars { ::GDBFLAGS } {
> + if { [target_info gdb_protocol] == "extended-remote"} {
> + append ::GDBFLAGS " -ex \"maintenance set target-non-stop on\""
> + }
> +
> + clean_restart ${::binfile_other}
> + }
> +
> + gdb_test "run&" "Starting program: .*" "start background inferior"
> + gdb_test "add-inferior" "Added inferior 2.*"
> + gdb_test "inferior 2" "Switching to inferior 2.*"
> + gdb_file_cmd ${::binfile}
> + gdb_test "start" "Thread 2.1 .* hit Temporary breakpoint .*"
> +}
> +
> +do_test
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] gdb: make "start" breakpoint inferior-specific
2022-08-04 17:40 [PATCH] gdb: make "start" breakpoint inferior-specific Simon Marchi
2022-08-17 17:56 ` Simon Marchi
@ 2022-08-31 14:03 ` Bruno Larsen
2022-11-04 16:52 ` Simon Marchi
2022-09-01 10:42 ` Andrew Burgess
2022-11-08 19:43 ` Pedro Alves
3 siblings, 1 reply; 28+ messages in thread
From: Bruno Larsen @ 2022-08-31 14:03 UTC (permalink / raw)
To: gdb-patches
On 04/08/2022 19:40, Simon Marchi via Gdb-patches wrote:
> I saw this failure on a CI:
>
> (gdb) add-inferior
> [New inferior 2]
> Added inferior 2
> (gdb) PASS: gdb.threads/vfork-multi-inferior.exp: method=non-stop: add-inferior
> inferior 2
> [Switching to inferior 2 [<null>] (<noexec>)]
> (gdb) PASS: gdb.threads/vfork-multi-inferior.exp: method=non-stop: inferior 2
> kill
> The program is not being run.
> (gdb) file /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/tmp/tmp.GYATAXR8Ku/gdb/testsuite/outputs/gdb.threads/vfork-multi-inferior/vfork-multi-inferior-sleep
> Reading symbols from /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/tmp/tmp.GYATAXR8Ku/gdb/testsuite/outputs/gdb.threads/vfork-multi-inferior/vfork-multi-inferior-sleep...
> (gdb) run &
> Starting program: /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/tmp/tmp.GYATAXR8Ku/gdb/testsuite/outputs/gdb.threads/vfork-multi-inferior/vfork-multi-inferior-sleep
> (gdb) PASS: gdb.threads/vfork-multi-inferior.exp: method=non-stop: run inferior 2
> inferior 1
> [Switching to inferior 1 [<null>] (<noexec>)]
> (gdb) PASS: gdb.threads/vfork-multi-inferior.exp: method=non-stop: inferior 1
> kill
> The program is not being run.
> (gdb) file /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/tmp/tmp.GYATAXR8Ku/gdb/testsuite/outputs/gdb.threads/vfork-multi-inferior/vfork-multi-inferior
> Reading symbols from /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/tmp/tmp.GYATAXR8Ku/gdb/testsuite/outputs/gdb.threads/vfork-multi-inferior/vfork-multi-inferior...
> (gdb) break should_break_here
> Breakpoint 1 at 0x11b1: file /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/src/binutils-gdb/gdb/testsuite/gdb.threads/vfork-multi-inferior.c, line 25.
> (gdb) PASS: gdb.threads/vfork-multi-inferior.exp: method=non-stop: break should_break_here
> [Thread debugging using libthread_db enabled]
> Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
> start
> Temporary breakpoint 2 at 0x11c0: -qualified main. (2 locations)
> Starting program: /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/tmp/tmp.GYATAXR8Ku/gdb/testsuite/outputs/gdb.threads/vfork-multi-inferior/vfork-multi-inferior
> [Thread debugging using libthread_db enabled]
> Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
>
> Thread 2.1 "vfork-multi-inf" hit Temporary breakpoint 2, main () at /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/src/binutils-gdb/gdb/testsuite/gdb.threads/vfork-multi-inferior-sleep.c:23
> 23 sleep (30);
> (gdb) FAIL: gdb.threads/vfork-multi-inferior.exp: method=non-stop: start inferior 1
>
> What happens is:
>
> 1. We start inferior 2 with "run&", it runs very slowly, takes time to
> get to main
> 2. We switch to inferior 1, and run "start"
> 3. The temporary breakpoint inserted by "start" applies to all inferiors
> 4. Inferior 2 hits that breakpoint and GDB reports that hit
>
> To avoid this, breakpoints inserted by "start" should be
> inferior-specific. However, we don't have a nice way to make
> inferior-specific breakpoints yet. It's possible to make
> pspace-specific breakpoints (for example how the internal_breakpoint
> constructor does) by creating a symtab_and_line manually. However,
> inferiors can share program spaces (usually on particular embedded
> targets), so we could have a situation where two inferiors run the same
> code in the same program space. In that case, it would just not be
> possible to insert a breakpoint in one inferior but not the other.
>
> A simple solution that should work all the time is to add a condition to
> the breakpoint inserted by "start", to check the inferior reporting the
> hit is the expected one. This is what this patch implements.
>
> Add a test that does:
>
> - start a background inferior 1 that calls main in a loop
> - start aninferior 2 with the "start" command
> - validate that we hit the breakpoint in inferior 2
>
> Without the fix, we hit the breakpoint in inferior 1 pretty much all the
> time.
>
> Change-Id: Ib0148498a476bfa634ed62353c95f163623c686a
Hi Simon,
This change makes a lot of sense, I just wonder if it would make sense
to make breakpoints inferior-specific. I could see a situation where
someone would run the same program twice, one with an ok input while the
other has a buggy input for instance, to see the difference between
them, and inferior-specific breakpoints could come in handy in a
situation like this.
If you think it would be too much work for a usecase that is too niche,
this patch is pretty straight forward, I'd suggest you approve your patch.
Cheers,
Bruno
> ---
> gdb/infcmd.c | 3 +-
> .../gdb.multi/start-inferior-specific-other.c | 30 ++++++++++
> .../gdb.multi/start-inferior-specific.c | 22 +++++++
> .../gdb.multi/start-inferior-specific.exp | 58 +++++++++++++++++++
> 4 files changed, 112 insertions(+), 1 deletion(-)
> create mode 100644 gdb/testsuite/gdb.multi/start-inferior-specific-other.c
> create mode 100644 gdb/testsuite/gdb.multi/start-inferior-specific.c
> create mode 100644 gdb/testsuite/gdb.multi/start-inferior-specific.exp
>
> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
> index 69a7896b560..e9107cef520 100644
> --- a/gdb/infcmd.c
> +++ b/gdb/infcmd.c
> @@ -423,7 +423,8 @@ run_command_1 (const char *args, int from_tty, enum run_how run_how)
> /* Insert temporary breakpoint in main function if requested. */
> if (run_how == RUN_STOP_AT_MAIN)
> {
> - std::string arg = string_printf ("-qualified %s", main_name ());
> + std::string arg = string_printf ("-qualified %s if $_inferior == %d", main_name (),
> + current_inferior ()->num);
> tbreak_command (arg.c_str (), 0);
> }
>
> diff --git a/gdb/testsuite/gdb.multi/start-inferior-specific-other.c b/gdb/testsuite/gdb.multi/start-inferior-specific-other.c
> new file mode 100644
> index 00000000000..b593c3aa068
> --- /dev/null
> +++ b/gdb/testsuite/gdb.multi/start-inferior-specific-other.c
> @@ -0,0 +1,30 @@
> +/* 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/>. */
> +
> +#include <stdlib.h>
> +
> +int
> +main (int argc, const char **argv)
> +{
> + if (argc == 999)
> + return 0;
> +
> + for (;;)
> + main (999, NULL);
> +
> + return 0;
> +}
> diff --git a/gdb/testsuite/gdb.multi/start-inferior-specific.c b/gdb/testsuite/gdb.multi/start-inferior-specific.c
> new file mode 100644
> index 00000000000..b69e218962a
> --- /dev/null
> +++ b/gdb/testsuite/gdb.multi/start-inferior-specific.c
> @@ -0,0 +1,22 @@
> +/* 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/>. */
> +
> +int
> +main ()
> +{
> + return 0;
> +}
> diff --git a/gdb/testsuite/gdb.multi/start-inferior-specific.exp b/gdb/testsuite/gdb.multi/start-inferior-specific.exp
> new file mode 100644
> index 00000000000..5a5572f591a
> --- /dev/null
> +++ b/gdb/testsuite/gdb.multi/start-inferior-specific.exp
> @@ -0,0 +1,58 @@
> +# 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/>.
> +
> +# Test that "start"ing an inferior does not inadvertently stop in another
> +# inferior.
> +#
> +# To achieve this, we have an inferior (the "other") running in background
> +# constantly re-calling main. We then "start" a second inferior. A buggy GDB
> +# would report a breakpoint hit in "other".
> +
> +standard_testfile .c -other.c
> +
> +if { [use_gdb_stub] } {
> + return
> +}
> +
> +set srcfile_other ${srcfile2}
> +set binfile_other ${binfile}-other
> +
> +if { [build_executable ${testfile}.exp ${binfile} "${srcfile}" {debug}] != 0 } {
> + return -1
> +}
> +
> +if { [build_executable ${testfile}.exp ${binfile_other} "${srcfile_other}" {debug}] != 0 } {
> + return -1
> +}
> +
> +proc do_test {} {
> + # With remote, to be able to start an inferior while another one is
> + # running, we need to use the non-stop variant of the protocol.
> + save_vars { ::GDBFLAGS } {
> + if { [target_info gdb_protocol] == "extended-remote"} {
> + append ::GDBFLAGS " -ex \"maintenance set target-non-stop on\""
> + }
> +
> + clean_restart ${::binfile_other}
> + }
> +
> + gdb_test "run&" "Starting program: .*" "start background inferior"
> + gdb_test "add-inferior" "Added inferior 2.*"
> + gdb_test "inferior 2" "Switching to inferior 2.*"
> + gdb_file_cmd ${::binfile}
> + gdb_test "start" "Thread 2.1 .* hit Temporary breakpoint .*"
> +}
> +
> +do_test
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] gdb: make "start" breakpoint inferior-specific
2022-08-04 17:40 [PATCH] gdb: make "start" breakpoint inferior-specific Simon Marchi
2022-08-17 17:56 ` Simon Marchi
2022-08-31 14:03 ` Bruno Larsen
@ 2022-09-01 10:42 ` Andrew Burgess
2022-11-04 17:24 ` Simon Marchi
2022-11-08 19:43 ` Pedro Alves
3 siblings, 1 reply; 28+ messages in thread
From: Andrew Burgess @ 2022-09-01 10:42 UTC (permalink / raw)
To: Simon Marchi via Gdb-patches, gdb-patches; +Cc: Simon Marchi
Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
> I saw this failure on a CI:
>
> (gdb) add-inferior
> [New inferior 2]
> Added inferior 2
> (gdb) PASS: gdb.threads/vfork-multi-inferior.exp: method=non-stop: add-inferior
> inferior 2
> [Switching to inferior 2 [<null>] (<noexec>)]
> (gdb) PASS: gdb.threads/vfork-multi-inferior.exp: method=non-stop: inferior 2
> kill
> The program is not being run.
> (gdb) file /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/tmp/tmp.GYATAXR8Ku/gdb/testsuite/outputs/gdb.threads/vfork-multi-inferior/vfork-multi-inferior-sleep
> Reading symbols from /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/tmp/tmp.GYATAXR8Ku/gdb/testsuite/outputs/gdb.threads/vfork-multi-inferior/vfork-multi-inferior-sleep...
> (gdb) run &
> Starting program: /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/tmp/tmp.GYATAXR8Ku/gdb/testsuite/outputs/gdb.threads/vfork-multi-inferior/vfork-multi-inferior-sleep
> (gdb) PASS: gdb.threads/vfork-multi-inferior.exp: method=non-stop: run inferior 2
> inferior 1
> [Switching to inferior 1 [<null>] (<noexec>)]
> (gdb) PASS: gdb.threads/vfork-multi-inferior.exp: method=non-stop: inferior 1
> kill
> The program is not being run.
> (gdb) file /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/tmp/tmp.GYATAXR8Ku/gdb/testsuite/outputs/gdb.threads/vfork-multi-inferior/vfork-multi-inferior
> Reading symbols from /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/tmp/tmp.GYATAXR8Ku/gdb/testsuite/outputs/gdb.threads/vfork-multi-inferior/vfork-multi-inferior...
> (gdb) break should_break_here
> Breakpoint 1 at 0x11b1: file /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/src/binutils-gdb/gdb/testsuite/gdb.threads/vfork-multi-inferior.c, line 25.
> (gdb) PASS: gdb.threads/vfork-multi-inferior.exp: method=non-stop: break should_break_here
> [Thread debugging using libthread_db enabled]
> Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
> start
> Temporary breakpoint 2 at 0x11c0: -qualified main. (2 locations)
> Starting program: /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/tmp/tmp.GYATAXR8Ku/gdb/testsuite/outputs/gdb.threads/vfork-multi-inferior/vfork-multi-inferior
> [Thread debugging using libthread_db enabled]
> Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
>
> Thread 2.1 "vfork-multi-inf" hit Temporary breakpoint 2, main () at /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/src/binutils-gdb/gdb/testsuite/gdb.threads/vfork-multi-inferior-sleep.c:23
> 23 sleep (30);
> (gdb) FAIL: gdb.threads/vfork-multi-inferior.exp: method=non-stop: start inferior 1
>
> What happens is:
>
> 1. We start inferior 2 with "run&", it runs very slowly, takes time to
> get to main
> 2. We switch to inferior 1, and run "start"
> 3. The temporary breakpoint inserted by "start" applies to all inferiors
> 4. Inferior 2 hits that breakpoint and GDB reports that hit
>
> To avoid this, breakpoints inserted by "start" should be
> inferior-specific. However, we don't have a nice way to make
> inferior-specific breakpoints yet. It's possible to make
> pspace-specific breakpoints (for example how the internal_breakpoint
> constructor does) by creating a symtab_and_line manually. However,
> inferiors can share program spaces (usually on particular embedded
> targets), so we could have a situation where two inferiors run the same
> code in the same program space. In that case, it would just not be
> possible to insert a breakpoint in one inferior but not the other.
>
> A simple solution that should work all the time is to add a condition to
> the breakpoint inserted by "start", to check the inferior reporting the
> hit is the expected one. This is what this patch implements.
>
> Add a test that does:
>
> - start a background inferior 1 that calls main in a loop
> - start aninferior 2 with the "start" command
> - validate that we hit the breakpoint in inferior 2
>
> Without the fix, we hit the breakpoint in inferior 1 pretty much all the
> time.
>
> Change-Id: Ib0148498a476bfa634ed62353c95f163623c686a
> ---
> gdb/infcmd.c | 3 +-
> .../gdb.multi/start-inferior-specific-other.c | 30 ++++++++++
> .../gdb.multi/start-inferior-specific.c | 22 +++++++
> .../gdb.multi/start-inferior-specific.exp | 58 +++++++++++++++++++
> 4 files changed, 112 insertions(+), 1 deletion(-)
> create mode 100644 gdb/testsuite/gdb.multi/start-inferior-specific-other.c
> create mode 100644 gdb/testsuite/gdb.multi/start-inferior-specific.c
> create mode 100644 gdb/testsuite/gdb.multi/start-inferior-specific.exp
>
> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
> index 69a7896b560..e9107cef520 100644
> --- a/gdb/infcmd.c
> +++ b/gdb/infcmd.c
> @@ -423,7 +423,8 @@ run_command_1 (const char *args, int from_tty, enum run_how run_how)
> /* Insert temporary breakpoint in main function if requested. */
> if (run_how == RUN_STOP_AT_MAIN)
> {
> - std::string arg = string_printf ("-qualified %s", main_name ());
> + std::string arg = string_printf ("-qualified %s if $_inferior == %d", main_name (),
> + current_inferior ()->num);
> tbreak_command (arg.c_str (), 0);
> }
>
> diff --git a/gdb/testsuite/gdb.multi/start-inferior-specific-other.c b/gdb/testsuite/gdb.multi/start-inferior-specific-other.c
> new file mode 100644
> index 00000000000..b593c3aa068
> --- /dev/null
> +++ b/gdb/testsuite/gdb.multi/start-inferior-specific-other.c
> @@ -0,0 +1,30 @@
> +/* 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/>. */
> +
> +#include <stdlib.h>
> +
> +int
> +main (int argc, const char **argv)
> +{
> + if (argc == 999)
> + return 0;
> +
> + for (;;)
> + main (999, NULL);
> +
> + return 0;
> +}
I think test cases like this should have an `alarm` call, just in case
something goes wrong, this'll stop the test running forever.
> diff --git a/gdb/testsuite/gdb.multi/start-inferior-specific.c b/gdb/testsuite/gdb.multi/start-inferior-specific.c
> new file mode 100644
> index 00000000000..b69e218962a
> --- /dev/null
> +++ b/gdb/testsuite/gdb.multi/start-inferior-specific.c
> @@ -0,0 +1,22 @@
> +/* 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/>. */
> +
> +int
> +main ()
> +{
> + return 0;
> +}
> diff --git a/gdb/testsuite/gdb.multi/start-inferior-specific.exp b/gdb/testsuite/gdb.multi/start-inferior-specific.exp
> new file mode 100644
> index 00000000000..5a5572f591a
> --- /dev/null
> +++ b/gdb/testsuite/gdb.multi/start-inferior-specific.exp
> @@ -0,0 +1,58 @@
> +# 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/>.
> +
> +# Test that "start"ing an inferior does not inadvertently stop in another
> +# inferior.
> +#
> +# To achieve this, we have an inferior (the "other") running in background
> +# constantly re-calling main. We then "start" a second inferior. A buggy GDB
> +# would report a breakpoint hit in "other".
> +
> +standard_testfile .c -other.c
> +
> +if { [use_gdb_stub] } {
> + return
> +}
> +
> +set srcfile_other ${srcfile2}
> +set binfile_other ${binfile}-other
> +
> +if { [build_executable ${testfile}.exp ${binfile} "${srcfile}" {debug}] != 0 } {
> + return -1
> +}
> +
> +if { [build_executable ${testfile}.exp ${binfile_other} "${srcfile_other}" {debug}] != 0 } {
> + return -1
> +}
> +
> +proc do_test {} {
> + # With remote, to be able to start an inferior while another one is
> + # running, we need to use the non-stop variant of the protocol.
> + save_vars { ::GDBFLAGS } {
> + if { [target_info gdb_protocol] == "extended-remote"} {
> + append ::GDBFLAGS " -ex \"maintenance set target-non-stop on\""
> + }
> +
> + clean_restart ${::binfile_other}
> + }
> +
> + gdb_test "run&" "Starting program: .*" "start background inferior"
> + gdb_test "add-inferior" "Added inferior 2.*"
> + gdb_test "inferior 2" "Switching to inferior 2.*"
> + gdb_file_cmd ${::binfile}
> + gdb_test "start" "Thread 2.1 .* hit Temporary breakpoint .*"
I'm pretty sure there's a race here. We could set the breakpoint for
the 'start', start inferior 2 executing, hit the b/p and remove it, all
within the time it takes inferior 1 to perform a single iteration ... in
theory, though it does seem unlikely.
If, before the `start` you did `break *main` then we should hit the
`*main` breakpoint before the temporary `start` breakpoint, the
temporary breakpoint is left in place, which gives inferior 1 additional
time to not hit the breakpoint.
Of course, this relies on the `*main` and `start` breakpoints being at
different addresses, which should be the case if main has some prologue
to skip over.
I guess, ideally we would also do something to confirm that inferior 1
has gone past main while the temporary breakpoint is in place, right now
we're just relying on inferior 1 running fast enough.
In general I'm happy enough with the actual GDB change you propose here,
but like Bruno, I also wondered if it would be better to add inferior
specific breakpoints as an actual thing, rather than relying on the
condition logic like you do here?
I wasn't sure how you'd feel about this idea, so I didn't spend too
long, but below is a VERY rough proof of concept patch, that appies on
top of yours, that adds inferior specific breakpoints. You temporary
breakpoint now becomes:
tbreak -qualified main inferior 1
But this functionality would also be available to a user if they wanted
it, which might be nice.
When I say the code below is rough, I really mean it, here's a list of
things I think still need doing:
* I initially added a `breakpoint::inferior` field just like we have a
`breakpoint::thread` field, but I think a better solution would be
to change `breakpoint::thread` into a ptid_t and store inferior and
thread information in that one field,
* Obviously it should give an error if a user specifies both `thread`
and `inferior` for a breakpoint,
* The `info breakpoints` output is fine for single location
breakpoints, but I think multi-location breakpoints would need
sorting out still,
* There's a couple of TODO comments in there still to address,
* Need to write docs, NEWS, and add tests,
Like I said, you might see a reason why this is a bad idea, so I didn't
want to put any more work in without having a discussion about the idea.
Thoughts?
Thanks,
Andrew
---
commit 26a101c678cd14828cbd7f8c15240854a50c91e2
Author: Andrew Burgess <aburgess@redhat.com>
Date: Thu Sep 1 10:44:27 2022 +0100
wip: per-inferior breakpoints
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 63feea9e9cd..2ea453ff2c4 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -96,7 +96,7 @@ static void create_breakpoints_sal (struct gdbarch *,
gdb::unique_xmalloc_ptr<char>,
gdb::unique_xmalloc_ptr<char>,
enum bptype,
- enum bpdisp, int, int,
+ enum bpdisp, int, int, int,
int,
int, int, int, unsigned);
@@ -3101,6 +3101,17 @@ update_inserted_breakpoint_locations (void)
}
}
+static bool
+valid_global_inferior_id (int id)
+{
+ for (inferior *inf : all_inferiors ())
+ if (inf->num == id)
+ return true;
+
+ return false;
+}
+
+
/* Used when starting or continuing the program. */
static void
@@ -3132,6 +3143,10 @@ insert_breakpoint_locations (void)
&& !valid_global_thread_id (bl->owner->thread))
continue;
+ if (bl->owner->inferior != -1
+ && !valid_global_inferior_id (bl->owner->inferior))
+ continue;
+
switch_to_program_space_and_thread (bl->pspace);
/* For targets that support global breakpoints, there's no need
@@ -3236,6 +3251,27 @@ Thread-specific breakpoint %d deleted - thread %s no longer in the thread list.\
}
}
+/* ... */
+
+static void
+remove_inferior_breakpoints (struct inferior *inf)
+{
+ for (breakpoint *b : all_breakpoints_safe ())
+ {
+ if (b->inferior == inf->num && user_breakpoint_p (b))
+ {
+ b->disposition = disp_del_at_next_stop;
+
+ gdb_printf (_("\
+Inferior-specific breakpoint %d deleted - inferior %d has exited.\n"),
+ b->number, inf->num);
+
+ /* Hide it from the user. */
+ b->number = 0;
+ }
+ }
+}
+
/* See breakpoint.h. */
void
@@ -5331,6 +5367,7 @@ bpstat_check_breakpoint_conditions (bpstat *bs, thread_info *thread)
evaluating the condition if this isn't the specified
thread/task. */
if ((b->thread != -1 && b->thread != thread->global_num)
+ || (b->inferior != -1 && b->inferior != thread->inf->num)
|| (b->task != 0 && b->task != ada_get_task_number (thread)))
{
bs->stop = 0;
@@ -6339,6 +6376,11 @@ print_one_breakpoint_location (struct breakpoint *b,
uiout->text (" task ");
uiout->field_signed ("task", b->task);
}
+ else if (b->inferior != -1)
+ {
+ uiout->text (" inferior ");
+ uiout->field_signed ("inferior", b->inferior);
+ }
}
uiout->text ("\n");
@@ -6384,16 +6426,34 @@ print_one_breakpoint_location (struct breakpoint *b,
/* FIXME should make an annotation for this. */
uiout->text ("\tstop only in thread ");
if (uiout->is_mi_like_p ())
- uiout->field_signed ("thread", b->thread);
+ {
+ uiout->field_signed ("thread", b->thread);
+ }
else
{
struct thread_info *thr = find_thread_global_id (b->thread);
uiout->field_string ("thread", print_thread_id (thr));
+ uiout->field_signed ("thread", b->inferior);
}
uiout->text ("\n");
}
-
+
+ if (!part_of_multiple && b->inferior != -1)
+ {
+ /* FIXME should make an annotation for this. */
+ uiout->text ("\tstop only in inferior ");
+ if (uiout->is_mi_like_p ())
+ {
+ uiout->field_signed ("inferior", b->inferior);
+ }
+ else
+ {
+ uiout->field_signed ("inferior", b->inferior);
+ }
+ uiout->text ("\n");
+ }
+
if (!part_of_multiple)
{
if (b->hit_count)
@@ -7366,7 +7426,10 @@ delete_longjmp_breakpoint (int thread)
if (b->type == bp_longjmp || b->type == bp_exception)
{
if (b->thread == thread)
- delete_breakpoint (b);
+ {
+ gdb_assert (b->inferior == -1);
+ delete_breakpoint (b);
+ }
}
}
@@ -7377,7 +7440,10 @@ delete_longjmp_breakpoint_at_next_stop (int thread)
if (b->type == bp_longjmp || b->type == bp_exception)
{
if (b->thread == thread)
- b->disposition = disp_del_at_next_stop;
+ {
+ gdb_assert (b->inferior == -1);
+ b->disposition = disp_del_at_next_stop;
+ }
}
}
@@ -7430,6 +7496,7 @@ check_longjmp_breakpoint_for_call_dummy (struct thread_info *tp)
ALL_BREAKPOINTS_SAFE (b, b_tmp)
if (b->type == bp_longjmp_call_dummy && b->thread == tp->global_num)
{
+ gdb_assert (b->inferior == -1);
struct breakpoint *dummy_b = b->related_breakpoint;
/* Find the bp_call_dummy breakpoint in the list of breakpoints
@@ -8263,7 +8330,8 @@ code_breakpoint::code_breakpoint (struct gdbarch *gdbarch_,
gdb::unique_xmalloc_ptr<char> cond_string_,
gdb::unique_xmalloc_ptr<char> extra_string_,
enum bpdisp disposition_,
- int thread_, int task_, int ignore_count_,
+ int thread_, int task_, int inferior_,
+ int ignore_count_,
int from_tty,
int enabled_, unsigned flags,
int display_canonical_)
@@ -8289,6 +8357,7 @@ code_breakpoint::code_breakpoint (struct gdbarch *gdbarch_,
thread = thread_;
task = task_;
+ inferior = inferior_;
cond_string = std::move (cond_string_);
extra_string = std::move (extra_string_);
@@ -8390,7 +8459,7 @@ create_breakpoint_sal (struct gdbarch *gdbarch,
gdb::unique_xmalloc_ptr<char> cond_string,
gdb::unique_xmalloc_ptr<char> extra_string,
enum bptype type, enum bpdisp disposition,
- int thread, int task, int ignore_count,
+ int thread, int task, int inferior, int ignore_count,
int from_tty,
int enabled, int internal, unsigned flags,
int display_canonical)
@@ -8404,7 +8473,7 @@ create_breakpoint_sal (struct gdbarch *gdbarch,
std::move (cond_string),
std::move (extra_string),
disposition,
- thread, task, ignore_count,
+ thread, task, inferior, ignore_count,
from_tty,
enabled, flags,
display_canonical);
@@ -8433,7 +8502,8 @@ create_breakpoints_sal (struct gdbarch *gdbarch,
gdb::unique_xmalloc_ptr<char> cond_string,
gdb::unique_xmalloc_ptr<char> extra_string,
enum bptype type, enum bpdisp disposition,
- int thread, int task, int ignore_count,
+ int thread, int task, int inferior,
+ int ignore_count,
int from_tty,
int enabled, int internal, unsigned flags)
{
@@ -8457,7 +8527,7 @@ create_breakpoints_sal (struct gdbarch *gdbarch,
std::move (cond_string),
std::move (extra_string),
type, disposition,
- thread, task, ignore_count,
+ thread, task, inferior, ignore_count,
from_tty, enabled, internal, flags,
canonical->special_display);
}
@@ -8597,11 +8667,12 @@ check_fast_tracepoint_sals (struct gdbarch *gdbarch,
static void
find_condition_and_thread (const char *tok, CORE_ADDR pc,
gdb::unique_xmalloc_ptr<char> *cond_string,
- int *thread, int *task,
+ int *thread, int *inferior, int *task,
gdb::unique_xmalloc_ptr<char> *rest)
{
cond_string->reset ();
*thread = -1;
+ *inferior = -1;
*task = 0;
rest->reset ();
bool force = false;
@@ -8659,6 +8730,16 @@ find_condition_and_thread (const char *tok, CORE_ADDR pc,
*thread = thr->global_num;
tok = tmptok;
}
+ else if (toklen >= 1 && strncmp (tok, "inferior", toklen) == 0)
+ {
+ char *tmptok;
+
+ tok = end_tok + 1;
+ *inferior = strtol (tok, &tmptok, 0);
+ if (tok == tmptok)
+ error (_("Junk after thread keyword."));
+ tok = tmptok;
+ }
else if (toklen >= 1 && strncmp (tok, "task", toklen) == 0)
{
char *tmptok;
@@ -8690,7 +8771,7 @@ static void
find_condition_and_thread_for_sals (const std::vector<symtab_and_line> &sals,
const char *input,
gdb::unique_xmalloc_ptr<char> *cond_string,
- int *thread, int *task,
+ int *thread, int *inferior, int *task,
gdb::unique_xmalloc_ptr<char> *rest)
{
int num_failures = 0;
@@ -8698,6 +8779,7 @@ find_condition_and_thread_for_sals (const std::vector<symtab_and_line> &sals,
{
gdb::unique_xmalloc_ptr<char> cond;
int thread_id = 0;
+ int inferior_id = 0;
int task_id = 0;
gdb::unique_xmalloc_ptr<char> remaining;
@@ -8710,9 +8792,10 @@ find_condition_and_thread_for_sals (const std::vector<symtab_and_line> &sals,
try
{
find_condition_and_thread (input, sal.pc, &cond, &thread_id,
- &task_id, &remaining);
+ &inferior_id, &task_id, &remaining);
*cond_string = std::move (cond);
*thread = thread_id;
+ *inferior = inferior_id;
*task = task_id;
*rest = std::move (remaining);
break;
@@ -8814,6 +8897,7 @@ create_breakpoint (struct gdbarch *gdbarch,
struct linespec_result canonical;
int pending = 0;
int task = 0;
+ int inferior = 0;
int prev_bkpt_count = breakpoint_count;
gdb_assert (ops != NULL);
@@ -8891,7 +8975,8 @@ create_breakpoint (struct gdbarch *gdbarch,
const linespec_sals &lsal = canonical.lsals[0];
find_condition_and_thread_for_sals (lsal.sals, extra_string,
- &cond, &thread, &task, &rest);
+ &cond, &thread, &inferior,
+ &task, &rest);
cond_string_copy = std::move (cond);
extra_string_copy = std::move (rest);
}
@@ -8941,7 +9026,7 @@ create_breakpoint (struct gdbarch *gdbarch,
std::move (extra_string_copy),
type_wanted,
tempflag ? disp_del : disp_donttouch,
- thread, task, ignore_count,
+ thread, task, inferior, ignore_count,
from_tty, enabled, internal, flags);
}
else
@@ -9886,6 +9971,7 @@ watch_command_1 (const char *arg, int accessflag, int from_tty,
const char *cond_end = NULL;
enum bptype bp_type;
int thread = -1;
+ int inferior = -1;
/* Flag to indicate whether we are going to use masks for
the hardware watchpoint. */
bool use_mask = false;
@@ -9956,6 +10042,15 @@ watch_command_1 (const char *arg, int accessflag, int from_tty,
if (!valid_task_id (task))
error (_("Unknown task %d."), task);
}
+ else if (toklen == 8 && startswith (tok, "inferior"))
+ {
+ char *tmp;
+
+ inferior = strtol (value_start, &tmp, 0);
+ if (tmp == value_start)
+ error (_("Junk after task keyword."));
+ /* TODO: Validate the inferior number. */
+ }
else if (toklen == 4 && startswith (tok, "mask"))
{
/* We've found a "mask" token, which means the user wants to
@@ -10127,6 +10222,7 @@ watch_command_1 (const char *arg, int accessflag, int from_tty,
w.reset (new watchpoint (nullptr, bp_type));
w->thread = thread;
+ w->inferior = inferior;
w->task = task;
w->disposition = disp_donttouch;
w->pspace = current_program_space;
@@ -12026,7 +12122,8 @@ strace_marker_create_breakpoints_sal (struct gdbarch *gdbarch,
enum bptype type_wanted,
enum bpdisp disposition,
int thread,
- int task, int ignore_count,
+ int task, int inferior,
+ int ignore_count,
int from_tty, int enabled,
int internal, unsigned flags)
{
@@ -12052,7 +12149,8 @@ strace_marker_create_breakpoints_sal (struct gdbarch *gdbarch,
std::move (cond_string),
std::move (extra_string),
disposition,
- thread, task, ignore_count,
+ /* TODO: Inferior. */
+ thread, task, -1, ignore_count,
from_tty, enabled, flags,
canonical->special_display));
@@ -12685,10 +12783,11 @@ code_breakpoint::location_spec_to_sals (location_spec *locspec,
if (condition_not_parsed && extra_string != NULL)
{
gdb::unique_xmalloc_ptr<char> local_cond, local_extra;
- int local_thread, local_task;
+ int local_thread, local_task, local_inferior;
find_condition_and_thread_for_sals (sals, extra_string.get (),
&local_cond, &local_thread,
+ &local_inferior,
&local_task, &local_extra);
gdb_assert (cond_string == nullptr);
if (local_cond != nullptr)
@@ -14806,4 +14905,6 @@ This is useful for formatted output in user-defined commands."));
"breakpoint");
gdb::observers::thread_exit.attach (remove_threaded_breakpoints,
"breakpoint");
+ gdb::observers::inferior_exit.attach (remove_inferior_breakpoints,
+ "breakpoint");
}
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index 360fa760577..f1df5128c0e 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -584,7 +584,7 @@ struct breakpoint_ops
struct linespec_result *,
gdb::unique_xmalloc_ptr<char>,
gdb::unique_xmalloc_ptr<char>,
- enum bptype, enum bpdisp, int, int,
+ enum bptype, enum bpdisp, int, int, int,
int, int, int, int, unsigned);
};
@@ -801,6 +801,9 @@ struct breakpoint
care. */
int thread = -1;
+ /* ... */
+ int inferior = -1;
+
/* Ada task number for task-specific breakpoint, or 0 if don't
care. */
int task = 0;
@@ -856,7 +859,7 @@ struct code_breakpoint : public breakpoint
gdb::unique_xmalloc_ptr<char> cond_string,
gdb::unique_xmalloc_ptr<char> extra_string,
enum bpdisp disposition,
- int thread, int task, int ignore_count,
+ int thread, int task, int inferior, int ignore_count,
int from_tty,
int enabled, unsigned flags,
int display_canonical);
diff --git a/gdb/dummy-frame.c b/gdb/dummy-frame.c
index 40b455c7e4a..5e48dca2982 100644
--- a/gdb/dummy-frame.c
+++ b/gdb/dummy-frame.c
@@ -132,6 +132,7 @@ pop_dummy_frame_bpt (struct breakpoint *b, struct dummy_frame *dummy)
if (b->thread == dummy->id.thread->global_num
&& b->disposition == disp_del && frame_id_eq (b->frame_id, dummy->id.id))
{
+ gdb_assert (b->inferior == -1);
while (b->related_breakpoint != b)
delete_breakpoint (b->related_breakpoint);
diff --git a/gdb/elfread.c b/gdb/elfread.c
index 13dbaf0227a..0087b19b44b 100644
--- a/gdb/elfread.c
+++ b/gdb/elfread.c
@@ -947,7 +947,10 @@ elf_gnu_ifunc_resolver_stop (code_breakpoint *b)
if (b_return->thread == thread_id
&& b_return->loc->requested_address == prev_pc
&& frame_id_eq (b_return->frame_id, prev_frame_id))
- break;
+ {
+ gdb_assert (b_return->inferior == -1);
+ break;
+ }
}
if (b_return == b)
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index e9107cef520..68b1fca7c51 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -423,7 +423,10 @@ run_command_1 (const char *args, int from_tty, enum run_how run_how)
/* Insert temporary breakpoint in main function if requested. */
if (run_how == RUN_STOP_AT_MAIN)
{
- std::string arg = string_printf ("-qualified %s if $_inferior == %d", main_name (),
+ //std::string arg = string_printf ("-qualified %s if $_inferior == %d", main_name (),
+ // current_inferior ()->num);
+ std::string arg = string_printf ("-qualified %s inferior %d",
+ main_name (),
current_inferior ()->num);
tbreak_command (arg.c_str (), 0);
}
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 033699bc3f7..e23bbd8dfa1 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -8041,6 +8041,7 @@ insert_exception_resume_breakpoint (struct thread_info *tp,
frame = NULL;
bp->thread = tp->global_num;
+ bp->inferior = -1;
inferior_thread ()->control.exception_resume_breakpoint = bp;
}
}
@@ -8074,6 +8075,7 @@ insert_exception_resume_from_probe (struct thread_info *tp,
bp = set_momentary_breakpoint_at_pc (get_frame_arch (frame),
handler, bp_exception_resume).release ();
bp->thread = tp->global_num;
+ bp->inferior = -1;
inferior_thread ()->control.exception_resume_breakpoint = bp;
}
diff --git a/gdb/linespec.c b/gdb/linespec.c
index 3db35998f7e..ca09f630382 100644
--- a/gdb/linespec.c
+++ b/gdb/linespec.c
@@ -254,7 +254,7 @@ enum linespec_token_type
/* List of keywords. This is NULL-terminated so that it can be used
as enum completer. */
-const char * const linespec_keywords[] = { "if", "thread", "task", "-force-condition", NULL };
+const char * const linespec_keywords[] = { "if", "thread", "task", "inferior", "-force-condition", NULL };
#define IF_KEYWORD_INDEX 0
#define FORCE_KEYWORD_INDEX 3
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] gdb: make "start" breakpoint inferior-specific
2022-08-31 14:03 ` Bruno Larsen
@ 2022-11-04 16:52 ` Simon Marchi
2022-11-07 8:14 ` Bruno Larsen
2022-11-08 17:24 ` Tom Tromey
0 siblings, 2 replies; 28+ messages in thread
From: Simon Marchi @ 2022-11-04 16:52 UTC (permalink / raw)
To: Bruno Larsen, gdb-patches
On 8/31/22 10:03, Bruno Larsen via Gdb-patches wrote:
> On 04/08/2022 19:40, Simon Marchi via Gdb-patches wrote:
>> I saw this failure on a CI:
>>
>> (gdb) add-inferior
>> [New inferior 2]
>> Added inferior 2
>> (gdb) PASS: gdb.threads/vfork-multi-inferior.exp: method=non-stop: add-inferior
>> inferior 2
>> [Switching to inferior 2 [<null>] (<noexec>)]
>> (gdb) PASS: gdb.threads/vfork-multi-inferior.exp: method=non-stop: inferior 2
>> kill
>> The program is not being run.
>> (gdb) file /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/tmp/tmp.GYATAXR8Ku/gdb/testsuite/outputs/gdb.threads/vfork-multi-inferior/vfork-multi-inferior-sleep
>> Reading symbols from /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/tmp/tmp.GYATAXR8Ku/gdb/testsuite/outputs/gdb.threads/vfork-multi-inferior/vfork-multi-inferior-sleep...
>> (gdb) run &
>> Starting program: /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/tmp/tmp.GYATAXR8Ku/gdb/testsuite/outputs/gdb.threads/vfork-multi-inferior/vfork-multi-inferior-sleep
>> (gdb) PASS: gdb.threads/vfork-multi-inferior.exp: method=non-stop: run inferior 2
>> inferior 1
>> [Switching to inferior 1 [<null>] (<noexec>)]
>> (gdb) PASS: gdb.threads/vfork-multi-inferior.exp: method=non-stop: inferior 1
>> kill
>> The program is not being run.
>> (gdb) file /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/tmp/tmp.GYATAXR8Ku/gdb/testsuite/outputs/gdb.threads/vfork-multi-inferior/vfork-multi-inferior
>> Reading symbols from /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/tmp/tmp.GYATAXR8Ku/gdb/testsuite/outputs/gdb.threads/vfork-multi-inferior/vfork-multi-inferior...
>> (gdb) break should_break_here
>> Breakpoint 1 at 0x11b1: file /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/src/binutils-gdb/gdb/testsuite/gdb.threads/vfork-multi-inferior.c, line 25.
>> (gdb) PASS: gdb.threads/vfork-multi-inferior.exp: method=non-stop: break should_break_here
>> [Thread debugging using libthread_db enabled]
>> Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
>> start
>> Temporary breakpoint 2 at 0x11c0: -qualified main. (2 locations)
>> Starting program: /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/tmp/tmp.GYATAXR8Ku/gdb/testsuite/outputs/gdb.threads/vfork-multi-inferior/vfork-multi-inferior
>> [Thread debugging using libthread_db enabled]
>> Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
>>
>> Thread 2.1 "vfork-multi-inf" hit Temporary breakpoint 2, main () at /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/src/binutils-gdb/gdb/testsuite/gdb.threads/vfork-multi-inferior-sleep.c:23
>> 23 sleep (30);
>> (gdb) FAIL: gdb.threads/vfork-multi-inferior.exp: method=non-stop: start inferior 1
>>
>> What happens is:
>>
>> 1. We start inferior 2 with "run&", it runs very slowly, takes time to
>> get to main
>> 2. We switch to inferior 1, and run "start"
>> 3. The temporary breakpoint inserted by "start" applies to all inferiors
>> 4. Inferior 2 hits that breakpoint and GDB reports that hit
>>
>> To avoid this, breakpoints inserted by "start" should be
>> inferior-specific. However, we don't have a nice way to make
>> inferior-specific breakpoints yet. It's possible to make
>> pspace-specific breakpoints (for example how the internal_breakpoint
>> constructor does) by creating a symtab_and_line manually. However,
>> inferiors can share program spaces (usually on particular embedded
>> targets), so we could have a situation where two inferiors run the same
>> code in the same program space. In that case, it would just not be
>> possible to insert a breakpoint in one inferior but not the other.
>>
>> A simple solution that should work all the time is to add a condition to
>> the breakpoint inserted by "start", to check the inferior reporting the
>> hit is the expected one. This is what this patch implements.
>>
>> Add a test that does:
>>
>> - start a background inferior 1 that calls main in a loop
>> - start aninferior 2 with the "start" command
>> - validate that we hit the breakpoint in inferior 2
>>
>> Without the fix, we hit the breakpoint in inferior 1 pretty much all the
>> time.
>>
>> Change-Id: Ib0148498a476bfa634ed62353c95f163623c686a
>
> Hi Simon,
>
> This change makes a lot of sense, I just wonder if it would make sense to make breakpoints inferior-specific. I could see a situation where someone would run the same program twice, one with an ok input while the other has a buggy input for instance, to see the difference between them, and inferior-specific breakpoints could come in handy in a situation like this.
>
> If you think it would be too much work for a usecase that is too niche, this patch is pretty straight forward, I'd suggest you approve your patch.
I certainly don't want to add a new user-facing feature just for this
fix, as that's a lot of work (figuring out the syntax, doc, NEWS, tests,
etc). We could enhance the internal API to allow passing an inferior to
make a breakpoint inferior-specific, but I don't think it should be a
prerequisite for this fix. It would end up doing functionally
identical.
Would you like to to add your Reviewed-By on this patch?
Simon
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] gdb: make "start" breakpoint inferior-specific
2022-09-01 10:42 ` Andrew Burgess
@ 2022-11-04 17:24 ` Simon Marchi
[not found] ` <8735asb7cj.fsf@redhat.com>
0 siblings, 1 reply; 28+ messages in thread
From: Simon Marchi @ 2022-11-04 17:24 UTC (permalink / raw)
To: Andrew Burgess, Simon Marchi via Gdb-patches; +Cc: Simon Marchi
> I think test cases like this should have an `alarm` call, just in case
> something goes wrong, this'll stop the test running forever.
Done, added this:
diff --git a/gdb/testsuite/gdb.multi/start-inferior-specific-other.c b/gdb/testsuite/gdb.multi/start-inferior-specific-other.c
index b593c3aa068..522971e81d4 100644
--- a/gdb/testsuite/gdb.multi/start-inferior-specific-other.c
+++ b/gdb/testsuite/gdb.multi/start-inferior-specific-other.c
@@ -16,6 +16,7 @@
along with this program. If not, see <http://www.gnu.org/licenses/>. */
#include <stdlib.h>
+#include <unistd.h>
int
main (int argc, const char **argv)
@@ -23,6 +24,8 @@ main (int argc, const char **argv)
if (argc == 999)
return 0;
+ alarm (30);
+
for (;;)
main (999, NULL);
>
>> diff --git a/gdb/testsuite/gdb.multi/start-inferior-specific.c b/gdb/testsuite/gdb.multi/start-inferior-specific.c
>> new file mode 100644
>> index 00000000000..b69e218962a
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.multi/start-inferior-specific.c
>> @@ -0,0 +1,22 @@
>> +/* 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/>. */
>> +
>> +int
>> +main ()
>> +{
>> + return 0;
>> +}
>> diff --git a/gdb/testsuite/gdb.multi/start-inferior-specific.exp b/gdb/testsuite/gdb.multi/start-inferior-specific.exp
>> new file mode 100644
>> index 00000000000..5a5572f591a
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.multi/start-inferior-specific.exp
>> @@ -0,0 +1,58 @@
>> +# 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/>.
>> +
>> +# Test that "start"ing an inferior does not inadvertently stop in another
>> +# inferior.
>> +#
>> +# To achieve this, we have an inferior (the "other") running in background
>> +# constantly re-calling main. We then "start" a second inferior. A buggy GDB
>> +# would report a breakpoint hit in "other".
>> +
>> +standard_testfile .c -other.c
>> +
>> +if { [use_gdb_stub] } {
>> + return
>> +}
>> +
>> +set srcfile_other ${srcfile2}
>> +set binfile_other ${binfile}-other
>> +
>> +if { [build_executable ${testfile}.exp ${binfile} "${srcfile}" {debug}] != 0 } {
>> + return -1
>> +}
>> +
>> +if { [build_executable ${testfile}.exp ${binfile_other} "${srcfile_other}" {debug}] != 0 } {
>> + return -1
>> +}
>> +
>> +proc do_test {} {
>> + # With remote, to be able to start an inferior while another one is
>> + # running, we need to use the non-stop variant of the protocol.
>> + save_vars { ::GDBFLAGS } {
>> + if { [target_info gdb_protocol] == "extended-remote"} {
>> + append ::GDBFLAGS " -ex \"maintenance set target-non-stop on\""
>> + }
>> +
>> + clean_restart ${::binfile_other}
>> + }
>> +
>> + gdb_test "run&" "Starting program: .*" "start background inferior"
>> + gdb_test "add-inferior" "Added inferior 2.*"
>> + gdb_test "inferior 2" "Switching to inferior 2.*"
>> + gdb_file_cmd ${::binfile}
>> + gdb_test "start" "Thread 2.1 .* hit Temporary breakpoint .*"
>
> I'm pretty sure there's a race here. We could set the breakpoint for
> the 'start', start inferior 2 executing, hit the b/p and remove it, all
> within the time it takes inferior 1 to perform a single iteration ... in
> theory, though it does seem unlikely.
Hmm, but it would just mean that we have a teeny tiny chance of not
catching the bug. A race condition in this way is more acceptable than
the opposite (the kind that gives random failures), especially if the
risk is super low.
If you think that the startup time of inferior 1 could be significant
enough to cause trouble, we could always run it until its main, then
continue. In that case we would know it's already in its tight loop by
the time we start inferior 2. Since I added that alarm call, inferior 1
has a bit more work to do before getting to the tight loop, so I might
as well do that just to be sure. Even then, there is always a
theoritical chance that inferior 1 gets scheduled out for the whole time
of inferior 2's "start". I don't think there is any way around that,
but I also think it doesn't matter given the low probability, and the
fact that over many test runs, it won't happen all the time.
> If, before the `start` you did `break *main` then we should hit the
> `*main` breakpoint before the temporary `start` breakpoint, the
> temporary breakpoint is left in place, which gives inferior 1 additional
> time to not hit the breakpoint.
I don't understand your proposal. You mean that the start of inferior 2
would hit that breakpoint on *main?
But what you describe makes me think that we don't really handle well
the case of "start" hitting another breakpoint than the one we insert.
I just tried debugging this program:
#include <stdio.h>
__attribute__((constructor)) void func(void)
{
printf("yoyoyo\n");
}
int main()
{
printf("Hi\n");
return 0;
}
$ ./gdb -nx -q --data-directory=data-directory a.out
Reading symbols from a.out...
(gdb) b func
Breakpoint 1 at 0x1151: file test.c, line 5.
(gdb) start
Temporary breakpoint 2 at 0x116b: file test.c, line 10.
Starting program: /home/smarchi/build/binutils-gdb/gdb/a.out
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
Breakpoint 1, func () at test.c:5
5 printf("yoyoyo\n");
(gdb) i b
Num Type Disp Enb Address What
1 breakpoint keep y 0x0000555555555151 in func at test.c:5
breakpoint already hit 1 time
2 breakpoint del y 0x000055555555516b in main at test.c:10
stop only if $_inferior == 1
(gdb) c
Continuing.
yoyoyo
Temporary breakpoint 2, main () at test.c:10
10 printf("Hi\n");
I believe that we should delete the breakpoint on main if we present the
user another stop. But I don't intend to fix this as part of this
patch.
> Of course, this relies on the `*main` and `start` breakpoints being a`t
> different addresses, which should be the case if main has some prologue
> to skip over.
>
> I guess, ideally we would also do something to confirm that inferior 1
> has gone past main while the temporary breakpoint is in place, right now
> we're just relying on inferior 1 running fast enough.
>
> In general I'm happy enough with the actual GDB change you propose here,
> but like Bruno, I also wondered if it would be better to add inferior
> specific breakpoints as an actual thing, rather than relying on the
> condition logic like you do here?
>
> I wasn't sure how you'd feel about this idea, so I didn't spend too
> long, but below is a VERY rough proof of concept patch, that appies on
> top of yours, that adds inferior specific breakpoints. You temporary
> breakpoint now becomes:
>
> tbreak -qualified main inferior 1
>
> But this functionality would also be available to a user if they wanted
> it, which might be nice.
I think it's a good idea, but I would prefer if that wasn't a
prerequesite for this patch, since adding this feature can become
somewhat of a rabbit hole. Functionally, a condition will essentially
behave the same, I think it's robust, and I would like to get rid of the
flakiness in the testsuite sooner than later :).
> When I say the code below is rough, I really mean it, here's a list of
> things I think still need doing:
>
> * I initially added a `breakpoint::inferior` field just like we have a
> `breakpoint::thread` field, but I think a better solution would be
> to change `breakpoint::thread` into a ptid_t and store inferior and
> thread information in that one field,
The problem is that you could create inferior-specific breakpoints
before you have execution, so before you have a p(t)id.
Another thing we need to handle (haven't looked at your code yet, maybe
you already do) is when an inferior gets deleted, breakpoints specific
to that inferior should probably be deleted too.
> * Obviously it should give an error if a user specifies both `thread`
> and `inferior` for a breakpoint,
Makes sense.
> * The `info breakpoints` output is fine for single location
> breakpoints, but I think multi-location breakpoints would need
> sorting out still,
Not sure what you are referring to exactly, but if you have two
inferiors with two distinct program spaces, I think we could manage to
create only a location in the program space attached to the inferior we
target. If we have two inferiors sharing a program space, I guess there
will be a single location in any case.
> * There's a couple of TODO comments in there still to address,
>
> * Need to write docs, NEWS, and add tests,
>
> Like I said, you might see a reason why this is a bad idea, so I didn't
> want to put any more work in without having a discussion about the idea.
>
> Thoughts?
As I said, I would prefer not to make this work a prerequisite for the
fix. It would be possible to change the internal API to make it
possible to make breakpoints inferior-specific, internally, but that
just seems more trouble and more risky for nothing. In the end, it
would just end up doing the same comparison, but in a different way.
I'm unable to apply your patch unfortunately, for some reason, and I'm
not good at reading raw diffs. I could look at it if you sent a clean
version (could be an RFC).
Simon
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] gdb: make "start" breakpoint inferior-specific
2022-11-04 16:52 ` Simon Marchi
@ 2022-11-07 8:14 ` Bruno Larsen
2022-11-08 17:24 ` Tom Tromey
1 sibling, 0 replies; 28+ messages in thread
From: Bruno Larsen @ 2022-11-07 8:14 UTC (permalink / raw)
To: Simon Marchi, gdb-patches
On 04/11/2022 17:52, Simon Marchi wrote:
> On 8/31/22 10:03, Bruno Larsen via Gdb-patches wrote:
>> On 04/08/2022 19:40, Simon Marchi via Gdb-patches wrote:
>>> I saw this failure on a CI:
>>>
>>> (gdb) add-inferior
>>> [New inferior 2]
>>> Added inferior 2
>>> (gdb) PASS: gdb.threads/vfork-multi-inferior.exp: method=non-stop: add-inferior
>>> inferior 2
>>> [Switching to inferior 2 [<null>] (<noexec>)]
>>> (gdb) PASS: gdb.threads/vfork-multi-inferior.exp: method=non-stop: inferior 2
>>> kill
>>> The program is not being run.
>>> (gdb) file /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/tmp/tmp.GYATAXR8Ku/gdb/testsuite/outputs/gdb.threads/vfork-multi-inferior/vfork-multi-inferior-sleep
>>> Reading symbols from /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/tmp/tmp.GYATAXR8Ku/gdb/testsuite/outputs/gdb.threads/vfork-multi-inferior/vfork-multi-inferior-sleep...
>>> (gdb) run &
>>> Starting program: /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/tmp/tmp.GYATAXR8Ku/gdb/testsuite/outputs/gdb.threads/vfork-multi-inferior/vfork-multi-inferior-sleep
>>> (gdb) PASS: gdb.threads/vfork-multi-inferior.exp: method=non-stop: run inferior 2
>>> inferior 1
>>> [Switching to inferior 1 [<null>] (<noexec>)]
>>> (gdb) PASS: gdb.threads/vfork-multi-inferior.exp: method=non-stop: inferior 1
>>> kill
>>> The program is not being run.
>>> (gdb) file /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/tmp/tmp.GYATAXR8Ku/gdb/testsuite/outputs/gdb.threads/vfork-multi-inferior/vfork-multi-inferior
>>> Reading symbols from /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/tmp/tmp.GYATAXR8Ku/gdb/testsuite/outputs/gdb.threads/vfork-multi-inferior/vfork-multi-inferior...
>>> (gdb) break should_break_here
>>> Breakpoint 1 at 0x11b1: file /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/src/binutils-gdb/gdb/testsuite/gdb.threads/vfork-multi-inferior.c, line 25.
>>> (gdb) PASS: gdb.threads/vfork-multi-inferior.exp: method=non-stop: break should_break_here
>>> [Thread debugging using libthread_db enabled]
>>> Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
>>> start
>>> Temporary breakpoint 2 at 0x11c0: -qualified main. (2 locations)
>>> Starting program: /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/tmp/tmp.GYATAXR8Ku/gdb/testsuite/outputs/gdb.threads/vfork-multi-inferior/vfork-multi-inferior
>>> [Thread debugging using libthread_db enabled]
>>> Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
>>>
>>> Thread 2.1 "vfork-multi-inf" hit Temporary breakpoint 2, main () at /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/src/binutils-gdb/gdb/testsuite/gdb.threads/vfork-multi-inferior-sleep.c:23
>>> 23 sleep (30);
>>> (gdb) FAIL: gdb.threads/vfork-multi-inferior.exp: method=non-stop: start inferior 1
>>>
>>> What happens is:
>>>
>>> 1. We start inferior 2 with "run&", it runs very slowly, takes time to
>>> get to main
>>> 2. We switch to inferior 1, and run "start"
>>> 3. The temporary breakpoint inserted by "start" applies to all inferiors
>>> 4. Inferior 2 hits that breakpoint and GDB reports that hit
>>>
>>> To avoid this, breakpoints inserted by "start" should be
>>> inferior-specific. However, we don't have a nice way to make
>>> inferior-specific breakpoints yet. It's possible to make
>>> pspace-specific breakpoints (for example how the internal_breakpoint
>>> constructor does) by creating a symtab_and_line manually. However,
>>> inferiors can share program spaces (usually on particular embedded
>>> targets), so we could have a situation where two inferiors run the same
>>> code in the same program space. In that case, it would just not be
>>> possible to insert a breakpoint in one inferior but not the other.
>>>
>>> A simple solution that should work all the time is to add a condition to
>>> the breakpoint inserted by "start", to check the inferior reporting the
>>> hit is the expected one. This is what this patch implements.
>>>
>>> Add a test that does:
>>>
>>> - start a background inferior 1 that calls main in a loop
>>> - start aninferior 2 with the "start" command
>>> - validate that we hit the breakpoint in inferior 2
>>>
>>> Without the fix, we hit the breakpoint in inferior 1 pretty much all the
>>> time.
>>>
>>> Change-Id: Ib0148498a476bfa634ed62353c95f163623c686a
>> Hi Simon,
>>
>> This change makes a lot of sense, I just wonder if it would make sense to make breakpoints inferior-specific. I could see a situation where someone would run the same program twice, one with an ok input while the other has a buggy input for instance, to see the difference between them, and inferior-specific breakpoints could come in handy in a situation like this.
>>
>> If you think it would be too much work for a usecase that is too niche, this patch is pretty straight forward, I'd suggest you approve your patch.
> I certainly don't want to add a new user-facing feature just for this
> fix, as that's a lot of work (figuring out the syntax, doc, NEWS, tests,
> etc). We could enhance the internal API to allow passing an inferior to
> make a breakpoint inferior-specific, but I don't think it should be a
> prerequisite for this fix. It would end up doing functionally
> identical.
What I had in mind is closer to your second idea, just internal API
changes to keep track of the inferior, though maybe I just hadn't
thought through the suggestion, since letting users add breakpoint to
other inferiors makes sense given the use case I mentioned above.
This is a long winded way of saying I agree that this is unreasonable
feature creep for a functionally identical end result for now, so the
patch LGTM.
Reviewed-By: Bruno Larsen <blarsen@redhat.com>
Cheers,
Bruno
>
> Would you like to to add your Reviewed-By on this patch?
>
> Simon
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] gdb: make "start" breakpoint inferior-specific
2022-11-04 16:52 ` Simon Marchi
2022-11-07 8:14 ` Bruno Larsen
@ 2022-11-08 17:24 ` Tom Tromey
1 sibling, 0 replies; 28+ messages in thread
From: Tom Tromey @ 2022-11-08 17:24 UTC (permalink / raw)
To: Simon Marchi via Gdb-patches; +Cc: Bruno Larsen, Simon Marchi
>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
Simon> I certainly don't want to add a new user-facing feature just for this
Simon> fix, as that's a lot of work (figuring out the syntax, doc, NEWS, tests,
Simon> etc). We could enhance the internal API to allow passing an inferior to
Simon> make a breakpoint inferior-specific, but I don't think it should be a
Simon> prerequisite for this fix. It would end up doing functionally
Simon> identical.
I didn't really follow this thread, but giving the user the ability to
restrict breakpoints has been on the multi-* wishlist forever. I'm not
sure if there's any design document readily available, though we've had
discussions about it in the past.
Tom
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] gdb: make "start" breakpoint inferior-specific
2022-08-04 17:40 [PATCH] gdb: make "start" breakpoint inferior-specific Simon Marchi
` (2 preceding siblings ...)
2022-09-01 10:42 ` Andrew Burgess
@ 2022-11-08 19:43 ` Pedro Alves
2022-11-08 20:14 ` Simon Marchi
3 siblings, 1 reply; 28+ messages in thread
From: Pedro Alves @ 2022-11-08 19:43 UTC (permalink / raw)
To: Simon Marchi, gdb-patches
On 2022-08-04 6:40 p.m., Simon Marchi via Gdb-patches wrote:
> +int
> +main (int argc, const char **argv)
> +{
> + if (argc == 999)
> + return 0;
> +
> + for (;;)
> + main (999, NULL);
> +
Calling main triggers undefined behavior. The C++ Standard says [1]
"The function main shall not be used within a program."
E.g., on some targets, global constructors are called from the start of main, by a magic
call the compiler injects. Calling main like that would run global ctors twice on such
ports.
[1] https://eel.is/c++draft/basic.start.main#3
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] gdb: make "start" breakpoint inferior-specific
2022-11-08 19:43 ` Pedro Alves
@ 2022-11-08 20:14 ` Simon Marchi
2022-11-08 21:09 ` Pedro Alves
0 siblings, 1 reply; 28+ messages in thread
From: Simon Marchi @ 2022-11-08 20:14 UTC (permalink / raw)
To: Pedro Alves, gdb-patches
On 11/8/22 14:43, Pedro Alves wrote:
> On 2022-08-04 6:40 p.m., Simon Marchi via Gdb-patches wrote:
>> +int
>> +main (int argc, const char **argv)
>> +{
>> + if (argc == 999)
>> + return 0;
>> +
>> + for (;;)
>> + main (999, NULL);
>> +
>
> Calling main triggers undefined behavior. The C++ Standard says [1]
> "The function main shall not be used within a program."
>
> E.g., on some targets, global constructors are called from the start of main, by a magic
> call the compiler injects. Calling main like that would run global ctors twice on such
> ports.
>
> [1] https://eel.is/c++draft/basic.start.main#3
How on earth do you know the C++ standard by heart?
Ok, I think I found an alternative solution for the test, to make the
inferiors sleep a bit before main. I can achieve that in a C program
using __attribute__((constructor)), but I believe that's a GCC
extension? Otherwise, I can do it in C++ using the constructor of a
global object. Which one would be best? Or, do you see another way?
Simon
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] gdb: make "start" breakpoint inferior-specific
2022-11-08 20:14 ` Simon Marchi
@ 2022-11-08 21:09 ` Pedro Alves
2022-11-08 21:20 ` [PATCH v2] " Simon Marchi
0 siblings, 1 reply; 28+ messages in thread
From: Pedro Alves @ 2022-11-08 21:09 UTC (permalink / raw)
To: Simon Marchi, gdb-patches
On 2022-11-08 8:14 p.m., Simon Marchi wrote:
> Ok, I think I found an alternative solution for the test, to make the
> inferiors sleep a bit before main. I can achieve that in a C program
> using __attribute__((constructor)), but I believe that's a GCC
> extension? Otherwise, I can do it in C++ using the constructor of a
> global object. Which one would be best? Or, do you see another way?
A C++ global object sounds best.
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v2] gdb: make "start" breakpoint inferior-specific
2022-11-08 21:09 ` Pedro Alves
@ 2022-11-08 21:20 ` Simon Marchi
2022-11-10 16:45 ` Pedro Alves
2022-11-11 12:37 ` Tom de Vries
0 siblings, 2 replies; 28+ messages in thread
From: Simon Marchi @ 2022-11-08 21:20 UTC (permalink / raw)
To: gdb-patches; +Cc: Simon Marchi
New in v2:
- Change the test so it doesn't call the main function
I saw this failure on a CI:
(gdb) add-inferior
[New inferior 2]
Added inferior 2
(gdb) PASS: gdb.threads/vfork-multi-inferior.exp: method=non-stop: add-inferior
inferior 2
[Switching to inferior 2 [<null>] (<noexec>)]
(gdb) PASS: gdb.threads/vfork-multi-inferior.exp: method=non-stop: inferior 2
kill
The program is not being run.
(gdb) file /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/tmp/tmp.GYATAXR8Ku/gdb/testsuite/outputs/gdb.threads/vfork-multi-inferior/vfork-multi-inferior-sleep
Reading symbols from /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/tmp/tmp.GYATAXR8Ku/gdb/testsuite/outputs/gdb.threads/vfork-multi-inferior/vfork-multi-inferior-sleep...
(gdb) run &
Starting program: /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/tmp/tmp.GYATAXR8Ku/gdb/testsuite/outputs/gdb.threads/vfork-multi-inferior/vfork-multi-inferior-sleep
(gdb) PASS: gdb.threads/vfork-multi-inferior.exp: method=non-stop: run inferior 2
inferior 1
[Switching to inferior 1 [<null>] (<noexec>)]
(gdb) PASS: gdb.threads/vfork-multi-inferior.exp: method=non-stop: inferior 1
kill
The program is not being run.
(gdb) file /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/tmp/tmp.GYATAXR8Ku/gdb/testsuite/outputs/gdb.threads/vfork-multi-inferior/vfork-multi-inferior
Reading symbols from /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/tmp/tmp.GYATAXR8Ku/gdb/testsuite/outputs/gdb.threads/vfork-multi-inferior/vfork-multi-inferior...
(gdb) break should_break_here
Breakpoint 1 at 0x11b1: file /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/src/binutils-gdb/gdb/testsuite/gdb.threads/vfork-multi-inferior.c, line 25.
(gdb) PASS: gdb.threads/vfork-multi-inferior.exp: method=non-stop: break should_break_here
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
start
Temporary breakpoint 2 at 0x11c0: -qualified main. (2 locations)
Starting program: /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/tmp/tmp.GYATAXR8Ku/gdb/testsuite/outputs/gdb.threads/vfork-multi-inferior/vfork-multi-inferior
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
Thread 2.1 "vfork-multi-inf" hit Temporary breakpoint 2, main () at /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/src/binutils-gdb/gdb/testsuite/gdb.threads/vfork-multi-inferior-sleep.c:23
23 sleep (30);
(gdb) FAIL: gdb.threads/vfork-multi-inferior.exp: method=non-stop: start inferior 1
What happens is:
1. We start inferior 2 with "run&", it runs very slowly, takes time to
get to main
2. We switch to inferior 1, and run "start"
3. The temporary breakpoint inserted by "start" applies to all inferiors
4. Inferior 2 hits that breakpoint and GDB reports that hit
To avoid this, breakpoints inserted by "start" should be
inferior-specific. However, we don't have a nice way to make
inferior-specific breakpoints yet. It's possible to make
pspace-specific breakpoints (for example how the internal_breakpoint
constructor does) by creating a symtab_and_line manually. However,
inferiors can share program spaces (usually on particular embedded
targets), so we could have a situation where two inferiors run the same
code in the same program space. In that case, it would just not be
possible to insert a breakpoint in one inferior but not the other.
A simple solution that should work all the time is to add a condition to
the breakpoint inserted by "start", to check the inferior reporting the
hit is the expected one. This is what this patch implements.
Add a test that does:
- start in background inferior 1 that sleeps 3 seconds before reaching
its main function (using a sleep in a global C++ object constructor)
- start inferior 2, which also sleeps 3 seconds before reaching its m
ain function, with the "start" command
- validate that we hit the breakpoint in inferior 2
Without the fix, we hit the breakpoint in inferior 1 pretty much all the
time. There could be some unfortunate scheduling causing the test not
to catch the bug, for instance if the scheduler decides not to schedule
inferior 1 for a long time, but it would be really rare. If the bug is
re-introduced, the test will catch it much more often than not, so it
will be noticed.
Change-Id: Ib0148498a476bfa634ed62353c95f163623c686a
---
gdb/infcmd.c | 3 +-
.../start-inferior-specific-other.cc | 35 +++++++++++
.../gdb.multi/start-inferior-specific.cc | 32 ++++++++++
.../gdb.multi/start-inferior-specific.exp | 61 +++++++++++++++++++
4 files changed, 130 insertions(+), 1 deletion(-)
create mode 100644 gdb/testsuite/gdb.multi/start-inferior-specific-other.cc
create mode 100644 gdb/testsuite/gdb.multi/start-inferior-specific.cc
create mode 100644 gdb/testsuite/gdb.multi/start-inferior-specific.exp
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index c03ca103c91..f91ae33eb25 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -423,7 +423,8 @@ run_command_1 (const char *args, int from_tty, enum run_how run_how)
/* Insert temporary breakpoint in main function if requested. */
if (run_how == RUN_STOP_AT_MAIN)
{
- std::string arg = string_printf ("-qualified %s", main_name ());
+ std::string arg = string_printf ("-qualified %s if $_inferior == %d", main_name (),
+ current_inferior ()->num);
tbreak_command (arg.c_str (), 0);
}
diff --git a/gdb/testsuite/gdb.multi/start-inferior-specific-other.cc b/gdb/testsuite/gdb.multi/start-inferior-specific-other.cc
new file mode 100644
index 00000000000..e6f55d326bb
--- /dev/null
+++ b/gdb/testsuite/gdb.multi/start-inferior-specific-other.cc
@@ -0,0 +1,35 @@
+/* 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/>. */
+
+#include <unistd.h>
+
+struct global
+{
+ global ()
+ {
+ sleep (3);
+ }
+} global;
+
+int
+main (int argc, const char **argv)
+{
+ /* We don't want this program finishing and causing spurious "inferior
+ exited" notifications in GDB, so keep sleeping here. */
+ sleep (60);
+ return 0;
+}
diff --git a/gdb/testsuite/gdb.multi/start-inferior-specific.cc b/gdb/testsuite/gdb.multi/start-inferior-specific.cc
new file mode 100644
index 00000000000..432a6aa8ab8
--- /dev/null
+++ b/gdb/testsuite/gdb.multi/start-inferior-specific.cc
@@ -0,0 +1,32 @@
+/* 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/>. */
+
+#include <unistd.h>
+
+struct global
+{
+ global ()
+ {
+ sleep (3);
+ }
+} global;
+
+int
+main ()
+{
+ return 0;
+}
diff --git a/gdb/testsuite/gdb.multi/start-inferior-specific.exp b/gdb/testsuite/gdb.multi/start-inferior-specific.exp
new file mode 100644
index 00000000000..94d1a956b37
--- /dev/null
+++ b/gdb/testsuite/gdb.multi/start-inferior-specific.exp
@@ -0,0 +1,61 @@
+# 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/>.
+
+# Test that "start"ing an inferior does not inadvertently stop in another
+# inferior.
+#
+# To achieve this, we start an inferior (the "other") in background, which
+# sleeps for 3 seconds before reaching its main function. We then "start" a
+# second inferior, which also sleeps for 3 seconds before reaching its main
+# function. A buggy GDB would report a breakpoint hit in "other".
+
+standard_testfile .cc -other.cc
+
+if { [use_gdb_stub] } {
+ return
+}
+
+set srcfile_other ${srcfile2}
+set binfile_other ${binfile}-other
+
+if { [build_executable ${testfile}.exp ${binfile} "${srcfile}" \
+ {debug c++}] != 0 } {
+ return -1
+}
+
+if { [build_executable ${testfile}.exp ${binfile_other} "${srcfile_other}" \
+ {debug c++}] != 0 } {
+ return -1
+}
+
+proc do_test {} {
+ # With remote, to be able to start an inferior while another one is
+ # running, we need to use the non-stop variant of the protocol.
+ save_vars { ::GDBFLAGS } {
+ if { [target_info gdb_protocol] == "extended-remote"} {
+ append ::GDBFLAGS " -ex \"maintenance set target-non-stop on\""
+ }
+
+ clean_restart ${::binfile_other}
+ }
+
+ gdb_test "run&" "Starting program: .*" "start background inferior"
+ gdb_test "add-inferior" "Added inferior 2.*"
+ gdb_test "inferior 2" "Switching to inferior 2.*"
+ gdb_file_cmd ${::binfile}
+ gdb_test "start" "Thread 2.1 .* hit Temporary breakpoint .*"
+}
+
+do_test
--
2.37.3
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] gdb: make "start" breakpoint inferior-specific
[not found] ` <8735asb7cj.fsf@redhat.com>
@ 2022-11-09 13:19 ` Simon Marchi
0 siblings, 0 replies; 28+ messages in thread
From: Simon Marchi @ 2022-11-09 13:19 UTC (permalink / raw)
To: Andrew Burgess; +Cc: Simon Marchi, gdb-patches
On 11/9/22 08:11, Andrew Burgess wrote:
> Simon Marchi <simark@simark.ca> writes:
>
>>>
>>> In general I'm happy enough with the actual GDB change you propose here,
>>> but like Bruno, I also wondered if it would be better to add inferior
>>> specific breakpoints as an actual thing, rather than relying on the
>>> condition logic like you do here?
>>>
>>> I wasn't sure how you'd feel about this idea, so I didn't spend too
>>> long, but below is a VERY rough proof of concept patch, that appies on
>>> top of yours, that adds inferior specific breakpoints. You temporary
>>> breakpoint now becomes:
>>>
>>> tbreak -qualified main inferior 1
>>>
>>> But this functionality would also be available to a user if they wanted
>>> it, which might be nice.
>>
>> I think it's a good idea, but I would prefer if that wasn't a
>> prerequesite for this patch, since adding this feature can become
>> somewhat of a rabbit hole. Functionally, a condition will essentially
>> behave the same, I think it's robust, and I would like to get rid of the
>> flakiness in the testsuite sooner than later :).
>
> That's fine - I assume you would be happy if you code was updated to
> make use of inferior specific breakpoints if such a feature appeared in
> the future?
(re-adding gdb-patches, not sure where it got removed, maybe I messed up
my initial reply)
Absolutely, I would be fine. What matters is that the user-visible
behavior works.
Simon
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2] gdb: make "start" breakpoint inferior-specific
2022-11-08 21:20 ` [PATCH v2] " Simon Marchi
@ 2022-11-10 16:45 ` Pedro Alves
2022-11-10 17:33 ` Simon Marchi
2022-11-11 12:37 ` Tom de Vries
1 sibling, 1 reply; 28+ messages in thread
From: Pedro Alves @ 2022-11-10 16:45 UTC (permalink / raw)
To: Simon Marchi, gdb-patches
On 2022-11-08 9:20 p.m., Simon Marchi via Gdb-patches wrote:
> New in v2:
>
> - Change the test so it doesn't call the main function
>
> I saw this failure on a CI:
>
> (gdb) add-inferior
> [New inferior 2]
> Added inferior 2
> (gdb) PASS: gdb.threads/vfork-multi-inferior.exp: method=non-stop: add-inferior
> inferior 2
> [Switching to inferior 2 [<null>] (<noexec>)]
> (gdb) PASS: gdb.threads/vfork-multi-inferior.exp: method=non-stop: inferior 2
> kill
> The program is not being run.
> (gdb) file /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/tmp/tmp.GYATAXR8Ku/gdb/testsuite/outputs/gdb.threads/vfork-multi-inferior/vfork-multi-inferior-sleep
> Reading symbols from /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/tmp/tmp.GYATAXR8Ku/gdb/testsuite/outputs/gdb.threads/vfork-multi-inferior/vfork-multi-inferior-sleep...
> (gdb) run &
> Starting program: /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/tmp/tmp.GYATAXR8Ku/gdb/testsuite/outputs/gdb.threads/vfork-multi-inferior/vfork-multi-inferior-sleep
> (gdb) PASS: gdb.threads/vfork-multi-inferior.exp: method=non-stop: run inferior 2
> inferior 1
> [Switching to inferior 1 [<null>] (<noexec>)]
> (gdb) PASS: gdb.threads/vfork-multi-inferior.exp: method=non-stop: inferior 1
> kill
> The program is not being run.
> (gdb) file /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/tmp/tmp.GYATAXR8Ku/gdb/testsuite/outputs/gdb.threads/vfork-multi-inferior/vfork-multi-inferior
> Reading symbols from /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/tmp/tmp.GYATAXR8Ku/gdb/testsuite/outputs/gdb.threads/vfork-multi-inferior/vfork-multi-inferior...
> (gdb) break should_break_here
> Breakpoint 1 at 0x11b1: file /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/src/binutils-gdb/gdb/testsuite/gdb.threads/vfork-multi-inferior.c, line 25.
> (gdb) PASS: gdb.threads/vfork-multi-inferior.exp: method=non-stop: break should_break_here
> [Thread debugging using libthread_db enabled]
> Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
> start
> Temporary breakpoint 2 at 0x11c0: -qualified main. (2 locations)
> Starting program: /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/tmp/tmp.GYATAXR8Ku/gdb/testsuite/outputs/gdb.threads/vfork-multi-inferior/vfork-multi-inferior
> [Thread debugging using libthread_db enabled]
> Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
>
> Thread 2.1 "vfork-multi-inf" hit Temporary breakpoint 2, main () at /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/src/binutils-gdb/gdb/testsuite/gdb.threads/vfork-multi-inferior-sleep.c:23
> 23 sleep (30);
> (gdb) FAIL: gdb.threads/vfork-multi-inferior.exp: method=non-stop: start inferior 1
>
> What happens is:
>
> 1. We start inferior 2 with "run&", it runs very slowly, takes time to
> get to main
> 2. We switch to inferior 1, and run "start"
> 3. The temporary breakpoint inserted by "start" applies to all inferiors
> 4. Inferior 2 hits that breakpoint and GDB reports that hit
>
> To avoid this, breakpoints inserted by "start" should be
> inferior-specific. However, we don't have a nice way to make
> inferior-specific breakpoints yet. It's possible to make
> pspace-specific breakpoints (for example how the internal_breakpoint
> constructor does) by creating a symtab_and_line manually. However,
> inferiors can share program spaces (usually on particular embedded
> targets), so we could have a situation where two inferiors run the same
> code in the same program space. In that case, it would just not be
> possible to insert a breakpoint in one inferior but not the other.
>
> A simple solution that should work all the time is to add a condition to
> the breakpoint inserted by "start", to check the inferior reporting the
> hit is the expected one. This is what this patch implements.
>
Even though this does work, it still sets the breakpoint on all the pspaces
unnecessarily. It would be nice if the breakpoint was pspace specific, in
addition to inferior specific like you have (or some other way). But, what you
have is fine with me as is, as it is better than what we have today.
Maybe just add a little comment suggesting that it would be even better
to make the breakpoint apply to the current pspace only?
> Add a test that does:
>
> - start in background inferior 1 that sleeps 3 seconds before reaching
> its main function (using a sleep in a global C++ object constructor)
> - start inferior 2, which also sleeps 3 seconds before reaching its m
> ain function, with the "start" command
> - validate that we hit the breakpoint in inferior 2
>
> Without the fix, we hit the breakpoint in inferior 1 pretty much all the
> time. There could be some unfortunate scheduling causing the test not
> to catch the bug, for instance if the scheduler decides not to schedule
> inferior 1 for a long time, but it would be really rare. If the bug is
> re-introduced, the test will catch it much more often than not, so it
> will be noticed.
My thinking when I saw that both inferiors wait 3 seconds before reaching
main (before reading the commit log), was, "???, I don't understand this."
So this is assuming that because the first inferior was started first, that
its 3 seconds always finish before the second inferior's 3 seconds? That
seems a bit risky. gdb and the other inferiors may all be running on
different cores, and on a fast machine, gdb may be fast enough to spawn
both processes roughtly at the same time, and then inferior 2 may end up
reporting the hit first.
Why not make it so that inferior 3 takes like 3 seconds to reach main,
but inferior 2 takes 4 or 5 seconds? Or 2 vs 4. Something like that.
I.e., make sure that inferior 2's time is larger than inferior 1's.
That could be done by tweaking the sleep calls in the programs, or
adding a sleep call in the .exp file between "run&" and starting the
second inferior.
But maybe I'm missing something?
> +proc do_test {} {
> + # With remote, to be able to start an inferior while another one is
> + # running, we need to use the non-stop variant of the protocol.
> + save_vars { ::GDBFLAGS } {
> + if { [target_info gdb_protocol] == "extended-remote"} {
> + append ::GDBFLAGS " -ex \"maintenance set target-non-stop on\""
> + }
> +
> + clean_restart ${::binfile_other}
> + }
> +
> + gdb_test "run&" "Starting program: .*" "start background inferior"
I was going to point out that if the inferior prints something, this can
timeout, as that output would appear after the prompt. I then looked around
the tree for "run&" uses, to confirm we are using gdb_test_multiple with that,
and found that you just recently added "gdb_test -no-prompt-anchor", for exactly
this scenario. :-) I think that should be used here.
> + gdb_test "add-inferior" "Added inferior 2.*"
> + gdb_test "inferior 2" "Switching to inferior 2.*"
> + gdb_file_cmd ${::binfile}
> + gdb_test "start" "Thread 2.1 .* hit Temporary breakpoint .*"
> +}
> +
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2] gdb: make "start" breakpoint inferior-specific
2022-11-10 16:45 ` Pedro Alves
@ 2022-11-10 17:33 ` Simon Marchi
2022-11-10 17:36 ` Simon Marchi
2022-11-10 17:47 ` Pedro Alves
0 siblings, 2 replies; 28+ messages in thread
From: Simon Marchi @ 2022-11-10 17:33 UTC (permalink / raw)
To: Pedro Alves, Simon Marchi, gdb-patches
On 11/10/22 11:45, Pedro Alves wrote:
> On 2022-11-08 9:20 p.m., Simon Marchi via Gdb-patches wrote:
>> New in v2:
>>
>> - Change the test so it doesn't call the main function
>>
>> I saw this failure on a CI:
>>
>> (gdb) add-inferior
>> [New inferior 2]
>> Added inferior 2
>> (gdb) PASS: gdb.threads/vfork-multi-inferior.exp: method=non-stop: add-inferior
>> inferior 2
>> [Switching to inferior 2 [<null>] (<noexec>)]
>> (gdb) PASS: gdb.threads/vfork-multi-inferior.exp: method=non-stop: inferior 2
>> kill
>> The program is not being run.
>> (gdb) file /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/tmp/tmp.GYATAXR8Ku/gdb/testsuite/outputs/gdb.threads/vfork-multi-inferior/vfork-multi-inferior-sleep
>> Reading symbols from /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/tmp/tmp.GYATAXR8Ku/gdb/testsuite/outputs/gdb.threads/vfork-multi-inferior/vfork-multi-inferior-sleep...
>> (gdb) run &
>> Starting program: /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/tmp/tmp.GYATAXR8Ku/gdb/testsuite/outputs/gdb.threads/vfork-multi-inferior/vfork-multi-inferior-sleep
>> (gdb) PASS: gdb.threads/vfork-multi-inferior.exp: method=non-stop: run inferior 2
>> inferior 1
>> [Switching to inferior 1 [<null>] (<noexec>)]
>> (gdb) PASS: gdb.threads/vfork-multi-inferior.exp: method=non-stop: inferior 1
>> kill
>> The program is not being run.
>> (gdb) file /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/tmp/tmp.GYATAXR8Ku/gdb/testsuite/outputs/gdb.threads/vfork-multi-inferior/vfork-multi-inferior
>> Reading symbols from /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/tmp/tmp.GYATAXR8Ku/gdb/testsuite/outputs/gdb.threads/vfork-multi-inferior/vfork-multi-inferior...
>> (gdb) break should_break_here
>> Breakpoint 1 at 0x11b1: file /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/src/binutils-gdb/gdb/testsuite/gdb.threads/vfork-multi-inferior.c, line 25.
>> (gdb) PASS: gdb.threads/vfork-multi-inferior.exp: method=non-stop: break should_break_here
>> [Thread debugging using libthread_db enabled]
>> Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
>> start
>> Temporary breakpoint 2 at 0x11c0: -qualified main. (2 locations)
>> Starting program: /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/tmp/tmp.GYATAXR8Ku/gdb/testsuite/outputs/gdb.threads/vfork-multi-inferior/vfork-multi-inferior
>> [Thread debugging using libthread_db enabled]
>> Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
>>
>> Thread 2.1 "vfork-multi-inf" hit Temporary breakpoint 2, main () at /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/unix/src/binutils-gdb/gdb/testsuite/gdb.threads/vfork-multi-inferior-sleep.c:23
>> 23 sleep (30);
>> (gdb) FAIL: gdb.threads/vfork-multi-inferior.exp: method=non-stop: start inferior 1
>>
>> What happens is:
>>
>> 1. We start inferior 2 with "run&", it runs very slowly, takes time to
>> get to main
>> 2. We switch to inferior 1, and run "start"
>> 3. The temporary breakpoint inserted by "start" applies to all inferiors
>> 4. Inferior 2 hits that breakpoint and GDB reports that hit
>>
>> To avoid this, breakpoints inserted by "start" should be
>> inferior-specific. However, we don't have a nice way to make
>> inferior-specific breakpoints yet. It's possible to make
>> pspace-specific breakpoints (for example how the internal_breakpoint
>> constructor does) by creating a symtab_and_line manually. However,
>> inferiors can share program spaces (usually on particular embedded
>> targets), so we could have a situation where two inferiors run the same
>> code in the same program space. In that case, it would just not be
>> possible to insert a breakpoint in one inferior but not the other.
>>
>> A simple solution that should work all the time is to add a condition to
>> the breakpoint inserted by "start", to check the inferior reporting the
>> hit is the expected one. This is what this patch implements.
>>
>
> Even though this does work, it still sets the breakpoint on all the pspaces
> unnecessarily. It would be nice if the breakpoint was pspace specific, in
> addition to inferior specific like you have (or some other way). But, what you
> have is fine with me as is, as it is better than what we have today.
> Maybe just add a little comment suggesting that it would be even better
> to make the breakpoint apply to the current pspace only?
Will do.
>> Add a test that does:
>>
>> - start in background inferior 1 that sleeps 3 seconds before reaching
>> its main function (using a sleep in a global C++ object constructor)
>> - start inferior 2, which also sleeps 3 seconds before reaching its m
>> ain function, with the "start" command
>> - validate that we hit the breakpoint in inferior 2
>>
>> Without the fix, we hit the breakpoint in inferior 1 pretty much all the
>> time. There could be some unfortunate scheduling causing the test not
>> to catch the bug, for instance if the scheduler decides not to schedule
>> inferior 1 for a long time, but it would be really rare. If the bug is
>> re-introduced, the test will catch it much more often than not, so it
>> will be noticed.
>
> My thinking when I saw that both inferiors wait 3 seconds before reaching
> main (before reading the commit log), was, "???, I don't understand this."
>
> So this is assuming that because the first inferior was started first, that
> its 3 seconds always finish before the second inferior's 3 seconds? That
> seems a bit risky. gdb and the other inferiors may all be running on
> different cores, and on a fast machine, gdb may be fast enough to spawn
> both processes roughtly at the same time, and then inferior 2 may end up
> reporting the hit first.
>
> Why not make it so that inferior 3 takes like 3 seconds to reach main,
> but inferior 2 takes 4 or 5 seconds? Or 2 vs 4. Something like that.
> I.e., make sure that inferior 2's time is larger than inferior 1's.
> That could be done by tweaking the sleep calls in the programs, or
> adding a sleep call in the .exp file between "run&" and starting the
> second inferior.
>
> But maybe I'm missing something?
Hmm no, you're right, there's no reason for the two sleep times to be
equal. I'll do 2 seconds for inferior 1 and 4 seconds for inferior 2.
>> +proc do_test {} {
>> + # With remote, to be able to start an inferior while another one is
>> + # running, we need to use the non-stop variant of the protocol.
>> + save_vars { ::GDBFLAGS } {
>> + if { [target_info gdb_protocol] == "extended-remote"} {
>> + append ::GDBFLAGS " -ex \"maintenance set target-non-stop on\""
>> + }
>> +
>> + clean_restart ${::binfile_other}
>> + }
>> +
>> + gdb_test "run&" "Starting program: .*" "start background inferior"
>
> I was going to point out that if the inferior prints something, this can
> timeout, as that output would appear after the prompt. I then looked around
> the tree for "run&" uses, to confirm we are using gdb_test_multiple with that,
> and found that you just recently added "gdb_test -no-prompt-anchor", for exactly
> this scenario. :-) I think that should be used here.
Even if, in this case, we know the inferior won't print anything?
Simon
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2] gdb: make "start" breakpoint inferior-specific
2022-11-10 17:33 ` Simon Marchi
@ 2022-11-10 17:36 ` Simon Marchi
2022-11-10 17:47 ` Pedro Alves
1 sibling, 0 replies; 28+ messages in thread
From: Simon Marchi @ 2022-11-10 17:36 UTC (permalink / raw)
To: Pedro Alves, Simon Marchi, gdb-patches
>> Even though this does work, it still sets the breakpoint on all the pspaces
>> unnecessarily. It would be nice if the breakpoint was pspace specific, in
>> addition to inferior specific like you have (or some other way). But, what you
>> have is fine with me as is, as it is better than what we have today.
>> Maybe just add a little comment suggesting that it would be even better
>> to make the breakpoint apply to the current pspace only?
>
> Will do.
FWIW, here's what I'm adding
/* To avoid other inferiors hitting this breakpoint, make it
inferior-specific using a condition. A better solution would be to
have proper inferior-specific breakpoint support, in the breakpoint
machinery. We could then avoid inserting a breakpoint in the program
spaces unrelated to this inferior. */
Simon
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2] gdb: make "start" breakpoint inferior-specific
2022-11-10 17:33 ` Simon Marchi
2022-11-10 17:36 ` Simon Marchi
@ 2022-11-10 17:47 ` Pedro Alves
2022-11-10 17:53 ` Simon Marchi
1 sibling, 1 reply; 28+ messages in thread
From: Pedro Alves @ 2022-11-10 17:47 UTC (permalink / raw)
To: Simon Marchi, Simon Marchi, gdb-patches
On 2022-11-10 5:33 p.m., Simon Marchi wrote:
>>> +proc do_test {} {
>>> + # With remote, to be able to start an inferior while another one is
>>> + # running, we need to use the non-stop variant of the protocol.
>>> + save_vars { ::GDBFLAGS } {
>>> + if { [target_info gdb_protocol] == "extended-remote"} {
>>> + append ::GDBFLAGS " -ex \"maintenance set target-non-stop on\""
>>> + }
>>> +
>>> + clean_restart ${::binfile_other}
>>> + }
>>> +
>>> + gdb_test "run&" "Starting program: .*" "start background inferior"
>>
>> I was going to point out that if the inferior prints something, this can
>> timeout, as that output would appear after the prompt. I then looked around
>> the tree for "run&" uses, to confirm we are using gdb_test_multiple with that,
>> and found that you just recently added "gdb_test -no-prompt-anchor", for exactly
>> this scenario. :-) I think that should be used here.
>
> Even if, in this case, we know the inferior won't print anything?
Admitedly it's a bit pedantic, but it seems to me to be safer. Say
someones adds some logging to the program or something. It just looks
like good practice to me to not have an anchor when the inferior is left
running after the prompt is printed.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2] gdb: make "start" breakpoint inferior-specific
2022-11-10 17:47 ` Pedro Alves
@ 2022-11-10 17:53 ` Simon Marchi
0 siblings, 0 replies; 28+ messages in thread
From: Simon Marchi @ 2022-11-10 17:53 UTC (permalink / raw)
To: Pedro Alves, Simon Marchi, gdb-patches
On 11/10/22 12:47, Pedro Alves wrote:
> On 2022-11-10 5:33 p.m., Simon Marchi wrote:
>
>>>> +proc do_test {} {
>>>> + # With remote, to be able to start an inferior while another one is
>>>> + # running, we need to use the non-stop variant of the protocol.
>>>> + save_vars { ::GDBFLAGS } {
>>>> + if { [target_info gdb_protocol] == "extended-remote"} {
>>>> + append ::GDBFLAGS " -ex \"maintenance set target-non-stop on\""
>>>> + }
>>>> +
>>>> + clean_restart ${::binfile_other}
>>>> + }
>>>> +
>>>> + gdb_test "run&" "Starting program: .*" "start background inferior"
>>>
>>> I was going to point out that if the inferior prints something, this can
>>> timeout, as that output would appear after the prompt. I then looked around
>>> the tree for "run&" uses, to confirm we are using gdb_test_multiple with that,
>>> and found that you just recently added "gdb_test -no-prompt-anchor", for exactly
>>> this scenario. :-) I think that should be used here.
>>
>> Even if, in this case, we know the inferior won't print anything?
>
> Admitedly it's a bit pedantic, but it seems to me to be safer. Say
> someones adds some logging to the program or something. It just looks
> like good practice to me to not have an anchor when the inferior is left
> running after the prompt is printed.
Ack, I will add it.
Simon
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2] gdb: make "start" breakpoint inferior-specific
2022-11-08 21:20 ` [PATCH v2] " Simon Marchi
2022-11-10 16:45 ` Pedro Alves
@ 2022-11-11 12:37 ` Tom de Vries
2022-11-11 13:53 ` Simon Marchi
1 sibling, 1 reply; 28+ messages in thread
From: Tom de Vries @ 2022-11-11 12:37 UTC (permalink / raw)
To: Simon Marchi, gdb-patches
On 11/8/22 22:20, Simon Marchi via Gdb-patches wrote:
> - std::string arg = string_printf ("-qualified %s", main_name ());
> + std::string arg = string_printf ("-qualified %s if $_inferior == %d", main_name (),
> + current_inferior ()->num);
Hi,
it seems ada doesn't like the syntax, we get:
...
(gdb) start ^M
Error in expression, near `1'.^M
(gdb) UNTESTED: gdb.ada/start.exp: start failed to land inside the right
procedure
...
Thanks,
- Tom
(gdb) bt
#0 0x00007ffff4c3dad2 in __cxxabiv1::__cxa_throw (obj=0x2d8cef0,
tinfo=0x1471210 <typeinfo for gdb_exception_error>,
dest=0xb70b06 <gdb_exception_error::~gdb_exception_error()>)
at ../../../../libstdc++-v3/libsupc++/eh_throw.cc:78
#1 0x000000000141eb12 in throw_it(return_reason, errors, const char *,
typedef __va_list_tag __va_list_tag *) (reason=RETURN_ERROR,
error=GENERIC_ERROR,
fmt=0x14789a8 "Error in expression, near `%s'.", ap=0x7fffffffb258)
at
/home/vries/gdb_versions/devel/src/gdbsupport/common-exceptions.cc:200
#2 0x000000000141eb8e in throw_verror (error=GENERIC_ERROR,
fmt=0x14789a8 "Error in expression, near `%s'.", ap=0x7fffffffb258)
at
/home/vries/gdb_versions/devel/src/gdbsupport/common-exceptions.cc:208
#3 0x0000000000cce1d2 in verror (string=0x14789a8 "Error in expression,
near `%s'.",
args=0x7fffffffb258) at
/home/vries/gdb_versions/devel/src/gdb/utils.c:164
#4 0x0000000001423811 in error (fmt=0x14789a8 "Error in expression,
near `%s'.")
at /home/vries/gdb_versions/devel/src/gdbsupport/errors.cc:46
#5 0x000000000043b90d in ada_yyerror (msg=0x147623a "syntax error")
at /home/vries/gdb_versions/devel/src/gdb/ada-exp.y:1169
#6 0x000000000043793f in ada_yyparse () at ada-exp.c.tmp:2905
#7 0x000000000043b818 in ada_parse (par_state=0x7fffffffcca0)
at /home/vries/gdb_versions/devel/src/gdb/ada-exp.y:1155
#8 0x000000000047b418 in ada_language::parser (this=0x29937d0
<ada_language_defn>,
ps=0x7fffffffcca0) at
/home/vries/gdb_versions/devel/src/gdb/ada-lang.c:13855
#9 0x00000000009bdcd2 in parse_exp_in_context
(stringptr=0x7fffffffce08, pc=4202360,
block=0x37c2280, comma=0, void_context_p=false,
tracker=0x7fffffffcd20, completer=0x0)
at /home/vries/gdb_versions/devel/src/gdb/parse.c:515
#10 0x00000000009bda5f in parse_exp_1 (stringptr=0x7fffffffce08,
pc=4202360,
block=0x37c2280, comma=0, tracker=0x0)
at /home/vries/gdb_versions/devel/src/gdb/parse.c:428
#11 0x0000000000567921 in find_condition_and_thread (tok=0x376f439
"$_inferior == 1",
pc=4202360, cond_string=0x7fffffffcec8, thread=0x7fffffffcec4,
task=0x7fffffffcec0,
rest=0x7fffffffceb8) at
/home/vries/gdb_versions/devel/src/gdb/breakpoint.c:8649
#12 0x0000000000567c78 in find_condition_and_thread_for_sals (
sals=std::vector of length 1, capacity 1 = {...}, input=0x376f436
"if $_inferior == 1",
cond_string=0x7fffffffcf78, thread=0x7fffffffcf34,
task=0x7fffffffcfbc,
rest=0x7fffffffcf80) at
/home/vries/gdb_versions/devel/src/gdb/breakpoint.c:8728
#13 0x0000000000568389 in create_breakpoint (gdbarch=0x2d8d1e0,
locspec=0x376f500,
cond_string=0x0, thread=0, extra_string=0x376f436 "if $_inferior ==
1",
force_condition=false, parse_extra=1, tempflag=1,
type_wanted=bp_breakpoint,
ignore_count=0, pending_break_support=AUTO_BOOLEAN_AUTO,
ops=0x14afec0 <code_breakpoint_ops>, from_tty=0, enabled=1,
internal=0, flags=0)
at /home/vries/gdb_versions/devel/src/gdb/breakpoint.c:8909
#14 0x0000000000568be2 in break_command_1 (arg=0x376f436 "if $_inferior
== 1", flag=1,
from_tty=0) at /home/vries/gdb_versions/devel/src/gdb/breakpoint.c:9027
#15 0x0000000000568eb6 in tbreak_command (
arg=0x376f420 "-qualified _ada_dummy if $_inferior == 1", from_tty=0)
at /home/vries/gdb_versions/devel/src/gdb/breakpoint.c:9104
#16 0x000000000084f040 in run_command_1 (args=0x0, from_tty=0,
run_how=RUN_STOP_AT_MAIN)
at /home/vries/gdb_versions/devel/src/gdb/infcmd.c:433
#17 0x000000000084f555 in start_command (args=0x0, from_tty=0)
at /home/vries/gdb_versions/devel/src/gdb/infcmd.c:537
#18 0x00000000005e9bd6 in do_simple_func (args=0x0, from_tty=0, c=0x2bcc0a0)
at /home/vries/gdb_versions/devel/src/gdb/cli/cli-decode.c:95
#19 0x00000000005ee986 in cmd_func (cmd=0x2bcc0a0, args=0x0, from_tty=0)
at /home/vries/gdb_versions/devel/src/gdb/cli/cli-decode.c:2543
#20 0x0000000000c3648f in execute_command (p=0x7fffffffe14f "", from_tty=0)
at /home/vries/gdb_versions/devel/src/gdb/top.c:692
#21 0x00000000008faa55 in catch_command_errors (
command=0xc35ec2 <execute_command(char const*, int)>,
arg=0x7fffffffe14a "start",
from_tty=0, do_bp_actions=true) at
/home/vries/gdb_versions/devel/src/gdb/main.c:513
#22 0x00000000008fac2d in execute_cmdargs (cmdarg_vec=0x7fffffffd7a0,
file_type=CMDARG_FILE,
cmd_type=CMDARG_COMMAND, ret=0x7fffffffd77c)
at /home/vries/gdb_versions/devel/src/gdb/main.c:608
#23 0x00000000008fbfbd in captured_main_1 (context=0x7fffffffd9e0)
at /home/vries/gdb_versions/devel/src/gdb/main.c:1299
#24 0x00000000008fc1c0 in captured_main (data=0x7fffffffd9e0)
at /home/vries/gdb_versions/devel/src/gdb/main.c:1320
#25 0x00000000008fc22b in gdb_main (args=0x7fffffffd9e0)
at /home/vries/gdb_versions/devel/src/gdb/main.c:1345
#26 0x000000000041a24e in main (argc=13, argv=0x7fffffffdaf8)
at /home/vries/gdb_versions/devel/src/gdb/gdb.c:32
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2] gdb: make "start" breakpoint inferior-specific
2022-11-11 12:37 ` Tom de Vries
@ 2022-11-11 13:53 ` Simon Marchi
2022-11-11 15:21 ` Tom de Vries
0 siblings, 1 reply; 28+ messages in thread
From: Simon Marchi @ 2022-11-11 13:53 UTC (permalink / raw)
To: Tom de Vries, gdb-patches
On 11/11/22 07:37, Tom de Vries wrote:
> On 11/8/22 22:20, Simon Marchi via Gdb-patches wrote:
>> - std::string arg = string_printf ("-qualified %s", main_name ());
>> + std::string arg = string_printf ("-qualified %s if $_inferior == %d", main_name (),
>> + current_inferior ()->num);
>
> Hi,
>
> it seems ada doesn't like the syntax, we get:
> ...
> (gdb) start ^M
> Error in expression, near `1'.^M
> (gdb) UNTESTED: gdb.ada/start.exp: start failed to land inside the right procedure
Huh, sorry, I missed it because it shows up as UNTESTED, which my CI job
doesn't flag as a failure.
Here's a patch that fixes it in a rather naive way. Ideally, we would
implement proper inferior-specific breakpoints, but in any case we want
un-break the tests sooner than that.
From 28f370e7dda4fb2f240ed29493416e78ed47f176 Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@polymtl.ca>
Date: Fri, 11 Nov 2022 07:58:35 -0500
Subject: [PATCH] gdb: fix start breakpoint expression not working in some
languages
Commit 0be837be9fb4 ("gdb: make "start" breakpoint inferior-specific")
regresses gdb.ada/start.exp:
(gdb) start
Error in expression, near `1'.
(gdb) UNTESTED: gdb.ada/start.exp: start failed to land inside the right procedure
This is because in Ada, the equality operator is =, not ==.
I checked the other languages supported by GDB, these other languages
use = for equality:
- Pascal: tests like gdb.pascal/hello.exp are affected too
- Modula-2: I tried building a Modula-2 hello world using gm2, but it
seems like the generated DWARF doesn't specify the Modula-2 language
in the CUs, it's C++ and C, so the selected language isn't
"modula-2". But if I manually do "set language modula-2" on a dummy
program and then "start", I get the same error.
Other languages all use ==.
So, a short term fix would be to use = or == in the expression, based on
the current language. If this was meant to be permanent, I would
suggest adding something like an "equality_operator" method to
language_defn, that returns the right equality operator for the
language. But the goal is to replace all this with proper
inferior-specific breakpoints, so I hope all this is temporary.
Change-Id: Id4d38e14a80e6bbbb1ad2b2277f974dd55192969
---
gdb/infcmd.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index bf4a68e3557e..6f83949cc7c0 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -428,8 +428,13 @@ run_command_1 (const char *args, int from_tty, enum run_how run_how)
have proper inferior-specific breakpoint support, in the breakpoint
machinery. We could then avoid inserting a breakpoint in the program
spaces unrelated to this inferior. */
- std::string arg = string_printf ("-qualified %s if $_inferior == %d", main_name (),
- current_inferior ()->num);
+ const char *op
+ = ((current_language->la_language == language_ada
+ || current_language->la_language == language_pascal
+ || current_language->la_language == language_m2) ? "=" : "==");
+ std::string arg = string_printf
+ ("-qualified %s if $_inferior %s %d", main_name (), op,
+ current_inferior ()->num);
tbreak_command (arg.c_str (), 0);
}
base-commit: 70b9d05b26e861524d70ee90dcd28cfd77032ddd
--
2.38.1
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2] gdb: make "start" breakpoint inferior-specific
2022-11-11 13:53 ` Simon Marchi
@ 2022-11-11 15:21 ` Tom de Vries
2022-11-11 19:03 ` Simon Marchi
0 siblings, 1 reply; 28+ messages in thread
From: Tom de Vries @ 2022-11-11 15:21 UTC (permalink / raw)
To: Simon Marchi, gdb-patches
On 11/11/22 14:53, Simon Marchi wrote:
> On 11/11/22 07:37, Tom de Vries wrote:
>> On 11/8/22 22:20, Simon Marchi via Gdb-patches wrote:
>>> - std::string arg = string_printf ("-qualified %s", main_name ());
>>> + std::string arg = string_printf ("-qualified %s if $_inferior == %d", main_name (),
>>> + current_inferior ()->num);
>>
>> Hi,
>>
>> it seems ada doesn't like the syntax, we get:
>> ...
>> (gdb) start ^M
>> Error in expression, near `1'.^M
>> (gdb) UNTESTED: gdb.ada/start.exp: start failed to land inside the right procedure
>
> Huh, sorry, I missed it because it shows up as UNTESTED, which my CI job
> doesn't flag as a failure.
>
I only noticed by glancing at gdb.log scrolling by, which got stuck
waiting for "Starting program:" to appear. Which I've just realized is
a testsuite error, so I've fixed this with "[gdb/testsuite] Don't
timeout on prompt in gdb_start_cmd".
> Here's a patch that fixes it in a rather naive way. Ideally, we would
> implement proper inferior-specific breakpoints, but in any case we want
> un-break the tests sooner than that.
>
It fixes the "UNTESTED" for me, and LGTM.
I did wonder if this could be fixed in a way that the expression is
parsed independent of the current language, setting language to say C
for the duration of the command. And that does seem to work:
...
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index bf4a68e3557..f7b1d763838 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -430,7 +430,14 @@ run_command_1 (const char *args, int from_tty, enum
run_
how run_how)
spaces unrelated to this inferior. */
std::string arg = string_printf ("-qualified %s if $_inferior ==
%d", main_nam
e (),
current_inferior ()->num);
- tbreak_command (arg.c_str (), 0);
+ {
+ scoped_restore_current_language save_language;
+ scoped_restore save_language_mode
+ = make_scoped_restore (&language_mode);
+ language_mode = language_mode_manual;
+ current_language = language_def (language_c);
+ tbreak_command (arg.c_str (), 0);
+ }
}
exec_file = get_exec_file (0);
...
I'm not sure if this is a better solution: it's more intrusive.
Thanks,
- Tom
> From 28f370e7dda4fb2f240ed29493416e78ed47f176 Mon Sep 17 00:00:00 2001
> From: Simon Marchi <simon.marchi@polymtl.ca>
> Date: Fri, 11 Nov 2022 07:58:35 -0500
> Subject: [PATCH] gdb: fix start breakpoint expression not working in some
> languages
>
> Commit 0be837be9fb4 ("gdb: make "start" breakpoint inferior-specific")
> regresses gdb.ada/start.exp:
>
> (gdb) start
> Error in expression, near `1'.
> (gdb) UNTESTED: gdb.ada/start.exp: start failed to land inside the right procedure
>
> This is because in Ada, the equality operator is =, not ==.
>
> I checked the other languages supported by GDB, these other languages
> use = for equality:
>
> - Pascal: tests like gdb.pascal/hello.exp are affected too
> - Modula-2: I tried building a Modula-2 hello world using gm2, but it
> seems like the generated DWARF doesn't specify the Modula-2 language
> in the CUs, it's C++ and C, so the selected language isn't
> "modula-2". But if I manually do "set language modula-2" on a dummy
> program and then "start", I get the same error.
>
> Other languages all use ==.
>
> So, a short term fix would be to use = or == in the expression, based on
> the current language. If this was meant to be permanent, I would
> suggest adding something like an "equality_operator" method to
> language_defn, that returns the right equality operator for the
> language. But the goal is to replace all this with proper
> inferior-specific breakpoints, so I hope all this is temporary.
>
> Change-Id: Id4d38e14a80e6bbbb1ad2b2277f974dd55192969
> ---
> gdb/infcmd.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
> index bf4a68e3557e..6f83949cc7c0 100644
> --- a/gdb/infcmd.c
> +++ b/gdb/infcmd.c
> @@ -428,8 +428,13 @@ run_command_1 (const char *args, int from_tty, enum run_how run_how)
> have proper inferior-specific breakpoint support, in the breakpoint
> machinery. We could then avoid inserting a breakpoint in the program
> spaces unrelated to this inferior. */
> - std::string arg = string_printf ("-qualified %s if $_inferior == %d", main_name (),
> - current_inferior ()->num);
> + const char *op
> + = ((current_language->la_language == language_ada
> + || current_language->la_language == language_pascal
> + || current_language->la_language == language_m2) ? "=" : "==");
> + std::string arg = string_printf
> + ("-qualified %s if $_inferior %s %d", main_name (), op,
> + current_inferior ()->num);
> tbreak_command (arg.c_str (), 0);
> }
>
>
> base-commit: 70b9d05b26e861524d70ee90dcd28cfd77032ddd
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2] gdb: make "start" breakpoint inferior-specific
2022-11-11 15:21 ` Tom de Vries
@ 2022-11-11 19:03 ` Simon Marchi
2022-11-12 10:43 ` Tom de Vries
2022-11-14 11:29 ` Tom de Vries
0 siblings, 2 replies; 28+ messages in thread
From: Simon Marchi @ 2022-11-11 19:03 UTC (permalink / raw)
To: Tom de Vries, Simon Marchi, gdb-patches
On 11/11/22 10:21, Tom de Vries via Gdb-patches wrote:
> On 11/11/22 14:53, Simon Marchi wrote:
>> On 11/11/22 07:37, Tom de Vries wrote:
>>> On 11/8/22 22:20, Simon Marchi via Gdb-patches wrote:
>>>> - std::string arg = string_printf ("-qualified %s", main_name ());
>>>> + std::string arg = string_printf ("-qualified %s if $_inferior == %d", main_name (),
>>>> + current_inferior ()->num);
>>>
>>> Hi,
>>>
>>> it seems ada doesn't like the syntax, we get:
>>> ...
>>> (gdb) start ^M
>>> Error in expression, near `1'.^M
>>> (gdb) UNTESTED: gdb.ada/start.exp: start failed to land inside the right procedure
>>
>> Huh, sorry, I missed it because it shows up as UNTESTED, which my CI job
>> doesn't flag as a failure.
>>
>
> I only noticed by glancing at gdb.log scrolling by, which got stuck waiting for "Starting program:" to appear. Which I've just realized is a testsuite error, so I've fixed this with "[gdb/testsuite] Don't timeout on prompt in gdb_start_cmd".
Thanks. I think it's strange for these tests to emit an UNTESTED if
gdb_start_cmd fails. Clearly, something is wrong if that happens. I'll
send a patch that changes them to fail.
>
>> Here's a patch that fixes it in a rather naive way. Ideally, we would
>> implement proper inferior-specific breakpoints, but in any case we want
>> un-break the tests sooner than that.
>>
>
> It fixes the "UNTESTED" for me, and LGTM.
>
> I did wonder if this could be fixed in a way that the expression is parsed independent of the current language, setting language to say C for the duration of the command. And that does seem to work:
> ...
> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
> index bf4a68e3557..f7b1d763838 100644
> --- a/gdb/infcmd.c
> +++ b/gdb/infcmd.c
> @@ -430,7 +430,14 @@ run_command_1 (const char *args, int from_tty, enum run_
> how run_how)
> spaces unrelated to this inferior. */
> std::string arg = string_printf ("-qualified %s if $_inferior == %d", main_nam
> e (),
> current_inferior ()->num);
> - tbreak_command (arg.c_str (), 0);
> + {
> + scoped_restore_current_language save_language;
> + scoped_restore save_language_mode
> + = make_scoped_restore (&language_mode);
> + language_mode = language_mode_manual;
> + current_language = language_def (language_c);
> + tbreak_command (arg.c_str (), 0);
> + }
> }
>
> exec_file = get_exec_file (0);
> ...
>
> I'm not sure if this is a better solution: it's more intrusive.
Ah, that would be good too. We wouldn't have to bake in the knowledge
of which languages use which operator. But I'm also a bit scared of
other unintended consequences when looking up the main symbol, as I see
the current_language is involved in some places. To be safe, I'll just
go with my naive patch. Thanks for the suggestion.
Simon
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2] gdb: make "start" breakpoint inferior-specific
2022-11-11 19:03 ` Simon Marchi
@ 2022-11-12 10:43 ` Tom de Vries
2022-11-14 11:29 ` Tom de Vries
1 sibling, 0 replies; 28+ messages in thread
From: Tom de Vries @ 2022-11-12 10:43 UTC (permalink / raw)
To: Simon Marchi, Simon Marchi, gdb-patches
On 11/11/22 20:03, Simon Marchi wrote:
> Thanks. I think it's strange for these tests to emit an UNTESTED if
> gdb_start_cmd fails. Clearly, something is wrong if that happens. I'll
> send a patch that changes them to fail.
>
Yes, that's a good point.
After thinking about this a bit, I've filed a PR for an overall revision
of the untested usage in gdb testsuite (
https://sourceware.org/bugzilla/show_bug.cgi?id=29778 ).
Thanks,
- Tom
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2] gdb: make "start" breakpoint inferior-specific
2022-11-11 19:03 ` Simon Marchi
2022-11-12 10:43 ` Tom de Vries
@ 2022-11-14 11:29 ` Tom de Vries
2022-11-14 13:19 ` Simon Marchi
1 sibling, 1 reply; 28+ messages in thread
From: Tom de Vries @ 2022-11-14 11:29 UTC (permalink / raw)
To: Simon Marchi, Simon Marchi, gdb-patches
On 11/11/22 20:03, Simon Marchi via Gdb-patches wrote:
>> I'm not sure if this is a better solution: it's more intrusive.
> Ah, that would be good too. We wouldn't have to bake in the knowledge
> of which languages use which operator. But I'm also a bit scared of
> other unintended consequences when looking up the main symbol, as I see
> the current_language is involved in some places. To be safe, I'll just
> go with my naive patch. Thanks for the suggestion.
FWIW, I've just stumbled onto language unknown, and realized that
strictly speaking we've got a regression because we used to have:
...
$ gdb -q -batch a.out -ex "set language unknown" -ex start
Temporary breakpoint 1 at 0x40050b: file /home/vries/hello.c, line 6.
Temporary breakpoint 1, main () at /home/vries/hello.c:6
6 printf ("hello\n");
Warning: the current language does not match this frame.
$
...
but now we have:
...
$ gdb -q -batch a.out -ex "set language unknown" -ex start
expression parsing not implemented for language "Unknown"
$
...
I don't really care about this, I thought I just mention it.
Thanks,
- Tom
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2] gdb: make "start" breakpoint inferior-specific
2022-11-14 11:29 ` Tom de Vries
@ 2022-11-14 13:19 ` Simon Marchi
2022-11-14 14:18 ` Tom de Vries
2022-11-16 16:22 ` Tom Tromey
0 siblings, 2 replies; 28+ messages in thread
From: Simon Marchi @ 2022-11-14 13:19 UTC (permalink / raw)
To: Tom de Vries, Simon Marchi, gdb-patches
On 11/14/22 06:29, Tom de Vries wrote:
> On 11/11/22 20:03, Simon Marchi via Gdb-patches wrote:
>>> I'm not sure if this is a better solution: it's more intrusive.
>> Ah, that would be good too. We wouldn't have to bake in the knowledge
>> of which languages use which operator. But I'm also a bit scared of
>> other unintended consequences when looking up the main symbol, as I see
>> the current_language is involved in some places. To be safe, I'll just
>> go with my naive patch. Thanks for the suggestion.
>
> FWIW, I've just stumbled onto language unknown, and realized that strictly speaking we've got a regression because we used to have:
> ...
> $ gdb -q -batch a.out -ex "set language unknown" -ex start
> Temporary breakpoint 1 at 0x40050b: file /home/vries/hello.c, line 6.
>
> Temporary breakpoint 1, main () at /home/vries/hello.c:6
> 6 printf ("hello\n");
> Warning: the current language does not match this frame.
> $
> ...
> but now we have:
> ...
> $ gdb -q -batch a.out -ex "set language unknown" -ex start
> expression parsing not implemented for language "Unknown"
> $
> ...
>
> I don't really care about this, I thought I just mention it.
Huh...
What is "set language unknown" useful for, generally speaking? If
debugging something in a language GDB doesn't know about, I think it
will fall back to minimal. Could we just not expose the "unknown"
language to the user?
Simon
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2] gdb: make "start" breakpoint inferior-specific
2022-11-14 13:19 ` Simon Marchi
@ 2022-11-14 14:18 ` Tom de Vries
2022-11-16 16:22 ` Tom Tromey
1 sibling, 0 replies; 28+ messages in thread
From: Tom de Vries @ 2022-11-14 14:18 UTC (permalink / raw)
To: Simon Marchi, Simon Marchi, gdb-patches; +Cc: Pedro Alves
On 11/14/22 14:19, Simon Marchi wrote:
>
>
> On 11/14/22 06:29, Tom de Vries wrote:
>> On 11/11/22 20:03, Simon Marchi via Gdb-patches wrote:
>>>> I'm not sure if this is a better solution: it's more intrusive.
>>> Ah, that would be good too. We wouldn't have to bake in the knowledge
>>> of which languages use which operator. But I'm also a bit scared of
>>> other unintended consequences when looking up the main symbol, as I see
>>> the current_language is involved in some places. To be safe, I'll just
>>> go with my naive patch. Thanks for the suggestion.
>>
>> FWIW, I've just stumbled onto language unknown, and realized that strictly speaking we've got a regression because we used to have:
>> ...
>> $ gdb -q -batch a.out -ex "set language unknown" -ex start
>> Temporary breakpoint 1 at 0x40050b: file /home/vries/hello.c, line 6.
>>
>> Temporary breakpoint 1, main () at /home/vries/hello.c:6
>> 6 printf ("hello\n");
>> Warning: the current language does not match this frame.
>> $
>> ...
>> but now we have:
>> ...
>> $ gdb -q -batch a.out -ex "set language unknown" -ex start
>> expression parsing not implemented for language "Unknown"
>> $
>> ...
>>
>> I don't really care about this, I thought I just mention it.
>
> Huh...
>
> What is "set language unknown" useful for, generally speaking? If
> debugging something in a language GDB doesn't know about, I think it
> will fall back to minimal. Could we just not expose the "unknown"
> language to the user?
Yeah, interestingly it also doesn't show up in the help:
...
$ gdb
(gdb) help set language
Set the current source language.
The currently understood settings are:
local or auto Automatic setting based on source file
c Use the C language
objective-c Use the Objective-C language
c++ Use the C++ language
d Use the D language
go Use the Go language
fortran Use the Fortran language
modula-2 Use the Modula-2 language
asm Use the Assembly language
pascal Use the Pascal language
opencl Use the OpenCL C language
rust Use the Rust language
minimal Use the Minimal language
ada Use the Ada language
...
Though that may have been an oversight, I'm not sure.
Looking through the testsuite, there are a few uses:
...
$ find gdb/testsuite/ -type f | xargs grep "set lang.*unknown"
gdb/testsuite/gdb.base/parse_number.exp:gdb_test_no_output "set language
unknown"
gdb/testsuite/gdb.base/langs.exp: gdb_test_no_output "set language
unknown"
gdb/testsuite/gdb.base/default.exp:gdb_test "set language" "Requires an
argument. Valid arguments are auto, local, unknown, ada, asm, c, c.., d,
fortran, go, minimal, modula-2, objective-c, opencl, pascal, rust."
gdb/testsuite/gdb.cp/demangle.exp: gdb_test_no_output "set language
unknown"
...
But I think removing it from the set language command, and replacing it
with "maint set language unknown" should be ok.
Thanks,
- Tom
Thanks,
- Tom
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2] gdb: make "start" breakpoint inferior-specific
2022-11-14 13:19 ` Simon Marchi
2022-11-14 14:18 ` Tom de Vries
@ 2022-11-16 16:22 ` Tom Tromey
2022-11-16 16:26 ` Simon Marchi
1 sibling, 1 reply; 28+ messages in thread
From: Tom Tromey @ 2022-11-16 16:22 UTC (permalink / raw)
To: Simon Marchi via Gdb-patches; +Cc: Tom de Vries, Simon Marchi, Simon Marchi
>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
Simon> What is "set language unknown" useful for, generally speaking? If
Simon> debugging something in a language GDB doesn't know about, I think it
Simon> will fall back to minimal. Could we just not expose the "unknown"
Simon> language to the user?
I wonder if we can remove it entirely and replace it with "minimal".
Tom
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2] gdb: make "start" breakpoint inferior-specific
2022-11-16 16:22 ` Tom Tromey
@ 2022-11-16 16:26 ` Simon Marchi
0 siblings, 0 replies; 28+ messages in thread
From: Simon Marchi @ 2022-11-16 16:26 UTC (permalink / raw)
To: Tom Tromey, Simon Marchi via Gdb-patches; +Cc: Tom de Vries, Simon Marchi
On 11/16/22 11:22, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
>
> Simon> What is "set language unknown" useful for, generally speaking? If
> Simon> debugging something in a language GDB doesn't know about, I think it
> Simon> will fall back to minimal. Could we just not expose the "unknown"
> Simon> language to the user?
>
> I wonder if we can remove it entirely and replace it with "minimal".
>
> Tom
I think so. I started a patch here [1] but haven't had time to follow
up on it.
In the uses of "set lang unknown" in the testsuite, two are to verify
that trying to print an expression while the unknown language is
selected gives an error message and does not crash, we don't care about
those.
The other one, in gdb.cp/demangle.exp, I think it's to make sure that
the current language is something else than the language we pass to
"demangle -l". I think using the minimal language there should be fine.
Simon
[1] https://review.lttng.org/c/binutils-gdb/+/8952
^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2022-11-16 16:26 UTC | newest]
Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-04 17:40 [PATCH] gdb: make "start" breakpoint inferior-specific Simon Marchi
2022-08-17 17:56 ` Simon Marchi
2022-08-31 14:03 ` Bruno Larsen
2022-11-04 16:52 ` Simon Marchi
2022-11-07 8:14 ` Bruno Larsen
2022-11-08 17:24 ` Tom Tromey
2022-09-01 10:42 ` Andrew Burgess
2022-11-04 17:24 ` Simon Marchi
[not found] ` <8735asb7cj.fsf@redhat.com>
2022-11-09 13:19 ` Simon Marchi
2022-11-08 19:43 ` Pedro Alves
2022-11-08 20:14 ` Simon Marchi
2022-11-08 21:09 ` Pedro Alves
2022-11-08 21:20 ` [PATCH v2] " Simon Marchi
2022-11-10 16:45 ` Pedro Alves
2022-11-10 17:33 ` Simon Marchi
2022-11-10 17:36 ` Simon Marchi
2022-11-10 17:47 ` Pedro Alves
2022-11-10 17:53 ` Simon Marchi
2022-11-11 12:37 ` Tom de Vries
2022-11-11 13:53 ` Simon Marchi
2022-11-11 15:21 ` Tom de Vries
2022-11-11 19:03 ` Simon Marchi
2022-11-12 10:43 ` Tom de Vries
2022-11-14 11:29 ` Tom de Vries
2022-11-14 13:19 ` Simon Marchi
2022-11-14 14:18 ` Tom de Vries
2022-11-16 16:22 ` Tom Tromey
2022-11-16 16:26 ` Simon Marchi
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).