diff mbox

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

Message ID 566F18A8.2020006@codesourcery.com
State New
Headers show

Commit Message

Don Breazeal Dec. 14, 2015, 7:29 p.m. UTC
On 12/8/2015 4:55 AM, Pedro Alves wrote:
> 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?

I'm a believer in "either it always works, or it never works".

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

Wow.  I have a backlog of new tests that we've talked about related to
the fork/exec event support. I can likely make some other tests out of
the above as well.

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

Sorry about the gibberish.  It is a FAIL that didn't occur with target
remote mode before, but now it behaves identically to how native and
extended-remote behave by generating the FAIL.

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

Done.

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

This is back in place.

> 
>> +
>> +/* 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".

Fixed.

> 
> Otherwise looks good to me.
> 
> Thanks,
> Pedro Alves
> 
In testing after making the changes above I ran into a failure I hadn't
seen before, which turned out to be an uninitialized variable that had
been exposed by the new functionality.  That resulted in this valgrind
output:

    ==12133== Invalid free() / delete / delete[] / realloc()
    ==12133==    at 0x4C2BDEC: free (in
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
    ==12133==    by 0x7943FF: xfree (common-utils.c:100)
    ==12133==    by 0x4D0DEE: free_private_thread_info (remote.c:457)
    ==12133==    by 0x62F0A2: free_thread (thread.c:200)
    ==12133==    by 0x62F8DF: delete_thread_1 (thread.c:464)
    ==12133==    by 0x62F93B: delete_thread_silent (thread.c:480)
    ==12133==    by 0x783386: delete_thread_of_inferior (inferior.c:189)
    ==12133==    by 0x62FA2E: iterate_over_threads (thread.c:531)
    ==12133==    by 0x7834EF: exit_inferior_1 (inferior.c:250)
    ==12133==    by 0x7835EC: exit_inferior_silent (inferior.c:288)
    ==12133==    by 0x7836DE: discard_all_inferiors (inferior.c:328)
    ==12133==    by 0x4D5D3C: remote_close (remote.c:3432)
    ==12133==  Address 0x746e69206669206b is not stack'd, malloc'd or
(recently) free'd

I've included the fix in my commit as obvious:

 	q = remote_query_supported_append (q, "vContSupported+");
@@ -4975,7 +4973,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 +5070,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,42 +8800,53 @@ kill_new_fork_children (int pid, struct
remote_state *rs)
 }

 
+/* 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 ();

-  /* Catch errors so the user can quit from gdb even when we
-     aren't on speaking terms with the remote system.  */
-  TRY
+  if (packet_support (PACKET_vKill) != PACKET_DISABLE)
     {
-      putpkt ("k");
-    }
-  CATCH (ex, RETURN_MASK_ERROR)
-    {
-      if (ex.error == TARGET_CLOSE_ERROR)
+      /* 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 == 0)
 	{
-	  /* 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".  */
+	  target_mourn_inferior ();
 	  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);
+  /* 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 ();
+
+      /* 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 ();
+
+      return;
     }
-  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 ();
+  error (_("Can't kill process"));
 }

+/* Send a kill request to the target using the 'vKill' packet.  */
+
 static int
 remote_vkill (int pid, struct remote_state *rs)
 {
@@ -8861,55 +8872,52 @@ 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;
-  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)))
+  /* Catch errors so the user can quit from gdb even when we
+     aren't on speaking terms with the remote system.  */
+  TRY
     {
-      /* Don't try 'k' on a multi-process aware stub -- it has no way
-	 to specify the pid.  */
-
       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
     }
+  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;
+	}

-  if (res != 0)
-    error (_("Can't kill process"));
-
-  target_mourn_inferior ();
+      /* 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
 }

 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 close the
connection.  */
+  if (!rs->extended && number_of_live_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.  */
@@ -8940,10 +8948,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 +10470,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 +11640,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 +13072,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 +13098,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

Comments

Pedro Alves Dec. 15, 2015, 10:52 a.m. UTC | #1
On 12/14/2015 07:29 PM, Don Breazeal wrote:
> On 12/8/2015 4:55 AM, Pedro Alves wrote:

> Wow.  I have a backlog of new tests that we've talked about related to
> the fork/exec event support. I can likely make some other tests out of
> the above as well.

That'd be great, thanks!

>>
>> Sorry, I'm confused.  Does this series cause a new FAIL, or not?
> 
> Sorry about the gibberish.  It is a FAIL that didn't occur with target
> remote mode before, but now it behaves identically to how native and
> extended-remote behave by generating the FAIL.

Ah, I see now.  Sorry for being dense.

> 
> This is now pushed; the updated patch is attached.
> I think with this, basic remote fork and exec events are complete.  I
> appreciate all your help with this project, Pedro.  Thanks for hanging
> in there with me!

Thanks for doing all this!

Thanks,
Pedro Alves
diff mbox

Patch

--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -1918,7 +1918,8 @@  demand_private_info (ptid_t ptid)
       info->priv = XNEW (struct private_thread_info);
       info->private_dtor = free_private_thread_info;
       info->priv->core = -1;
-      info->priv->extra = 0;
+      info->priv->extra = NULL;
+      info->priv->name = NULL;


This is now pushed; the updated patch is attached.
I think with this, basic remote fork and exec events are complete.  I
appreciate all your help with this project, Pedro.  Thanks for hanging
in there with me!
--Don

From: Don Breazeal <donb@codesourcery.com>
Subject: [pushed][PATCH v3 2/3] Target remote mode fork and exec events

This is the version of the patch that was pushed.  The only differences
from v2 are:

 * server.c:process_serial_event, use target_running instead of
   get_first_inferior
 * replace remote_kill in its original location in remote.c
 * remote.c:remote_kill, remove unreachable "return"

I also ran into an intermittent failure that was caused by an
uninitialized variable, exposed by the new functionality.  I just
included the change here as obvious:

--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -1918,7 +1918,8 @@  demand_private_info (ptid_t ptid)
       info->priv = XNEW (struct private_thread_info);
       info->private_dtor = free_private_thread_info;
       info->priv->core = -1;
-      info->priv->extra = 0;
+      info->priv->extra = NULL;
+      info->priv->name = NULL;

Here is the commit message:

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.

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.

gdb/gdbserver/ChangeLog:

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

gdb/ChangeLog:

	* 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/ChangeLog           |  25 ++++++
 gdb/gdbserver/ChangeLog |   5 ++
 gdb/gdbserver/server.c  |   6 +-
 gdb/inferior.c          |  31 ++++++--
 gdb/inferior.h          |   3 +
 gdb/remote.c            | 205
+++++++++++++++++++++++-------------------------
 6 files changed, 161 insertions(+), 114 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index f9ed66d..9ded377 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,28 @@ 
+2015-12-11  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.
+
 2015-12-14  Andrew Burgess  <andrew.burgess@embecosm.com>

 	* i386-tdep.c (i386_mpx_info_bounds): Use TYPE_LENGTH.
diff --git a/gdb/gdbserver/ChangeLog b/gdb/gdbserver/ChangeLog
index 617c249..63bb250 100644
--- a/gdb/gdbserver/ChangeLog
+++ b/gdb/gdbserver/ChangeLog
@@ -1,3 +1,8 @@ 
+2015-12-11  Don Breazeal  <donb@codesourcery.com>
+
+	* server.c (process_serial_event): Don't exit from gdbserver
+	in remote mode if there are still active inferiors.
+
 2015-12-11  Yao Qi  <yao.qi@linaro.org>

 	* linux-aarch64-low.c (aarch64_breakpoint_at): Call
diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index a09841c..8f097e5 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 || target_running ())
 	    {
-	      /* Treat this like a normal program exit.  */
+	      /* There is still at least one inferior remaining or
+		 we are in extended mode, so don't terminate gdbserver,
+		 and instead 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..1190522 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -119,12 +119,12 @@  struct remote_state;

 static int remote_vkill (int pid, struct remote_state *rs);

+static void remote_kill_k (void);
+
 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);
@@ -1918,7 +1918,8 @@  demand_private_info (ptid_t ptid)
       info->priv = XNEW (struct private_thread_info);
       info->private_dtor = free_private_thread_info;
       info->priv->core = -1;
-      info->priv->extra = 0;
+      info->priv->extra = NULL;
+      info->priv->name = NULL;
     }

   return info->priv;
@@ -2097,7 +2098,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 +4610,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)