PING: Re: [PATCHv2 2/2] gdb: Clean up inferior list when reconnecting to new target

[Andrew Burgess <andrew dot burgess at embecosm dot com>]

Ping!

Thanks,
Andrew

* Andrew Burgess <andrew.burgess@embecosm.com> [2018-08-08 13:30:59 +0100]:

> After pushing the first patch with a change of location for the test,
> this patch needed updating.  Nothing else has changed except the
> location of the modified test script.
> 
> Thanks,
> Andrew
> 
> ---
> 
> gdb: Clean up inferior list when reconnecting to new target
> 
> When connected to a target and running an inferior, if the inferior
> forks then a new, inferior is created to track the child
> process. Usually the second inferior (the child process) will be
> automatically cleaned up (removed) by GDB once the child has completed.
> However, if we connect to a new target then the second inferior is left
> around, and we can even end up using the second inferior in order to run
> a program on the new target, for example in this session:
> 
>    (gdb) target extended-remote :2347
>    (gdb) file /path/to/exe
>    (gdb) set remote exec-file /path/to/exe
>    (gdb) set detach-on-fork off
>    (gdb) break breakpt
>    (gdb) run
>    # ... hits breakpoint
>    (gdb) info inferiors
>      Num  Description       Executable
>    * 1    process 15406     /path/to/exe
>      2    process 15407     /path/to/exe
>    (gdb) kill
>    Kill the program being debugged? (y or n) y
>    (gdb) info inferiors
>      Num  Description       Executable
>    * 1    <null>            /path/to/exe
>      2    process 15433     /path/to/exe
>    (gdb) target extended-remote :2348
>    (gdb) file /path/to/exe
>    (gdb) set remote exec-file /path/to/exe
>    (gdb) run
>    # ... hits breakpoint
>    (gdb) info inferiors
>      Num  Description       Executable
>      1    <null>            /path/to/exe
>    * 2    process 15408     /path/to/exe
>      3    process 15409     /path/to/exe
>    (gdb)
> 
> Notice how after connecting to the second extended-remote target, and
> then running the test program we now have inferiors 1, 2, and 3.
> There's nothing really _wrong_ with this, but a better experience would,
> I think, be to have only inferiors 1 and 2 in the new target's session.
> 
> The issue here is that in target.c:dispose_inferior GDB uses
> switch_to_thread, which also switches the current inferior.  This leaves
> the last inferior in the list selected after target.c:target_preopen has
> completed.
> 
> We could change target.c:dispose_inferior to ensure the selected
> inferior is not left changed, however, in the above example, I think
> that, even if the user manually selected inferior 2 _before_ connecting
> to the new target, the best behaviour would still be for GDB to switch
> back to inferior 1.  This seems to suggest that the right thing to do is
> have GDB _choose_ a new inferior as part of target_preopen, after which
> is can call prune_inferiors to remove transient inferiors from the list.
> 
> An existing test is extended to cover this behaviour.
> 
> gdb/ChangeLog:
> 
> 	* target.c (find_first_killed_inferior): New function.
> 	(target_preopen): Use find_first_killed_inferior to reset selected
> 	inferior, and prune killed inferior as appropriate.
> 
> gdb/testsuite/ChangeLog:
> 
> 	* gdb.server/extended-remote-restart.exp: Extend.
> ---
>  gdb/ChangeLog                                      |  6 ++++
>  gdb/target.c                                       | 35 ++++++++++++++++++++++
>  gdb/testsuite/ChangeLog                            |  4 +++
>  .../gdb.server/extended-remote-restart.exp         |  6 ++++
>  4 files changed, 51 insertions(+)
> 
> diff --git a/gdb/target.c b/gdb/target.c
> index 115e9ae4948..7dfb5693f11 100644
> --- a/gdb/target.c
> +++ b/gdb/target.c
> @@ -2043,6 +2043,31 @@ dispose_inferior (struct inferior *inf, void *args)
>    return 0;
>  }
>  
> +/* Callback for iterate_over_inferiors.  Record in ARG the first killed
> +   inferior.  If we find an inferior that is both killed and not removable,
> +   this is returned in preference.  The definition of "first" here is
> +   pretty loose, and depends on the order in the inferior list.  */
> +
> +static int
> +find_first_killed_inferior (struct inferior *inf, void *arg)
> +{
> +  struct inferior **infp = (struct inferior **) arg;
> +
> +  if (inf->pid == 0)
> +    {
> +      if (!inf->removable)
> +	{
> +	  *infp = inf;
> +	  return 1;
> +	}
> +      else if (*infp == nullptr)
> +	*infp = inf;
> +    }
> +
> +  return 0;
> +}
> +
> +
>  /* This is to be called by the open routine before it does
>     anything.  */
>  
> @@ -2059,6 +2084,16 @@ target_preopen (int from_tty)
>  	iterate_over_inferiors (dispose_inferior, NULL);
>        else
>  	error (_("Program not killed."));
> +
> +      /* The call to DISPOSE_INFERIOR will leave the last inferior we
> +	 killed selected.  Reset the selection to the earliest inferior
> +	 that is killed and not removable.  The prune any other killed
> +	 inferiors.  */
> +      struct inferior *inf = nullptr;
> +      iterate_over_inferiors (find_first_killed_inferior, &inf);
> +      if (inf != nullptr)
> +	set_current_inferior (inf);
> +      prune_inferiors ();
>      }
>  
>    /* Calling target_kill may remove the target from the stack.  But if
> diff --git a/gdb/testsuite/gdb.server/extended-remote-restart.exp b/gdb/testsuite/gdb.server/extended-remote-restart.exp
> index 39fcb9e2e58..fb4bdd1755c 100644
> --- a/gdb/testsuite/gdb.server/extended-remote-restart.exp
> +++ b/gdb/testsuite/gdb.server/extended-remote-restart.exp
> @@ -119,6 +119,12 @@ proc test_reload { do_kill_p follow_child_p } {
>      } else {
>  	fail "reconnect after fork"
>      }
> +
> +    gdb_test "info inferiors" \
> +	[multi_line \
> +	     "  Num  Description       Executable.*" \
> +	     "\\* 1 +${dead_inf_ptn} \[^\r\n\]+" ] \
> +	"Check inferiors after reconnect"
>  }
>  
>  # Run all combinations of the test.
> -- 
> 2.14.4
>