[06/16,v3] Extended-remote Linux follow fork

  On 11/13/2014 11:06 AM, Breazeal, Don wrote:
> On 11/13/2014 10:59 AM, Pedro Alves wrote:
>> On 11/13/2014 06:53 PM, Breazeal, Don wrote:
>>> My initial approach was to do just that, but I ended up with
>>> linux-specific code in remote.c (the code that lives in linux-nat.c
>>> for the native implementation).  I guess the direction of recent
>>> changes would be to put that code into a common file in gdb/nat,
>>> if possible.  Would that be the approach you would recommend?
>>
>> I'm not seeing what would be linux-specific?  On remote_follow_fork
>> fork, we switch the current remote thread to gdb's current
>> thread (either parent or child), by
>> calling 'set_general_thread (inferior_ptid);'
>> And then if we need to detach parent or child, we detach it with
>> the D;PID packet.
>>
>> Thanks,
>> Pedro Alves
>>
> I don't recall the details at this point.  I'll proceed
> with your recommendation, assuming I don't need any
> common code, and if I run into a problem I'll post a
> question about it.
> 
> Thanks
> --Don
> 
> 
Hi Pedro,

I have finally had a chance to spend a chunk of time on this.  I have
removed the vFollowFork packet and enabled fork events for remote as
well as extended-remote.  When gdbserver detaches an inferior, it
only exits if it's not in extended mode *and* there are no more inferiors.

This version of the patch is *mostly working*, but should still be
considered WIP.  I wanted to get it posted to make sure that we are
aligned on the direction that the implementation is taking before
going too much farther with it.

BTW this has made it back to the top of my priority list, so I should be
more responsive to reviews for the next month or so.

One WIP issue: extended-remote targets are working as well as before,
but there are still a few failures with 'target remote': an assertion in
multi-forks.exp and a few failures in gdb.threads/linux-dp.exp and
gdb.trace/report.exp.

Another WIP issue: I have a question about how to deal with the
workaround for the hardware single-step kernel bug, as in
linux-nat.c:linux_child_follow_fork where it manually steps the inferior
before detaching.  In the remote implementation I put the workaround in
a function linux-low.c:linux_detach_fork and called it from
linux_detach_one_lwp.  I experimented with keeping some state in the
thread_info structure flagging the "pending follow", but since gdbserver
doesn't know which inferiors are being followed, there was no way to
clean up the threads that weren't detached.  (I hope that makes sense).

In the current implementation it just executes linux_detach_fork for all
detach requests, whether they have anything to do with a fork or not.
That seems to work, but one might consider it a questionable solution.
Another option could be to just not work around the kernel bug in the
remote case.  It looks like the kernel fix may be as old as December of
2009 -- is that old enough so that for a new feature we can just skip
it?  Or are there other options I should be considering?

The revised patch is below, including an updated commit message.
Thanks!
--Don

This patch implements basic support for follow-fork and detach-on-fork on
remote and extended-remote Linux targets.  Only 'fork' is supported in this
patch; 'vfork' support is added n a subsequent patch.  Sufficient
extended-remote functionality has been implemented here to pass
gdb.base/foll-fork.exp with the catchpoint tests commented out.

The implementation follows the same general structure as for the native
implementation as much as possible.

This implementation included:
 * enabling fork events in linux-low.c in initialize_low and
   linux_enable_extended_features

 * handling fork events in gdbserver/linux-low.c:handle_extended_wait

   - when a fork event occurs in gdbserver, we must do the full creation
     of the new process, thread, lwp, and breakpoint lists.  This is
     required whether or not the new child is destined to be
     detached-on-fork, because GDB will make target calls that require all
     the structures.  In particular we need the breakpoint lists in order
     to remove the breakpoints from a detaching child.  If we are not
     detaching the child we will need all these structures anyway.

   - as part of this event handling we store the target_waitstatus in a new
     member of the parent thread_info structure, 'pending_follow'.  This
     is used to store extended event information for reporting to GDB.

   - handle_extended_wait is given a return value, denoting whether the
     handled event should be reported to GDB.  Previously it had only
     handled clone events, which were never reported.

 * using a new predicate in gdbserver to control handling of the fork event
   (and eventually all extended events) in linux_wait_1.  The predicate,
   extended_event_reported, checks a target_waitstatus.kind for an
   extended ptrace event.

 * adds a function linux_detach_fork to the gdbserver detach procedure.
   This function implements the workaround for the kernel bug related
   to inheritance of hardware singlestep settings.

 * implementing a new RSP 'T' Stop Reply Packet stop reason: "fork", in
   gdbserver/remote-utils.c and remote.c.

 * implementing new target and RSP support for target_follow_fork with
   target extended-remote.  (The RSP components were actually defined in
   patch 4, but they see their first use here).

   - remote target routine remote_follow_fork, which just sends the 'D;pid'
     detach packet to detach the new fork child cleanly.  We can't just
     call target_detach because the fork child data structures have not
     been allocated on the host side.

Tested on x64 Ubuntu Lucid, native, remote, extended-remote.

gdb/gdbserver/
2014-12-05  Don Breazeal  <donb@codesourcery.com>

	* gdbthread.h (struct thread_info) <pending_follow>: New member.
	* linux-low.c (handle_extended_wait): Implement return value,
	rename event_child to event_lwp, handle PTRACE_EVENT_FORK.
	(linux_detach_fork): New function.
	(linux_detach_one_lwp): Call linux_detach_fork.
	(linux_low_filter_event): Use return value from
	handle_extended_wait.
	(extended_event_reported): New function.
	(linux_write_memory): Add 'process <pid>' to debug message.
	* remote-utils.c (prepare_resume_reply): Implement stop reason
	'fork' for 'T' stop message.
	* server.c (process_serial_event): Don't exit if there is still
	one inferior.

gdb/
2014-12-05  Don Breazeal  <donb@codesourcery.com>

	* remote.c (remote_fork_event_p): Move out of #ifdef.
	(remote_follow_fork): New function.
	(set_general_process): Remove extended-mode restriction.
	(remote_detach_1): Add struct target_ops argument, don't
	mourn detached process if it was detach-on-fork.
	(remote_detach, extended_remote_detach): Pass struct target_ops
	argument to remote_detach_1.
	(remote_parse_stop_reply): Handle new 'T' stop reason 'fork'.
	(extended_remote_kill): Remove extended-mode restriction.
	(remote_pid_to_str): Print 'process' strings for pid/0/0 ptids.
	(remote_supports_multi_process): Remove extended-mode restriction.

---
 gdb/gdbserver/gdbthread.h    |   4 +
 gdb/gdbserver/linux-low.c    | 172
+++++++++++++++++++++++++++++++++++++++----
 gdb/gdbserver/remote-utils.c |  14 +++-
 gdb/gdbserver/server.c       |   7 +-
 gdb/remote.c                 |  97 +++++++++++++++++++-----
 5 files changed, 259 insertions(+), 35 deletions(-)


 /* Same as remote_detach, but don't send the "D" packet; just
disconnect.  */
@@ -5561,7 +5618,8 @@ remote_parse_stop_reply (char *buf, struct
stop_reply *event)
 	     as a register number.  */

 	  if (strncmp (p, "awatch", strlen("awatch")) != 0
-	      && strncmp (p, "core", strlen ("core") != 0))
+	      && strncmp (p, "core", strlen ("core") != 0)
+	      && strncmp (p, "fork", strlen ("fork") != 0))
 	    {
 	      /* Read the ``P'' register number.  */
 	      pnum = strtol (p, &p_temp, 16);
@@ -5613,6 +5671,11 @@ Packet: '%s'\n"),
 		  p = unpack_varlen_hex (++p1, &c);
 		  event->core = c;
 		}
+	      else if (strncmp (p, "fork", p1 - p) == 0)
+		{
+		  event->ws.value.related_pid = read_ptid (++p1, &p);
+		  event->ws.kind = TARGET_WAITKIND_FORKED;
+		}
 	      else
 		{
 		  /* Silently skip unknown optional info.  */
@@ -7877,7 +7940,7 @@ extended_remote_kill (struct target_ops *ops)
   struct remote_state *rs = get_remote_state ();

   res = remote_vkill (pid, rs);
-  if (res == -1 && !(rs->extended && remote_multi_process_p (rs)))
+  if (res == -1 && !(remote_multi_process_p (rs)))
     {
       /* Don't try 'k' on a multi-process aware stub -- it has no way
 	 to specify the pid.  */
@@ -9423,9 +9486,12 @@ 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))
-	xsnprintf (buf, sizeof buf, "Thread %d.%ld",
-		   ptid_get_pid (ptid), ptid_get_lwp (ptid));
+      else if (remote_multi_process_p (rs))
+	if (ptid_get_lwp (ptid) == 0)
+	  return normal_pid_to_str (ptid);
+	else
+	  xsnprintf (buf, sizeof buf, "Thread %d.%ld",
+		     ptid_get_pid (ptid), ptid_get_lwp (ptid));
       else
 	xsnprintf (buf, sizeof buf, "Thread %ld",
 		   ptid_get_lwp (ptid));
@@ -10422,11 +10488,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
@@ -11559,6 +11621,7 @@ Specify the serial device it is connected to\n\
   remote_ops.to_remove_watchpoint = remote_remove_watchpoint;
   remote_ops.to_kill = remote_kill;
   remote_ops.to_load = remote_load;
+  remote_ops.to_follow_fork = remote_follow_fork;
   remote_ops.to_mourn_inferior = remote_mourn;
   remote_ops.to_pass_signals = remote_pass_signals;
   remote_ops.to_program_signals = remote_program_signals;
  

[06/16,v3] Extended-remote Linux follow fork

Commit Message

Comments

Patch