diff mbox

[v2,2/3] Target remote mode fork and exec events

Message ID 1449526447-10039-3-git-send-email-donb@codesourcery.com
State New
Headers show

Commit Message

Don Breazeal Dec. 7, 2015, 10:14 p.m. UTC
Hi Pedro,

This is version two of a patch to implement support for fork and
exec events in target remote mode.  The differences from the previous
version are all based on the comments from the previous review, as
follows:

On 11/20/2015 5:04 AM, Pedro Alves wrote:
> Hi Don,
> 
> Thanks for doing this.  Starting to look at the series.
> 
> On 11/06/2015 11:56 PM, Don Breazeal wrote:
>> This patch implements support for fork and exec events with target remote
>> mode Linux targets.  For such targets with Linux kernels 2.5.46 and later,
>> this enables follow-fork-mode, detach-on-fork and fork and exec
>> catchpoints.  
> 
>> Note that follow-exec-mode is not supported, because target
>> remote mode does not support the 'run' command.
> 
> Not sure I don't understand this part/comment.

As I noted in my previous reply, this was an overly-pessimistic way of
putting it.  I revised the docs etc to say something like this:
"follow-exec-mode is of limited used in target remote mode, since the 'run'
command is not supported."  The point being that the main reason a user
would want to set follow-exec-mode would be to get control over which
inferior is run when a 'run' command is executed.  It is supported in
that the inferiors are created or not created as specified, and one could
(for example) use follow-exec-mode to keep a record of the programs that
were executed.

>>
>> The changes required to implement this included:
>>
>>  * Don't exit from gdbserver if there are still active inferiors.
>>
>>  * Allow changing the active process in remote mode.
>>
>>  * Enable fork and exec events in remote mode.
>>
>>  * Print "Ending remote debugging" when detaching only if in remote
>>    mode and there is only one inferior left.
>>
>>    (As I write this up I'm concluding that this is incorrect.  We
>>    don't care how many active inferiors there are, yes?
> 
> Not sure I understand the question.  But I'd say what matters is
> whether we're disconnecting/closing the connection.

Agreed.

>> Perhaps we
>>    need a test that detaches when there are multiple inferiors. I've
>>    added that to my list.)
>>
>>  * Combine remote_kill and extended_remote_kill into a single function
>>    that can handle the multiple inferior case for target remote.  Also,
>>    the same thing for remote_mourn and extended_remote_mourn.
>>
>>  * Enable process-style ptids in target remote.
>>
>>  * Remove restriction on multiprocess mode in target remote.
>>
>> Thanks
>> --Don
>>
>> gdb/gdbserver/
>> 2015-11-06  Don Breazeal  <donb@sourceware.org>
>>
>> 	* server.c (process_serial_event): Don't exit from gdbserver
>> 	in remote mode if there are still active inferiors.
>>
>> gdb/
>> 2015-11-06  Don Breazeal  <donb@sourceware.org>
>>
>> 	* remote.c (set_general_process): Remove restriction on target
>> 	remote mode.
>> 	(remote_query_supported): Likewise.
>> 	(remote_detach_1): Change restriction to target remote mode to
>> 	restriction to last inferior left.
>> 	(remote_disconnect): Unpush the target directly instead of 
>> 	calling remote_mourn.
>> 	(remote_kill, extended_remote_kill): Combine functions into one,
>> 	remote_kill, and enable extended functionality for target remote.
>> 	(remote_mourn, extended_remote_mourn): Combine functions into
>> 	one, remote_mourn, and enable extended functionality for target
>> 	remote.
>> 	(remote_pid_to_str): Enable "process" style ptid string for
>> 	target remote.
>> 	(remote_supports_multi_process): Remove restriction on target
>> 	remote mode.
>>
>> ---
>>  gdb/gdbserver/server.c |   6 +-
>>  gdb/remote.c           | 166 ++++++++++++++++++++++++-------------------------
>>  2 files changed, 84 insertions(+), 88 deletions(-)
>>
>> diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
>> index fd2804b..0f57237 100644
>> --- a/gdb/gdbserver/server.c
>> +++ b/gdb/gdbserver/server.c
>> @@ -3865,9 +3865,11 @@ process_serial_event (void)
>>  	  discard_queued_stop_replies (pid_to_ptid (pid));
>>  	  write_ok (own_buf);
>>  
>> -	  if (extended_protocol)
>> +	  if (extended_protocol || get_first_inferior (&all_threads) != NULL)
>>  	    {
>> -	      /* Treat this like a normal program exit.  */
>> +	      /* There is still at least one inferior remaining, so
>> +		 don't terminate gdbserver and treat this like a
>> +		 normal program exit.  */
>>  	      last_status.kind = TARGET_WAITKIND_EXITED;
>>  	      last_status.value.integer = 0;
>>  	      last_ptid = pid_to_ptid (pid);
>> diff --git a/gdb/remote.c b/gdb/remote.c
>> index fed397a..60da26c 100644
>> --- a/gdb/remote.c
>> +++ b/gdb/remote.c
>> @@ -123,8 +123,6 @@ static void remote_mourn (struct target_ops *ops);
>>  
>>  static void extended_remote_restart (void);
>>  
>> -static void extended_remote_mourn (struct target_ops *);
>> -
>>  static void remote_send (char **buf, long *sizeof_buf_p);
>>  
>>  static int readchar (int timeout);
>> @@ -2084,7 +2082,7 @@ set_general_process (void)
>>    struct remote_state *rs = get_remote_state ();
>>  
>>    /* If the remote can't handle multiple processes, don't bother.  */
>> -  if (!rs->extended || !remote_multi_process_p (rs))
>> +  if (!remote_multi_process_p (rs))
>>      return;
>>  
>>    /* We only need to change the remote current thread if it's pointing
>> @@ -4472,18 +4470,15 @@ remote_query_supported (void)
>>  
>>        q = remote_query_supported_append (q, "qRelocInsn+");
>>  
>> -      if (rs->extended)
>> -	{
>> -	  if (packet_set_cmd_state (PACKET_fork_event_feature)
>> -	      != AUTO_BOOLEAN_FALSE)
>> -	    q = remote_query_supported_append (q, "fork-events+");
>> -	  if (packet_set_cmd_state (PACKET_vfork_event_feature)
>> -	      != AUTO_BOOLEAN_FALSE)
>> -	    q = remote_query_supported_append (q, "vfork-events+");
>> -	  if (packet_set_cmd_state (PACKET_exec_event_feature)
>> -	      != AUTO_BOOLEAN_FALSE)
>> -	    q = remote_query_supported_append (q, "exec-events+");
>> -	}
>> +      if (packet_set_cmd_state (PACKET_fork_event_feature)
>> +	  != AUTO_BOOLEAN_FALSE)
>> +	q = remote_query_supported_append (q, "fork-events+");
>> +      if (packet_set_cmd_state (PACKET_vfork_event_feature)
>> +	  != AUTO_BOOLEAN_FALSE)
>> +	q = remote_query_supported_append (q, "vfork-events+");
>> +      if (packet_set_cmd_state (PACKET_exec_event_feature)
>> +	  != AUTO_BOOLEAN_FALSE)
>> +	q = remote_query_supported_append (q, "exec-events+");
>>  
>>        if (packet_set_cmd_state (PACKET_vContSupported) != AUTO_BOOLEAN_FALSE)
>>  	q = remote_query_supported_append (q, "vContSupported+");
>> @@ -4827,7 +4822,8 @@ remote_detach_1 (const char *args, int from_tty)
>>    /* Tell the remote target to detach.  */
>>    remote_detach_pid (pid);
>>  
>> -  if (from_tty && !rs->extended)
>> +  /* Exit only if this is the only active inferior.  */
>> +  if (from_tty && !rs->extended && number_of_inferiors () == 1)
>>      puts_filtered (_("Ending remote debugging.\n"));
>>  
>>    /* Check to see if we are detaching a fork parent.  Note that if we
>> @@ -4923,10 +4919,11 @@ remote_disconnect (struct target_ops *target, const char *args, int from_tty)
>>    if (args)
>>      error (_("Argument given to \"disconnect\" when remotely debugging."));
>>  
>> -  /* Make sure we unpush even the extended remote targets; mourn
>> -     won't do it.  So call remote_mourn directly instead of
>> -     target_mourn_inferior.  */
>> -  remote_mourn (target);
>> +  /* Make sure we unpush even the extended remote targets.  Calling
>> +     target_mourn_inferior won't unpush, and remote_mourn won't
>> +     unpush if there is more than one inferior left.  */
>> +  unpush_target (target);
>> +  generic_mourn_inferior ();
> 
> Note: this generic_mourn_inferior call here looks wrong, because we may be
> debugging more than one inferior.  But remote_mourn was already wrong
> for the same reason, so, not a problem added by this patch.

OK, I'll leave it in place for now.

>>  
>>    if (from_tty)
>>      puts_filtered ("Ending remote debugging.\n");
>> @@ -8562,42 +8559,6 @@ kill_new_fork_children (int pid, struct remote_state *rs)
>>  }
>>  
>>  
>> -static void
>> -remote_kill (struct target_ops *ops)
>> -{
> 
> Please rename this (making it a helper function) instead of inlining
> it in the new remote_kill.  E.g., remote_kill_k.

Done.

>> -
>> -  /* Catch errors so the user can quit from gdb even when we
>> -     aren't on speaking terms with the remote system.  */
>> -  TRY
>> -    {
>> -      putpkt ("k");
>> -    }
>> -  CATCH (ex, RETURN_MASK_ERROR)
>> -    {
>> -      if (ex.error == TARGET_CLOSE_ERROR)
>> -	{
>> -	  /* If we got an (EOF) error that caused the target
>> -	     to go away, then we're done, that's what we wanted.
>> -	     "k" is susceptible to cause a premature EOF, given
>> -	     that the remote server isn't actually required to
>> -	     reply to "k", and it can happen that it doesn't
>> -	     even get to reply ACK to the "k".  */
>> -	  return;
>> -	}
>> -
>> -	/* Otherwise, something went wrong.  We didn't actually kill
>> -	   the target.  Just propagate the exception, and let the
>> -	   user or higher layers decide what to do.  */
>> -	throw_exception (ex);
>> -    }
>> -  END_CATCH
>> -
>> -  /* We've killed the remote end, we get to mourn it.  Since this is
>> -     target remote, single-process, mourning the inferior also
>> -     unpushes remote_ops.  */
>> -  target_mourn_inferior ();
> 
> Maybe do still move this mourn out into new remote_kill.

Done.

>> -}
>> -
>>  static int
>>  remote_vkill (int pid, struct remote_state *rs)
>>  {
>> @@ -8624,19 +8585,58 @@ remote_vkill (int pid, struct remote_state *rs)
>>  }
>>  
>>  static void
>> -extended_remote_kill (struct target_ops *ops)
>> +remote_kill (struct target_ops *ops)
>>  {
>>    int res;
>>    int pid = ptid_get_pid (inferior_ptid);
>>    struct remote_state *rs = get_remote_state ();
>>  
>> +  /* If we are in 'target remote' mode and we are killing the only
>> +     inferior, then we will tell gdbserver to exit and unpush the
>> +     target.  */
>> +  if (!rs->extended && number_of_inferiors () <= 1)
> 
> It's number of inferiors, or number of _live_ inferiors that matters?
> I'd think the latter.  Likewise all other new number_of_inferiors
> calls should be audited.

Agreed, it is number of _live_ inferiors.  I've written a new function
for that called number_of_live_inferiors, since it was a bit problematic
finding an existing mechanism that could determine whether an inferior
that had a non-zero pid but no threads was actually live.  That case
shows up in remote_mourn, for example.

>> +    {
>> +      /* Catch errors so the user can quit from gdb even when we
>> +	 aren't on speaking terms with the remote system.  */
>> +      TRY
>> +	{
>> +	  putpkt ("k");
>> +	}
>> +      CATCH (ex, RETURN_MASK_ERROR)
>> +	{
>> +	  if (ex.error == TARGET_CLOSE_ERROR)
>> +	    {
>> +	      /* If we got an (EOF) error that caused the target
>> +		 to go away, then we're done, that's what we wanted.
>> +		 "k" is susceptible to cause a premature EOF, given
>> +		 that the remote server isn't actually required to
>> +		 reply to "k", and it can happen that it doesn't
>> +		 even get to reply ACK to the "k".  */
>> +	      return;
>> +	    }
>> +
>> +	  /* Otherwise, something went wrong.  We didn't actually kill
>> +	     the target.  Just propagate the exception, and let the
>> +	     user or higher layers decide what to do.  */
>> +	  throw_exception (ex);
>> +	}
>> +      END_CATCH
>> +
>> +      /* We've killed the remote end, we get to mourn it.  Since this is
>> +	 target remote, single-process, mourning the inferior also
>> +	 unpushes remote_ops.  */
> 
> Mentioning "single-process," here no longer looks right to me.

I've updated this comment.

>> +      target_mourn_inferior ();
>> +
>> +      return;
>> +    }
>> +
>>    /* If we're stopped while forking and we haven't followed yet, kill the
>>       child task.  We need to do this before killing the parent task
>>       because if this is a vfork then the parent will be sleeping.  */
>>    kill_new_fork_children (pid, rs);
>>  
>>    res = remote_vkill (pid, rs);
>> -  if (res == -1 && !(rs->extended && remote_multi_process_p (rs)))
>> +  if (res == -1 && !(remote_multi_process_p (rs)))
> 
> Drop the now-unnecessary parens.

Done.

> 
>>      {
>>        /* Don't try 'k' on a multi-process aware stub -- it has no way
>>  	 to specify the pid.  */
> 
> 
> I wonder about re-writing this function in top-down approach?
> 
>  #0 - if vkill is supported:
> 
>      - kill the new fork children first.
> 
>      - then kill the current process with remote_vkill.
> 
>  #1 - then if not multi-process, and there are still
>       live processes, call the new remote_kill_k helper, to kill
>       with "k".
> 
>  #2 - then, if !extended, and there are no live processes
>       left, disconnect.
> 
> 
> Note this also gets rid of the putpkt("k") call, which currently misses
> handling the EOF issue already handled by the (new) remote_kill_k.

I did the rewrite of the function as you suggested here.  Note that for
step #2 above, the check for "!extended and no live processes" happens
inside remote_mourn.

> 
>> @@ -8662,16 +8662,17 @@ extended_remote_kill (struct target_ops *ops)
>>  static void
>>  remote_mourn (struct target_ops *target)
>>  {
>> -  unpush_target (target);
>> +  struct remote_state *rs = get_remote_state ();
>>  
>> -  /* remote_close takes care of doing most of the clean up.  */
>> -  generic_mourn_inferior ();
>> -}
>> +  /* In 'target remote' mode with one inferior, we shut down gdbserver.  */
> 
> I'd rather say:
> 
>      /* In 'target remote' mode with one inferior, we close the connection.  */

Done.

> 
>> +  if (!rs->extended && number_of_inferiors () <= 1)
>> +    {
>> +      unpush_target (target);
>>  
>> -static void
>> -extended_remote_mourn (struct target_ops *target)
>> -{
>> -  struct remote_state *rs = get_remote_state ();
>> +      /* remote_close takes care of doing most of the clean up.  */
>> +      generic_mourn_inferior ();
>> +      return;
>> +    }
>>  
>>    /* In case we got here due to an error, but we're going to stay
>>       connected.  */
>> @@ -8702,8 +8703,9 @@ extended_remote_mourn (struct target_ops *target)
>>       current thread.  */
>>    record_currthread (rs, minus_one_ptid);
>>  
>> -  /* Unlike "target remote", we do not want to unpush the target; then
>> -     the next time the user says "run", we won't be connected.  */
>> +  /* Unlike 'target remote' with no more inferiors, we do not want to
>> +     unpush the target.  If we do then the next time the user says
>> +     "run", we won't be connected.  */
>>  
>>    /* Call common code to mark the inferior as not running.	*/
>>    generic_mourn_inferior ();
>> @@ -10224,7 +10226,7 @@ remote_pid_to_str (struct target_ops *ops, ptid_t ptid)
>>      {
>>        if (ptid_equal (magic_null_ptid, ptid))
>>  	xsnprintf (buf, sizeof buf, "Thread <main>");
>> -      else if (rs->extended && remote_multi_process_p (rs))
>> +      else if (remote_multi_process_p (rs))
>>  	if (ptid_get_lwp (ptid) == 0)
>>  	  return normal_pid_to_str (ptid);
>>  	else
>> @@ -11398,7 +11400,7 @@ remote_supports_multi_process (struct target_ops *self)
>>       processes, even though plain remote can use the multi-process
>>       thread id extensions, so that GDB knows the target process's
>>       PID.  */
> 
> Comment above needs updating.

I just deleted the comment, since it was describing the differences
between remote and extended-remote, and those differences didn't
exist anymore.

> 
>> -  return rs->extended && remote_multi_process_p (rs);
>> +  return remote_multi_process_p (rs);
>>  }
>>  
> 
> Thanks,
> Pedro Alves
> 

Tested on x86_64 native, native-gdbserver, native-extended-gdbserver. No
regressions, other than one issue with gdb.reverse/waitpid-reverse.exp.
My version of GDB seemed to evoke an existing KFAIL, and after that I
would get a new FAIL.  I didn't see the KFAIL on "master", and so the
subsequent FAIL wouldn't show up either.

Here is the updated patch description and ChangeLog.
Thanks!
--Don

This patch implements support for fork and exec events with target remote
mode Linux targets.  For such targets with Linux kernels 2.5.46 and later,
this enables follow-fork-mode, detach-on-fork and fork and exec
catchpoints.  Note that follow-exec-mode is not supported, because target
remote mode does not support the 'run' command.

The changes required to implement this included:

 * Don't exit from gdbserver if there are still active inferiors.

 * Allow changing the active process in remote mode.

 * Enable fork and exec events in remote mode.

 * Print "Ending remote debugging" only when disconnecting.

 * Combine remote_kill and extended_remote_kill into a single function
   that can handle the multiple inferior case for target remote.  Also,
   the same thing for remote_mourn and extended_remote_mourn.

 * Enable process-style ptids in target remote.

 * Remove restriction on multiprocess mode in target remote.

Thanks
--Don

gdb/gdbserver/
2015-12-07  Don Breazeal  <donb@codesourcery.com>

	* server.c (process_serial_event): Don't exit from gdbserver
	in remote mode if there are still active inferiors.

gdb/
2015-12-07  Don Breazeal  <donb@codesourcery.com>

	* inferior.c (number_of_live_inferiors): New function.
	(have_live_inferiors): Use number_of_live_inferiors in place
	of duplicate code.
	* inferior.h (number_of_live_inferiors): Declare new function.
	* remote.c (set_general_process): Remove restriction on target
	remote mode.
	(remote_query_supported): Likewise.
	(remote_detach_1): Exit in target remote mode only when there
	is just one live inferior left.
	(remote_disconnect): Unpush the target directly instead of 
	calling remote_mourn.
	(remote_kill): Rewrite function to handle both target remote
	and extended-remote.  Call remote_kill_k.
	(remote_kill_k): New function.
	(extended_remote_kill): Delete function.
	(remote_mourn, extended_remote_mourn): Combine functions into
	one, remote_mourn, and enable extended functionality for target
	remote.
	(remote_pid_to_str): Enable "process" style ptid string for
	target remote.
	(remote_supports_multi_process): Remove restriction on target
	remote mode.

---
 gdb/gdbserver/server.c |   6 +-
 gdb/inferior.c         |  31 +++++--
 gdb/inferior.h         |   3 +
 gdb/remote.c           | 217 ++++++++++++++++++++++++-------------------------
 4 files changed, 136 insertions(+), 121 deletions(-)

Comments

Pedro Alves Dec. 8, 2015, 12:55 p.m. UTC | #1
On 12/07/2015 10:14 PM, Don Breazeal wrote:

> On 11/20/2015 5:04 AM, Pedro Alves wrote:

>>> Note that follow-exec-mode is not supported, because target
>>> remote mode does not support the 'run' command.
>>
>> Not sure I don't understand this part/comment.
> 
> As I noted in my previous reply, this was an overly-pessimistic way of
> putting it.  I revised the docs etc to say something like this:
> "follow-exec-mode is of limited used in target remote mode, since the 'run'
> command is not supported."  The point being that the main reason a user
> would want to set follow-exec-mode would be to get control over which
> inferior is run when a 'run' command is executed.  It is supported in
> that the inferiors are created or not created as specified, and one could
> (for example) use follow-exec-mode to keep a record of the programs that
> were executed.

Right.  Note that we could support "run" with "target remote", as long
as the remote side supports vRun.  The problem with simply enabling
"run" is that:

 (gdb) target remote
 ...
 (gdb) run
 The program being debugged has been started already.
 Start it from the beginning? (y or n)

... saying yes will kill the inferior and close the connection, before
there's a chance to start the new process...


But, with "set detach-on-fork off", say the program forks, and then the child
execs, and then it exits.  The parent is still running so the "target remote"
connection is still up.  Switch to the child inferior (inferior 2), and
issue "run".  I can see people wondering why that doesn't work.

Likewise the simpler:

 (gdb) target remote ...
 *inferior 1 is now bound to progress N (the only process) running on the target*
 (gdb) add-inferior
 *inferior 2 added*
 (gdb) inferior 2
 (gdb) run

Should we allow that "run" to actually succeed in this case?



((((
Actually trying the detach-on-fork scenario above, we see very
confusing behavior (this is with your series, but I think
current master will have similar problems):

$ gdb -ex "tar rem :9999"
0x0000003615a011f0 in _start () from target:/lib64/ld-linux-x86-64.so.2
(gdb) set detach-on-fork off
(gdb) c
Continuing.
Reading /lib64/libm.so.6 from remote target...
Reading /lib64/libc.so.6 from remote target...
[New Thread 32274.32274]
Reading /home/pedro/gdb/mygit/build/gdb/testsuite/gdb.base/foll-fork from remote target...
Reading /home/pedro/gdb/mygit/build/gdb/testsuite/gdb.base/foll-fork from remote target...
Reading /lib64/libm.so.6 from remote target...
Reading /lib64/libc.so.6 from remote target...
Reading /lib64/ld-linux-x86-64.so.2 from remote target...
Reading /lib64/ld-linux-x86-64.so.2 from remote target...
[Inferior 1 (process 32258) exited normally]
Reading /home/pedro/gdb/mygit/build/gdb/testsuite/gdb.base/foll-fork from remote target...
(gdb) info inferiors
  Num  Description       Executable
* 1    <null>            target:/home/pedro/gdb/mygit/build/gdb/testsuite/gdb.base/foll-fork
  2    process 32274     target:/home/pedro/gdb/mygit/build/gdb/testsuite/gdb.base/foll-fork
(gdb) maint print target-stack
The current target stack is:
  - remote (Remote serial target in gdb-specific protocol)
  - exec (Local exec file)
  - None (None)
(gdb) run
Reading /home/pedro/gdb/mygit/build/gdb/testsuite/gdb.base/foll-fork from remote target...
`target:/home/pedro/gdb/mygit/build/gdb/testsuite/gdb.base/foll-fork' has disappeared; keeping its symbols.
Starting program: target:/home/pedro/gdb/mygit/build/gdb/testsuite/gdb.base/foll-fork
Detaching after fork from child process 32279.
/bin/bash: /home/pedro/gdb/mygit/build/gdb/target:/home/pedro/gdb/mygit/build/gdb/testsuite/gdb.base/foll-fork: No such file or directory
During startup program exited with code 127.
(gdb) maint print target-stack
The current target stack is:
  - exec (Local exec file)
  - None (None)
(gdb)


Note that that last run tried to create the inferior in the
native target and its the native target that silently closes
the remote connection...:

(top-gdb) bt
#0  remote_close (self=0xdde300 <remote_ops>) at /home/pedro/gdb/mygit/src/gdb/remote.c:3403
#1  0x0000000000696101 in target_close (targ=0xdde300 <remote_ops>) at /home/pedro/gdb/mygit/src/gdb/target.c:3308
#2  0x0000000000691e8d in push_target (t=0xece890) at /home/pedro/gdb/mygit/src/gdb/target.c:697
#3  0x00000000004b9495 in inf_ptrace_create_inferior (ops=0xece890, exec_file=0x1bfa190 "target:/home/pedro/gdb/mygit/build/gdb/testsuite/gdb.base/foll-fork",
    allargs=0x1c09f60 "", env=0x12d0220, from_tty=1) at /home/pedro/gdb/mygit/src/gdb/inf-ptrace.c:105
#4  0x00000000004bf6dc in linux_nat_create_inferior (ops=0xece890, exec_file=0x1bfa190 "target:/home/pedro/gdb/mygit/build/gdb/testsuite/gdb.base/foll-fork",
    allargs=0x1c09f60 "", env=0x12d0220, from_tty=1) at /home/pedro/gdb/mygit/src/gdb/linux-nat.c:1194
#5  0x000000000062c018 in run_command_1 (args=0x0, from_tty=1, tbreak_at_main=0) at /home/pedro/gdb/mygit/src/gdb/infcmd.c:604
#6  0x000000000062c121 in run_command (args=0x0, from_tty=1) at /home/pedro/gdb/mygit/src/gdb/infcmd.c:639



I tried the "add-inferior" scenario too now, and it's also
quite confusing:

(gdb) info inferiors
  Num  Description       Executable
* 1    process 32274     target:/home/pedro/gdb/mygit/build/gdb/testsuite/gdb.base/foll-fork
(gdb) add-inferior
Added inferior 2
(gdb) inferior 2
[Switching to inferior 2 [<null>] (<noexec>)]
(gdb) file /home/pedro/gdb/mygit/build/gdb/testsuite/gdb.base/foll-fork
Reading symbols from /home/pedro/gdb/mygit/build/gdb/testsuite/gdb.base/foll-fork...done.
(gdb) set remote exec-file /home/pedro/gdb/mygit/build/gdb/testsuite/gdb.base/foll-fork
(gdb) start
Temporary breakpoint 1 at 0x400671: main. (2 locations)
Starting program: /home/pedro/gdb/mygit/build/gdb/testsuite/gdb.base/foll-fork
Detaching after fork from child process 32353.
Detaching after fork from child process 32354.
Detaching after fork from child process 32356.
Error in re-setting breakpoint 1: Cannot access memory at address 0x400669
Error in re-setting breakpoint 1: Cannot access memory at address 0x400669
Error in re-setting breakpoint 1: Cannot access memory at address 0x400669
Error in re-setting breakpoint 1: Cannot access memory at address 0x400669
Error in re-setting breakpoint 1: Cannot access memory at address 0x400669
[Inferior 2 (process 32353) exited normally]
(gdb) info inferiors
  Num  Description       Executable
  1    <null>            target:/home/pedro/gdb/mygit/build/gdb/testsuite/gdb.base/foll-fork
* 2    <null>            /home/pedro/gdb/mygit/build/gdb/testsuite/gdb.base/foll-fork
(gdb) maint print target-stack
The current target stack is:
  - exec (Local exec file)
  - None (None)
(gdb)

Likewise here start closed the remote connection, and then
find_run_target() returns the native target.

"set auto-connect-native-target off" works around these issues.

))))

> Tested on x86_64 native, native-gdbserver, native-extended-gdbserver. No
> regressions, other than one issue with gdb.reverse/waitpid-reverse.exp.
> My version of GDB seemed to evoke an existing KFAIL, and after that I
> would get a new FAIL.  I didn't see the KFAIL on "master", and so the
> subsequent FAIL wouldn't show up either.

Sorry, I'm confused.  Does this series cause a new FAIL, or not?

> 
> diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
> index 5e053b3..2e0e1d5 100644
> --- a/gdb/gdbserver/server.c
> +++ b/gdb/gdbserver/server.c
> @@ -3959,9 +3959,11 @@ process_serial_event (void)
>  	  discard_queued_stop_replies (pid_to_ptid (pid));
>  	  write_ok (own_buf);
>  
> -	  if (extended_protocol)
> +	  if (extended_protocol || get_first_inferior (&all_threads) != NULL)

Replace that get_first_inferior() by get_first_thread() or even target_running().



> @@ -8800,41 +8797,7 @@ kill_new_fork_children (int pid, struct remote_state *rs)
>  }
>  
>  
> -static void
> -remote_kill (struct target_ops *ops)
> -{
> -
> -  /* Catch errors so the user can quit from gdb even when we
> -     aren't on speaking terms with the remote system.  */
> -  TRY
> -    {
> -      putpkt ("k");
> -    }
> -  CATCH (ex, RETURN_MASK_ERROR)
> -    {
> -      if (ex.error == TARGET_CLOSE_ERROR)
> -	{
> -	  /* If we got an (EOF) error that caused the target
> -	     to go away, then we're done, that's what we wanted.
> -	     "k" is susceptible to cause a premature EOF, given
> -	     that the remote server isn't actually required to
> -	     reply to "k", and it can happen that it doesn't
> -	     even get to reply ACK to the "k".  */
> -	  return;
> -	}
> -
> -	/* Otherwise, something went wrong.  We didn't actually kill
> -	   the target.  Just propagate the exception, and let the
> -	   user or higher layers decide what to do.  */
> -	throw_exception (ex);
> -    }
> -  END_CATCH
> -
> -  /* We've killed the remote end, we get to mourn it.  Since this is
> -     target remote, single-process, mourning the inferior also
> -     unpushes remote_ops.  */
> -  target_mourn_inferior ();
> -}
> +/* Send a kill request to the target using the 'vKill' packet.  */
>  
>  static int
>  remote_vkill (int pid, struct remote_state *rs)
> @@ -8861,56 +8824,99 @@ remote_vkill (int pid, struct remote_state *rs)
>      }
>  }
>  
> +/* Send a kill request to the target using the 'k' packet.  */
> +
>  static void
> -extended_remote_kill (struct target_ops *ops)
> +remote_kill_k (void)
>  {
> -  int res;
> +  /* Catch errors so the user can quit from gdb even when we
> +     aren't on speaking terms with the remote system.  */
> +  TRY
> +    {
> +      putpkt ("k");
> +    }
> +  CATCH (ex, RETURN_MASK_ERROR)
> +    {
> +      if (ex.error == TARGET_CLOSE_ERROR)
> +	{
> +	  /* If we got an (EOF) error that caused the target
> +	     to go away, then we're done, that's what we wanted.
> +	     "k" is susceptible to cause a premature EOF, given
> +	     that the remote server isn't actually required to
> +	     reply to "k", and it can happen that it doesn't
> +	     even get to reply ACK to the "k".  */
> +	  return;
> +	}
> +
> +      /* Otherwise, something went wrong.  We didn't actually kill
> +	 the target.  Just propagate the exception, and let the
> +	 user or higher layers decide what to do.  */
> +      throw_exception (ex);
> +    }
> +  END_CATCH
> +}

Can't tell why you moved the function below rather than rename it
in place.  I suspect that it was just a consequence of how v1 was
written.  If there's no reason for the move, please leave the function
where it was (and check for spurious changes afterwards), which helps
git blame archaeology.

> +
> +/* Target hook to kill the current inferior.  */
> +
> +static void
> +remote_kill (struct target_ops *ops)
> +{


> -static void
> -remote_mourn (struct target_ops *target)
> -{
> -  unpush_target (target);
> +      return;
> +    }
>  
> -  /* remote_close takes care of doing most of the clean up.  */
> -  generic_mourn_inferior ();
> +  error (_("Can't kill process"));
> +  return;

"return" is unreachable after "error".

Otherwise looks good to me.

Thanks,
Pedro Alves
diff mbox

Patch

diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index 5e053b3..2e0e1d5 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -3959,9 +3959,11 @@  process_serial_event (void)
 	  discard_queued_stop_replies (pid_to_ptid (pid));
 	  write_ok (own_buf);
 
-	  if (extended_protocol)
+	  if (extended_protocol || get_first_inferior (&all_threads) != NULL)
 	    {
-	      /* Treat this like a normal program exit.  */
+	      /* There is still at least one inferior remaining, so
+		 don't terminate gdbserver and treat this like a
+		 normal program exit.  */
 	      last_status.kind = TARGET_WAITKIND_EXITED;
 	      last_status.value.integer = 0;
 	      last_ptid = pid_to_ptid (pid);
diff --git a/gdb/inferior.c b/gdb/inferior.c
index 157e236..a0296c8 100644
--- a/gdb/inferior.c
+++ b/gdb/inferior.c
@@ -459,22 +459,41 @@  have_inferiors (void)
   return 0;
 }
 
+/* Return the number of live inferiors.  We account for the case
+   where an inferior might have a non-zero pid but no threads, as
+   in the middle of a 'mourn' operation.  */
+
 int
-have_live_inferiors (void)
+number_of_live_inferiors (void)
 {
   struct inferior *inf;
+  int num_inf = 0;
 
   for (inf = inferior_list; inf; inf = inf->next)
     if (inf->pid != 0)
       {
 	struct thread_info *tp;
-	
-	tp = any_thread_of_process (inf->pid);
-	if (tp && target_has_execution_1 (tp->ptid))
-	  break;
+
+	ALL_NON_EXITED_THREADS (tp)
+	 if (tp && ptid_get_pid (tp->ptid) == inf->pid)
+	   if (target_has_execution_1 (tp->ptid))
+	     {
+	       /* Found a live thread in this inferior, go to the next
+		  inferior.  */
+	       ++num_inf;
+	       break;
+	     }
       }
 
-  return inf != NULL;
+  return num_inf;
+}
+
+/* Return true if there is at least one live inferior.  */
+
+int
+have_live_inferiors (void)
+{
+  return number_of_live_inferiors () > 0;
 }
 
 /* Prune away any unused inferiors, and then prune away no longer used
diff --git a/gdb/inferior.h b/gdb/inferior.h
index d3cf615..78a5ed3 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -490,6 +490,9 @@  extern struct inferior *iterate_over_inferiors (int (*) (struct inferior *,
 /* Returns true if the inferior list is not empty.  */
 extern int have_inferiors (void);
 
+/* Returns the number of live inferiors (real live processes).  */
+extern int number_of_live_inferiors (void);
+
 /* Returns true if there are any live inferiors in the inferior list
    (not cores, not executables, real live processes).  */
 extern int have_live_inferiors (void);
diff --git a/gdb/remote.c b/gdb/remote.c
index 52c5df8..d6bae42 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -123,8 +123,6 @@  static void remote_mourn (struct target_ops *ops);
 
 static void extended_remote_restart (void);
 
-static void extended_remote_mourn (struct target_ops *);
-
 static void remote_send (char **buf, long *sizeof_buf_p);
 
 static int readchar (int timeout);
@@ -2097,7 +2095,7 @@  set_general_process (void)
   struct remote_state *rs = get_remote_state ();
 
   /* If the remote can't handle multiple processes, don't bother.  */
-  if (!rs->extended || !remote_multi_process_p (rs))
+  if (!remote_multi_process_p (rs))
     return;
 
   /* We only need to change the remote current thread if it's pointing
@@ -4609,18 +4607,15 @@  remote_query_supported (void)
 
       q = remote_query_supported_append (q, "qRelocInsn+");
 
-      if (rs->extended)
-	{
-	  if (packet_set_cmd_state (PACKET_fork_event_feature)
-	      != AUTO_BOOLEAN_FALSE)
-	    q = remote_query_supported_append (q, "fork-events+");
-	  if (packet_set_cmd_state (PACKET_vfork_event_feature)
-	      != AUTO_BOOLEAN_FALSE)
-	    q = remote_query_supported_append (q, "vfork-events+");
-	  if (packet_set_cmd_state (PACKET_exec_event_feature)
-	      != AUTO_BOOLEAN_FALSE)
-	    q = remote_query_supported_append (q, "exec-events+");
-	}
+      if (packet_set_cmd_state (PACKET_fork_event_feature)
+	  != AUTO_BOOLEAN_FALSE)
+	q = remote_query_supported_append (q, "fork-events+");
+      if (packet_set_cmd_state (PACKET_vfork_event_feature)
+	  != AUTO_BOOLEAN_FALSE)
+	q = remote_query_supported_append (q, "vfork-events+");
+      if (packet_set_cmd_state (PACKET_exec_event_feature)
+	  != AUTO_BOOLEAN_FALSE)
+	q = remote_query_supported_append (q, "exec-events+");
 
       if (packet_set_cmd_state (PACKET_vContSupported) != AUTO_BOOLEAN_FALSE)
 	q = remote_query_supported_append (q, "vContSupported+");
@@ -4975,7 +4970,8 @@  remote_detach_1 (const char *args, int from_tty)
   /* Tell the remote target to detach.  */
   remote_detach_pid (pid);
 
-  if (from_tty && !rs->extended)
+  /* Exit only if this is the only active inferior.  */
+  if (from_tty && !rs->extended && number_of_live_inferiors () == 1)
     puts_filtered (_("Ending remote debugging.\n"));
 
   /* Check to see if we are detaching a fork parent.  Note that if we
@@ -5071,10 +5067,11 @@  remote_disconnect (struct target_ops *target, const char *args, int from_tty)
   if (args)
     error (_("Argument given to \"disconnect\" when remotely debugging."));
 
-  /* Make sure we unpush even the extended remote targets; mourn
-     won't do it.  So call remote_mourn directly instead of
-     target_mourn_inferior.  */
-  remote_mourn (target);
+  /* Make sure we unpush even the extended remote targets.  Calling
+     target_mourn_inferior won't unpush, and remote_mourn won't
+     unpush if there is more than one inferior left.  */
+  unpush_target (target);
+  generic_mourn_inferior ();
 
   if (from_tty)
     puts_filtered ("Ending remote debugging.\n");
@@ -8800,41 +8797,7 @@  kill_new_fork_children (int pid, struct remote_state *rs)
 }
 
 
-static void
-remote_kill (struct target_ops *ops)
-{
-
-  /* Catch errors so the user can quit from gdb even when we
-     aren't on speaking terms with the remote system.  */
-  TRY
-    {
-      putpkt ("k");
-    }
-  CATCH (ex, RETURN_MASK_ERROR)
-    {
-      if (ex.error == TARGET_CLOSE_ERROR)
-	{
-	  /* If we got an (EOF) error that caused the target
-	     to go away, then we're done, that's what we wanted.
-	     "k" is susceptible to cause a premature EOF, given
-	     that the remote server isn't actually required to
-	     reply to "k", and it can happen that it doesn't
-	     even get to reply ACK to the "k".  */
-	  return;
-	}
-
-	/* Otherwise, something went wrong.  We didn't actually kill
-	   the target.  Just propagate the exception, and let the
-	   user or higher layers decide what to do.  */
-	throw_exception (ex);
-    }
-  END_CATCH
-
-  /* We've killed the remote end, we get to mourn it.  Since this is
-     target remote, single-process, mourning the inferior also
-     unpushes remote_ops.  */
-  target_mourn_inferior ();
-}
+/* Send a kill request to the target using the 'vKill' packet.  */
 
 static int
 remote_vkill (int pid, struct remote_state *rs)
@@ -8861,56 +8824,99 @@  remote_vkill (int pid, struct remote_state *rs)
     }
 }
 
+/* Send a kill request to the target using the 'k' packet.  */
+
 static void
-extended_remote_kill (struct target_ops *ops)
+remote_kill_k (void)
 {
-  int res;
+  /* Catch errors so the user can quit from gdb even when we
+     aren't on speaking terms with the remote system.  */
+  TRY
+    {
+      putpkt ("k");
+    }
+  CATCH (ex, RETURN_MASK_ERROR)
+    {
+      if (ex.error == TARGET_CLOSE_ERROR)
+	{
+	  /* If we got an (EOF) error that caused the target
+	     to go away, then we're done, that's what we wanted.
+	     "k" is susceptible to cause a premature EOF, given
+	     that the remote server isn't actually required to
+	     reply to "k", and it can happen that it doesn't
+	     even get to reply ACK to the "k".  */
+	  return;
+	}
+
+      /* Otherwise, something went wrong.  We didn't actually kill
+	 the target.  Just propagate the exception, and let the
+	 user or higher layers decide what to do.  */
+      throw_exception (ex);
+    }
+  END_CATCH
+}
+
+/* Target hook to kill the current inferior.  */
+
+static void
+remote_kill (struct target_ops *ops)
+{
+  int res = -1;
   int pid = ptid_get_pid (inferior_ptid);
   struct remote_state *rs = get_remote_state ();
 
-  /* If we're stopped while forking and we haven't followed yet, kill the
-     child task.  We need to do this before killing the parent task
-     because if this is a vfork then the parent will be sleeping.  */
-  kill_new_fork_children (pid, rs);
-
-  res = remote_vkill (pid, rs);
-  if (res == -1 && !(rs->extended && remote_multi_process_p (rs)))
+  if (packet_support (PACKET_vKill) != PACKET_DISABLE)
     {
-      /* Don't try 'k' on a multi-process aware stub -- it has no way
-	 to specify the pid.  */
+      /* If we're stopped while forking and we haven't followed yet,
+	 kill the child task.  We need to do this before killing the
+	 parent task because if this is a vfork then the parent will
+	 be sleeping.  */
+      kill_new_fork_children (pid, rs);
 
-      putpkt ("k");
-#if 0
-      getpkt (&rs->buf, &rs->buf_size, 0);
-      if (rs->buf[0] != 'O' || rs->buf[0] != 'K')
-	res = 1;
-#else
-      /* Don't wait for it to die.  I'm not really sure it matters whether
-	 we do or not.  For the existing stubs, kill is a noop.  */
-      res = 0;
-#endif
+      res = remote_vkill (pid, rs);
+      if (res == 0)
+	{
+	  target_mourn_inferior ();
+	  return;
+	}
     }
 
-  if (res != 0)
-    error (_("Can't kill process"));
+  /* If we are in 'target remote' mode and we are killing the only
+     inferior, then we will tell gdbserver to exit and unpush the
+     target.  */
+  if (res == -1 && !remote_multi_process_p (rs)
+      && number_of_live_inferiors () == 1)
+    {
+      remote_kill_k ();
 
-  target_mourn_inferior ();
-}
+      /* We've killed the remote end, we get to mourn it.  If we are
+	 not in extended mode, mourning the inferior also unpushes
+	 remote_ops from the target stack, which closes the remote
+	 connection.  */
+      target_mourn_inferior ();
 
-static void
-remote_mourn (struct target_ops *target)
-{
-  unpush_target (target);
+      return;
+    }
 
-  /* remote_close takes care of doing most of the clean up.  */
-  generic_mourn_inferior ();
+  error (_("Can't kill process"));
+  return;
 }
 
 static void
-extended_remote_mourn (struct target_ops *target)
+remote_mourn (struct target_ops *target)
 {
   struct remote_state *rs = get_remote_state ();
 
+  /* In 'target remote' mode with one inferior, we close the connection.  */
+  if (!rs->extended && number_of_live_inferiors () <= 1)
+    {
+      unpush_target (target);
+
+      /* remote_close takes care of doing most of the clean up.  */
+      generic_mourn_inferior ();
+      return;
+    }
+
   /* In case we got here due to an error, but we're going to stay
      connected.  */
   rs->waiting_for_stop_reply = 0;
@@ -8940,10 +8946,7 @@  extended_remote_mourn (struct target_ops *target)
      current thread.  */
   record_currthread (rs, minus_one_ptid);
 
-  /* Unlike "target remote", we do not want to unpush the target; then
-     the next time the user says "run", we won't be connected.  */
-
-  /* Call common code to mark the inferior as not running.	*/
+  /* Call common code to mark the inferior as not running.  */
   generic_mourn_inferior ();
 
   if (!have_inferiors ())
@@ -10465,7 +10468,7 @@  remote_pid_to_str (struct target_ops *ops, ptid_t ptid)
     {
       if (ptid_equal (magic_null_ptid, ptid))
 	xsnprintf (buf, sizeof buf, "Thread <main>");
-      else if (rs->extended && remote_multi_process_p (rs))
+      else if (remote_multi_process_p (rs))
 	if (ptid_get_lwp (ptid) == 0)
 	  return normal_pid_to_str (ptid);
 	else
@@ -11635,11 +11638,7 @@  remote_supports_multi_process (struct target_ops *self)
 {
   struct remote_state *rs = get_remote_state ();
 
-  /* Only extended-remote handles being attached to multiple
-     processes, even though plain remote can use the multi-process
-     thread id extensions, so that GDB knows the target process's
-     PID.  */
-  return rs->extended && remote_multi_process_p (rs);
+  return remote_multi_process_p (rs);
 }
 
 static int
@@ -13071,6 +13070,14 @@  Specify the serial device it is connected to\n\
   remote_ops.to_btrace_conf = remote_btrace_conf;
   remote_ops.to_augmented_libraries_svr4_read =
     remote_augmented_libraries_svr4_read;
+  remote_ops.to_follow_fork = remote_follow_fork;
+  remote_ops.to_follow_exec = remote_follow_exec;
+  remote_ops.to_insert_fork_catchpoint = remote_insert_fork_catchpoint;
+  remote_ops.to_remove_fork_catchpoint = remote_remove_fork_catchpoint;
+  remote_ops.to_insert_vfork_catchpoint = remote_insert_vfork_catchpoint;
+  remote_ops.to_remove_vfork_catchpoint = remote_remove_vfork_catchpoint;
+  remote_ops.to_insert_exec_catchpoint = remote_insert_exec_catchpoint;
+  remote_ops.to_remove_exec_catchpoint = remote_remove_exec_catchpoint;
 }
 
 /* Set up the extended remote vector by making a copy of the standard
@@ -13089,27 +13096,11 @@  init_extended_remote_ops (void)
 Specify the serial device it is connected to (e.g. /dev/ttya).";
   extended_remote_ops.to_open = extended_remote_open;
   extended_remote_ops.to_create_inferior = extended_remote_create_inferior;
-  extended_remote_ops.to_mourn_inferior = extended_remote_mourn;
   extended_remote_ops.to_detach = extended_remote_detach;
   extended_remote_ops.to_attach = extended_remote_attach;
   extended_remote_ops.to_post_attach = extended_remote_post_attach;
-  extended_remote_ops.to_kill = extended_remote_kill;
   extended_remote_ops.to_supports_disable_randomization
     = extended_remote_supports_disable_randomization;
-  extended_remote_ops.to_follow_fork = remote_follow_fork;
-  extended_remote_ops.to_follow_exec = remote_follow_exec;
-  extended_remote_ops.to_insert_fork_catchpoint
-    = remote_insert_fork_catchpoint;
-  extended_remote_ops.to_remove_fork_catchpoint
-    = remote_remove_fork_catchpoint;
-  extended_remote_ops.to_insert_vfork_catchpoint
-    = remote_insert_vfork_catchpoint;
-  extended_remote_ops.to_remove_vfork_catchpoint
-    = remote_remove_vfork_catchpoint;
-  extended_remote_ops.to_insert_exec_catchpoint
-    = remote_insert_exec_catchpoint;
-  extended_remote_ops.to_remove_exec_catchpoint
-    = remote_remove_exec_catchpoint;
 }
 
 static int