RE: [PATCH] Switch the inferior too in switch_to_program_space_and_thread (Re: [PATCH v2 08/24] Introduce switch_to_inferior_no_thread)

Aktemur, Tankut Baris
2020-01-08T15:48:55Z

On Monday, December 23, 2019 8:30 PM, Pedro Alves wrote:
>
> On 12/20/19 6:50 PM, Pedro Alves wrote:
>
> > Oh wow, thanks much for this. You're right. I'm working on
> > converting your example above to a testsuite testcase.
>
> Here it is. I'm putting this at the end of the series, after the
> multi-target patches, because before multi-target, the test
> fails -- GDB sends a spurious Hg0.0 packet to the remote side.
>
> From 839bb32983d8afb085eeec1ac6ca499f5bbd6244 Mon Sep 17 00:00:00 2001
> From: Aleksandar Paunovic <aleksandar.paunovic@intel.com>
> Date: Mon, 23 Dec 2019 18:04:32 +0000
> Subject: [PATCH] Switch the inferior too in switch_to_program_space_and_thread
>
> With multi-target, each inferior now has its own target connection.
> The problem in switch_to_program_space_and_thread is that in the
> current state GDB switches to "no thread" and also sets the program
> space but because the inferior is not switched, potentially an
> incorrect target remains selected.
>
> Here is a sample scenario that exploits this flow:
>
> On terminal 1, start a gdbserver on a program named foo:
>
> $ gdbserver :1234 ./foo
>
> On terminal 2, start gdb on a program named bar. Suppose foo and bar
> are compiled from foo.c and bar.c. They are completely separate. So,
> bar.c:2 has no meaning for foo.
>
> $ gdb -q ./bar
> Reading symbols from ./bar...
> (gdb) add-inferior
> [New inferior 2]
> Added inferior 2
> (gdb) inferior 2
> [Switching to inferior 2 [<null>] (<noexec>)]
> (gdb) target remote :1234
> ...
> (gdb) set debug remote 2
> (gdb) break bar.c:2
> Sending packet: $Hgp0.0#ad...Packet received: OK
> Sending packet: $m5fa,12#f8...Packet received: E01
> Sending packet: $m5fa,1#c6...Packet received: E01
> Sending packet: $m5fb,3#c9...Packet received: E01
> Sending packet: $m5fe,1#ca...Packet received: E01
> Breakpoint 1 at 0x5fe: file bar.c, line 2.
> (gdb)
>
> Here we have an unnecessary sending of the packets to the gdbserver.
>
> With this fix in progspace-and-thread.c, we'll get this:
>
> (gdb) break bar.c:2
> Breakpoint 1 at 0x5fe: file bar.c, line 2.
> (gdb)
>
> Now there is no sending of the packets to gdbserver.
>
> gdb/ChangeLog:
> yyyy-mm-dd Aleksandar Paunovic <aleksandar.paunovic@intel.com>
> Pedro Alves <palves@redhat.com>
>
> * progspace-and-thread.c (switch_to_program_space_and_thread):
> Assert there's an inferior for PSPACE. Use
> switch_to_inferior_no_thread to switch the inferior too.
>
> gdb/testsuite/ChangeLog:
> yyyy-mm-dd Pedro Alves <palves@redhat.com>
>
> * gdb.server/bkpt-other-inferior.exp: New file.
> ---
> gdb/progspace-and-thread.c | 6 +-
> gdb/testsuite/gdb.server/bkpt-other-inferior.exp | 93 ++++++++++++++++++++++++
> 2 files changed, 96 insertions(+), 3 deletions(-)
> create mode 100644 gdb/testsuite/gdb.server/bkpt-other-inferior.exp
>
> diff --git a/gdb/progspace-and-thread.c b/gdb/progspace-and-thread.c
> index 3c92b5c8e0..d51748035e 100644
> --- a/gdb/progspace-and-thread.c
> +++ b/gdb/progspace-and-thread.c
> @@ -25,8 +25,9 @@ void
> switch_to_program_space_and_thread (program_space *pspace)
> {
> inferior *inf = find_inferior_for_program_space (pspace);
> + gdb_assert (inf != nullptr);

This creates failure in gdb.base/step-over-exit.exp. The problem is, a forked
child terminates, and GDB tries to switch to the pspace of that no-longer-existing
inferior through a remaining breakpoint location. Would it make sense to guard
calls to `switch_to_program_space_and_thread` as below, for example?

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 080e847de1..30af603c73 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -2879,6 +2879,9 @@ update_inserted_breakpoint_locations (void)
if (!bl->inserted || !bl->needs_update)
continue;

+ if (find_inferior_for_program_space (bl->pspace) == nullptr)
+ continue;
+
switch_to_program_space_and_thread (bl->pspace);

/* For targets that support global breakpoints, there's no need

There are a handful other cases that can be guarded/skipped similarly.
I'm not sure, though, if the problem is deeper and the calls to
`switch_to_program_space_and_thread` for a non-existing inferior should have
never occurred. That is, should the breakpoint locations of exited inferiors
be cleaned up much earlier, before we come to this point?

-Baris

> - if (inf != NULL && inf->pid != 0)
> + if (inf->pid != 0)
> {
> thread_info *tp = any_live_thread_of_inferior (inf);
>
> @@ -39,6 +40,5 @@ switch_to_program_space_and_thread (program_space *pspace)
> }
> }
>
> - switch_to_no_thread ();
> - set_current_program_space (pspace);
> + switch_to_inferior_no_thread (inf);
> }
Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Gary Kershaw
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928