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

[Pedro Alves <palves@redhat.com>]

On 08/01/2018 10:19 PM, Andrew Burgess wrote:
> 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

Does this PID here really correspond to the same debug session?
I'm confused, since 15433 wasn't in the list above.  I assume it
should be 15407?

>    (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)

And here, same confusion.  What really happened?
I think this is showing that process 15433 (or was it
15407) was killed/disposed, and a new process (15408) that
forked was spawned, and then that process forked 15409.  Right?

> 
> 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.

I'm a bit confused with the examples above, but off hand, I'm having trouble
liking the idea of GDB selecting an inferior on its own.  It seems to me
like this conflicts with multi-target?

For example, consider we have a setup like this one:

    (gdb) info inferiors
      Num  Description       Connection                    Executable
      1    <null>            1 (extended-remote :11111)    /path/to/exe
      2    <null>                                          /path/to/exe
      3    <null>            2 (native)                    /path/to/exe
      4    <null>            1 (extended-remote :11111)    /path/to/exe
      5    <null>            3 (extended-remote :33333)    /path/to/foo
    * 6    process 15433     3 (extended-remote :33333)    /path/to/bar
      7    process 15433     3 (extended-remote :33333)    /path/to/qux

If we connect to a new target while inferior 6 is selected,
it would seem quite natural to me to stay with inferior 6 selected.
Selecting inferiors 1 to 4 would seem unnatural to me.

Inferiors 1, 3 and 4 are connected to a different target.  Inferior 2
is not connected to a target yet, but then again, it has a different
executable loaded.  Similarly, inferior 5 is connected to the same
target and not running, but, it has an executable loaded which is different
from inferior 5's executable.  It's really not clear at all to me why
should GDB pick a different inferior.  

It seems to me that staying with the current inferior is the least
surprising and best option.

> 
> 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.base/extended-remote-restart.exp: Extend.
> ---
>  gdb/ChangeLog                                      |  6 ++++
>  gdb/target.c                                       | 35 ++++++++++++++++++++++
>  gdb/testsuite/ChangeLog                            |  4 +++
>  gdb/testsuite/gdb.base/extended-remote-restart.exp |  6 ++++
>  4 files changed, 51 insertions(+)
> 
> diff --git a/gdb/target.c b/gdb/target.c
> index f352e0d8283..d6b0bf42271 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.base/extended-remote-restart.exp b/gdb/testsuite/gdb.base/extended-remote-restart.exp
> index 39fcb9e2e58..fb4bdd1755c 100644
> --- a/gdb/testsuite/gdb.base/extended-remote-restart.exp
> +++ b/gdb/testsuite/gdb.base/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.
> 

Thanks,
Pedro Alves