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" <tankut.baris.aktemur@intel.com>]

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