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

login
register
mail settings
Submitter Tankut Baris Aktemur
Date Jan. 8, 2020, 3:48 p.m.
Message ID <BYAPR11MB3030EEB170A91F4A26C48051C43E0@BYAPR11MB3030.namprd11.prod.outlook.com>
Download mbox | patch
Permalink /patch/37258/
State New
Headers show

Comments

Tankut Baris Aktemur - Jan. 8, 2020, 3:48 p.m.
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?


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

Patch

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