[v5,03/06] Extended-remote Linux follow fork

  This is an updated version of the patch last reviewed here:
https://sourceware.org/ml/gdb-patches/2015-02/msg00261.html

Hi Pedro,
Here are responses to your review comments:

On 2/10/2015 8:36 AM, Pedro Alves wrote:
> On 01/25/2015 09:46 PM, Don Breazeal wrote:
...
>>> 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.
>> 
>> There are two linux-specific issues, both of which are workarounds for
>> kernel issues.
>> 
>> 1) workaround for the hardware single-step kernel bug that causes the 
>>    hardware single-step flag to be inherited across a vfork.  In
>>    linux-nat.c:linux_child_follow_fork we manually step the inferior
>>    before detaching.
>> 
>> 2) workaround for kernels that support PTRACE_O_TRACEVFORK but not
>>    PTRACE_O_TRACEVFORKDONE.  In linux-nat.c:linux_child_follow_fork
>>    we run the vfork child for a little while, then create a fake
>>    vfork-done event.
>> 
>> I propose that we don't do either of these workarounds for remote 
>> follow fork.  For (1), the kernel fix that eliminates the need for
>> this looks like it is from 2009.  For (2), the workaround is for
>> kernels between versions 2.5.46 (04.Nov.2002) and 2.6.18 (20.Sep.2006).
>> Is it necessary to provide workarounds in a new feature for kernels
>> as old as these?  (1) in particular is difficult to implement, since
>> gdbserver has no way to know whether detach-on-fork has been set, or
>> whether a given detach has anything to do with a vfork that preceded
>> it.  OK to drop these?
>
> OK, let's do that, even if just to focus on the essence first.

Thanks.

>>    - 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.
>
> This may need to be adjusted a bit when rebased on mainline.
> handle_extended_wait now never resumes any LWP.

I think there were some mainline changes in this area that I hadn't
caught.  I've made some changes here that I hope will address this concern.
Primarily linux_low_filter_event doesn't return after the call to
handle_extended_wait in the case where we get an event that we want to
report to GDB.

> We never, ever, send "T05 fork" to a GDB that does not understand that, right?
> Because although GDB skips magic register "numbers", "fork" starts with
> a hex digit and a GDB that doesn't understand it would be very confused.
> We did that mistake with "core", and broke backwards compatibility then.

The only time we will send GDB a "T05 fork" is when it has previously 
included "fork-events+" in the qSupported packet.  I did change the
"stop reason" from "fork" to "xfork" anyway, just to eliminate any
possible hex digit confusion.  I see that you changed how this is
handled, so maybe that change is now unnecessary.

>> diff --git a/gdb/gdbserver/gdbthread.h b/gdb/gdbserver/gdbthread.h
>> index b3e3e9d..33643af 100644
>> --- a/gdb/gdbserver/gdbthread.h
>> +++ b/gdb/gdbserver/gdbthread.h
>> @@ -41,6 +41,10 @@ struct thread_info
>>    /* True if LAST_STATUS hasn't been reported to GDB yet.  */
>>    int status_pending_p;
>>  
>> +  /* This is used to store fork and exec event information until
>> +     it is reported to GDB.  */
>> +  struct target_waitstatus pending_follow;
>> +
>
> I'm not sure this interact correctly with status_pending_p.  On the
> GDB/linux-nat.c side, the equivalent would be lp->waitstatus, not
> thread->pending_follow, and the former is considered a pending status.
> See linux-nat.c:lwp_status_pending_p.
> 
> Consider the case of debugging more than one process simulatenously,
> and two forking at the same time.  GDB is only going to report one
> fork event at a time.  So the other event will need to stay pending.
> 
> But I don't see anything that would make status_pending_p_callback
> understand that the forked LWP has a pending event to report, but
> that it has already been processed by handle_extended_wait.
> We can't process the same extended wait twice.  GDBserver would
> get confused and likely hang (waiting for the wait status of the child).

There are a couple of issues here, I think.
1) putting this field in the thread_info structure and naming it
   pending_follow wasn't consistent with the native side's implementation,
   where thread_info.pending_follow is used for a different purpose.  To
   be consistent with the native side, this should be in the lwp_info
   structure, named waitstatus.

I have made this change.

2) This field should interact with status_pending_p.

In the previous version of the patch, I had cleverly hidden the
assignment of status_pending_p in linux_low_filter_event after the
return from handle_extended_wait. :-S

In this new version I've put the assignment right next to the assignment of
lp->waitstatus inside handle_extended_wait.  Thanks for pointing this out;
it makes more sense this way.

>> +static void
>> +linux_low_enable_events (pid_t pid, int attached)
>> +{
>> +  if (!report_fork_events)
>> +    linux_ptrace_clear_additional_flags (PTRACE_O_TRACEFORK);
>
> I'm not sure about this bit.  We should also consider cases like:
> 
>   1. A GDB that supports extended-remote and fork events starts
>     a program.
>   2. GDB disconnects
>   3. A new GDB connection comes along.  But this time, this GDB
>     does not understand fork events.
> 
> And the reverse too.
> 
> Also, if a non-stop or disconnected tracing connection
> disconnects, it'll leave the program running.  When a new
> GDB connects, we may need to change the ptrace options
> to enable/disable these events.

In this version of the patch I've implemented a new gdbserver
target function that is called when a qSupported packet is
processed.  The assumption is that receipt of a qSupported
packet is equivalent to the start of a new GDB connection.

I've made some changes to linux-ptrace.c that keep track
of the "additional flags" that the OS supports, as well as
the set of valid "additional flags".  Another new function
uses this information to reset the ptrace options based on
what GDB requests in the qSupported option.

> 
> Alternatively, always enable the events at the ptrace
> level, and then filter them out if the connected
> GDB doesn't support them.

I made an attempt at doing this back when I switched to using the
qSupported packet for the fork event features, and concluded that
the complexity of handling the events but not reporting them wasn't
worth the benefit.  By not enabling them if GDB doesn't want them
we can just use the existing (working) code.

> 
> Or even, report a T05 / SIGTRAP to GDBs that don't support
> the events.  That may actually be better than not reporting
> anything like today, because what we do today (ignore the
> fork child) is just a recipe for it crashing when it trips
> on a breakpoint that was set on the parent.  A random SIGTRAP
> at least gives the user a chance of doing something.

My approach has been to make the implementation behave like
the native implementation and to keep the default case the
same as how things work today.  So although this sounds like
it would be a nice feature, I propose that we leave reporting
a SIGTRAP in this case for another day.  This patch series
has taken a while, and I'm concerned about the impact of
adding new functionality.

> 
> Hmm.  Maybe we should rename "T05 fork" to something that
> doesn't start with "f" (and hex char), and then that'd be
> the simplest.  We'd always send the event over.  That may
> be best, as it gives us more leverage even if we decide to
> not report the events to old GDB, and then change our minds.

As noted previously, this is now "T05 xfork'.

> 
> (Even with that, I think it'd be good to keep GDB / GDBserver
> broadcasting the support in qSupported.)

Agreed.

>> +/* Return non-zero if WAITSTATUS reflects an extended linux
>> +   event that gdbserver supports.  Otherwise, return zero.  */
>
> I don't understand "gdbserver supports" here.  This is gdbserver
> code, so anything gdbserver doesn't support shouldn't even
> compile, right?  :-)  Can you clarify this comment please?

You are right, that didn't make sense.  Removed "gdbserver supports".
Sorry for the noise.

>> +++ b/gdb/gdbserver/remote-utils.c
>> @@ -1114,12 +1114,24 @@ prepare_resume_reply (char *buf, ptid_t ptid,
>>    switch (status->kind)
>>      {
>>      case TARGET_WAITKIND_STOPPED:
>> +    case TARGET_WAITKIND_FORKED:
>>        {
>>  	struct thread_info *saved_thread;
>>  	const char **regp;
>>  	struct regcache *regcache;
>>  
>> -	sprintf (buf, "T%02x", status->value.sig);
>> +	if (status->kind == TARGET_WAITKIND_FORKED && multi_process)
>
> I'm not sure this "multi_process" check here is correct.
> Shouldn't it be "report_fork_events" instead?

You are correct.  Fixed.

>> +	    sprintf (buf, "T%02x%s:p%x.%lx;", signal, event,
>> +		     ptid_get_pid (status->value.related_pid),
>> +		     ptid_get_lwp (status->value.related_pid));
>
> Can this use write_ptid?

Yes, done.

>> +#include "inf-child.h"
>
> That's a layering violation.  inf-child.h is the
> base/prototype target for default child (native) targets.

Got it.  Fixed.

>> +static int
>> +remote_follow_fork (struct target_ops *target, int follow_child,
>> +		    int detach_fork)
>> +{
>> +  struct remote_state *rs = get_remote_state ();
>> +
>> +  /* Checking the fork event is sufficient for both fork and vfork.  */
>
> Why?

Because the fork and vfork events were both introduced in the same version
of the Linux kernel (as I expect you knew already :-) ).  But, for this
patch we only need fork events anyway, so I just removed the comment from
this patch.  I put an appropriately explanatory comment in the follow vfork
patch.  I could go ahead and add remote_vfork_event_p in the vfork patch 
instead, if that would be preferable.

>> +  if (remote_fork_event_p (rs))
>> +    {
>> +      if (detach_fork && !follow_child)
>> +	{
>> +	  ptid_t child_ptid;
>> +	  pid_t child_pid;
>> +
>> +	  gdb_assert ((inferior_thread ()->pending_follow.kind
>> +		       == TARGET_WAITKIND_FORKED));
>> +	  child_ptid = inferior_thread ()->pending_follow.value.related_pid;
>> +	  child_pid = ptid_get_pid (child_ptid);
>> +
>> +	  /* Tell the remote target to detach.  */
>> +	  xsnprintf (rs->buf, get_remote_packet_size (), "D;%x", child_pid);
>> +
>> +	  putpkt (rs->buf);
>> +	  getpkt (&rs->buf, &rs->buf_size, 0);
>> +
>> +	  if (rs->buf[0] == 'O' && rs->buf[1] == 'K')
>> +	    ;
>> +	  else if (rs->buf[0] == '\0')
>> +	    error (_("Remote doesn't know how to detach"));
>> +	  else
>> +	    error (_("Can't detach process."));
>
> Instead of copying/inlining this part of remote_detach_1 here,
> can you please factor out this bit to a function that is used
> both here and in remote_detach_1 ?

I figured out that I could just call remote_detach_1 from
remote_follow_fork.  I just had to set inferior_ptid to the
child's ptid, call remote_detach_1, then restore inferior_ptid
with the parent's ptid.

>> +
>> +	  inferior_ptid = null_ptid;
>> +	  detach_inferior (child_pid);
>> +	  inf_child_maybe_unpush_target (target);
>
> Please just do:
> 
>   if (!have_inferiors ())
>     unpush_target (target);
> 
> inf_child_maybe_unpush_target checks the inf_child_explicitly_opened
> flag that is completely unrelated to "target remote".  That's for
> "target native".
> 
> But wait, why would you ever want to unpush the target here?

You are right, we wouldn't want to.  Even for 'target remote', there
will still be at least one inferior at this point (the parent), so
we will always want to keep the target.

>> +remote_detach_1 (struct target_ops *ops, const char *args,
>> +		 int from_tty, int extended)
>>  {
>>    int pid = ptid_get_pid (inferior_ptid);
>>    struct remote_state *rs = get_remote_state ();
>> +  struct thread_info *tp = first_thread_of_process (pid);
>>  
>>    if (args)
>>      error (_("Argument given to \"detach\" when remotely debugging."));
>> @@ -4442,19 +4492,28 @@ remote_detach_1 (const char *args, int from_tty, int extended)
>>    if (from_tty && !extended)
>>      puts_filtered (_("Ending remote debugging.\n"));
>>  
>> -  target_mourn_inferior ();
>> +  /* If doing detach-on-fork, we don't mourn, because that will delete
>> +     breakpoints that should be available for the child.  */
>> +  if (tp->pending_follow.kind != TARGET_WAITKIND_FORKED)
>> +    target_mourn_inferior ();
>> +  else
>> +    {
>> +      inferior_ptid = null_ptid;
>> +      detach_inferior (pid);
>> +      inf_child_maybe_unpush_target (ops);
>
> Likewise.  If this is the last process, and we're doing
> extended-remote, this will disconnect from the target, but
> extended-remote shouldn't disconnect.  This doesn't look like
> will do the right thing?

It seemed to work OK, I assume because to get to the call to
inf_child_maybe_unpush_target it had to have been dealing with
a fork.  But you are right, the call is unnecessary, aside from
being the wrong function to call.

I think this was an artifact of my work on 'target remote'.  I expect
that we may revisit this sort of thing when we get to that patch,
although thanks to this review I understand it all much better.

The updated version looks like this:

+  /* If doing detach-on-fork, we don't mourn, because that will delete
+     breakpoints that should be available for the followed inferior.  */
+  if (!is_fork_child && !is_fork_parent)
+    target_mourn_inferior ();
+  else
+    {
+      inferior_ptid = null_ptid;
+      detach_inferior (pid);
+    }

>> diff --git a/gdb/testsuite/gdb.base/multi-forks.exp b/gdb/testsuite/gdb.base/multi-forks.exp
>> index e95cb4b..5682730 100644
>> --- a/gdb/testsuite/gdb.base/multi-forks.exp
>> +++ b/gdb/testsuite/gdb.base/multi-forks.exp
>> @@ -62,6 +62,18 @@ proc continue_to_exit_bp_loc {} {
>>      set seen_break 0
>>      set seen_prompt 0
>>      set seen_timeout 0
>> +
>> +    # If we are running with gdbserver, the output ($decimal done) will
>> +    # come via the spawn_id of gdbserver, not the spawn_id of gdb (the
>> +    # default).  So we grab the spawn_id of gdbserver, if it exists, and
>> +    # add it to the gdb_expect statement below using "-i", allowing us
>> +    # to apply the expect statement to the output of both spawn_ids.
>> +    global server_spawn_id
>> +    set current_spawn_id [board_info host fileid]
>> +    if {![info exists server_spawn_id]} {
>> +	set server_spawn_id ""
>> +    }
>> +
>
> Clever.  :-)  But it also means that the test will fail on
> true remote testing, right?
> 
> ISTM that if we don't have a server_spawn_id, the test should
> not be expecting the "done"s when gdb,noinferiorio is set.

I have changed the test so that if gdb,noinferiorio is set it
just ignores the test for the "done"s from the inferior.

I saw that you had implemented a more general version of the
spawn id changes.  I don't know if this will need to change
to work with that, but if it does presumably the changes will
be minor.

> +global server_spawn_id
>
> This should have a describing comment.
>
Done, but probably moot given your work on this.

>> +++ b/gdb/testsuite/lib/gdbserver-support.exp
>> @@ -39,7 +39,6 @@
>>  #	After GDB starts you should check global $gdbserver_gdbport for the
>>  #	real port used.  It is not useful if $gdbserver_reconnect_p was not set.
>>  #
>> -
>>  #
>
> Please don't remove that line.  It was splitting different sections.
> Above is baseboard config options.  Below is description of the
> following procedure.

Sorry about that.  I'll have to add "removed blank line" to the list of
common formatting errors I check for.

The updated patch is below.
Thanks!
--Don

This patch implements basic support for follow-fork and detach-on-fork on
extended-remote Linux targets.  Only 'fork' is supported in this patch;
'vfork' support is added n a subsequent patch.  This patch depends on 
the previous patches in the patch series.

Sufficient extended-remote functionality has been implemented here to pass
gdb.base/multi-forks.exp, as well as gdb.base/foll-fork.exp with the
catchpoint tests commented out.  Some other fork tests fail with this
patch because it doesn't provide the architecture support needed for
watchpoint inheritance or fork catchpoints.

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.

 * 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 data structures for the forked child
     have not been allocated on the host side.

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

gdb/doc/
2015-02-25  Don Breazeal  <donb@codesourcery.com>

	* gdb.texinfo (Forks): List targets that have fork debugging
	support.
	(Stop Reply Packets): List new stop reason "xfork".

gdb/gdbserver/
2015-02-25  Don Breazeal  <donb@codesourcery.com>

	* linux-low.c (handle_extended_wait): Implement return value,
	handle PTRACE_EVENT_FORK.
	(linux_low_enable_events): New function.
	(linux_low_filter_event): Use return value from
	handle_extended_wait.
	(extended_event_reported): New function.
	(linux_wait_1): Call extended_event_reported.
	(linux_write_memory): Add pid to debug message.
	(reset_lwp_ptrace_options_callback): New function.
	(linux_handle_new_gdb_connection): New function.
	(linux_target_ops): Initialize new structure member.
	(initialize_low): Call linux_ptrace_set_additional_flags.
	* linux-low.h (struct lwp_info) <waitstatus>: New member.
	* lynx-low.c: Initialize new structure member.
	* remote-utils.c (prepare_resume_reply): Implement stop reason
	"xfork" for "T" stop message.
	* server.h (report_fork_events): Declare global flag.
	* target.h (struct target_ops) <handle_new_gdb_connection>:
	New member.
	* win32-low.c: Initialize new structure member.

gdb/
2015-02-25  Don Breazeal  <donb@codesourcery.com>

	* NEWS: Announce follow fork support and associated RSP
	addition.
	* nat/linux-ptrace.c (supported_additional_flags): New
	static variable.
	(linux_check_ptrace_features): Use supported_additional_flags.
	(linux_test_for_tracesysgood): Likewise.
	(linux_test_for_tracefork): Likewise.
	(linux_reset_ptrace_options): New function.
	(linux_ptrace_clear_flags): New function.
	* nat/linux-ptrace.h: (linux_ptrace_clear_flags): Declare new
	function.
	* remote.c (remote_fork_event_p): New function.
	(remote_detach_1): Add target_ops argument, don't mourn inferior
	if doing detach-on-fork.
	(remote_detach): Add target_ops argument, pass to remote_detach_1.
	(extended_remote_detach): Add target_ops argument, pass to
	remote_detach_1.
	(remote_follow_fork): New function.
	(remote_parse_stop_reply): Handle new "T" stop reason "xfork".
	(remote_pid_to_str): Print "process" strings for pid/0/0 ptids.
	(init_extended_remote_ops): Initialize to_follow_fork.

gdb/testsuite/
2015-02-25  Don Breazeal  <donb@codesourcery.com>

	* gdb.base/multi-forks.exp (continue_to_exit_bp_loc):
	Use '-i' in expect statement to include input from forked child.
	Ignore inferior output if gdb,noinferiorio.
	* lib/gdb.exp (server_spawn_id): Declare new global
	variable.
	* lib/gdbserver-support.exp (server_spawn_id): Make
	variable global.

---
 gdb/NEWS                                |  15 ++-
 gdb/doc/gdb.texinfo                     |   9 ++
 gdb/gdbserver/linux-low.c               | 168 +++++++++++++++++++++++++++++---
 gdb/gdbserver/linux-low.h               |   4 +
 gdb/gdbserver/lynx-low.c                |   1 +
 gdb/gdbserver/remote-utils.c            |  15 ++-
 gdb/gdbserver/server.c                  |   3 +
 gdb/gdbserver/server.h                  |   1 +
 gdb/gdbserver/target.h                  |   7 ++
 gdb/gdbserver/win32-low.c               |   1 +
 gdb/linux-nat.c                         |   6 +-
 gdb/nat/linux-ptrace.c                  |  59 +++++++++--
 gdb/nat/linux-ptrace.h                  |  11 +++
 gdb/remote.c                            |  98 ++++++++++++++++---
 gdb/testsuite/gdb.base/multi-forks.exp  |  22 ++++-
 gdb/testsuite/lib/gdb.exp               |   3 +
 gdb/testsuite/lib/gdbserver-support.exp |   2 +
 17 files changed, 382 insertions(+), 43 deletions(-)

[v5,03/06] Extended-remote Linux follow fork

Commit Message

Patch