This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH v4 3/7 v3] Extended-remote Linux follow fork
- From: Pedro Alves <palves at redhat dot com>
- To: Don Breazeal <donb at codesourcery dot com>, gdb-patches at sourceware dot org
- Date: Tue, 10 Feb 2015 16:36:25 +0000
- Subject: Re: [PATCH v4 3/7 v3] Extended-remote Linux follow fork
- Authentication-results: sourceware.org; auth=none
- References: <54C236B9 dot 8060200 at redhat dot com> <1422222420-25421-1-git-send-email-donb at codesourcery dot com> <1422222420-25421-4-git-send-email-donb at codesourcery dot com>
On 01/25/2015 09:46 PM, Don Breazeal wrote:
> Pedro Alves wrote:
>> Let's try to shake out the higher-level concepts first before
>> focusing on specific details.
> ---snip---
>>>
>>> * 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).
>>>
>>> - extended_remote target routine extended_remote_follow_fork
>>>
>>> - RSP packet vFollowFork
>>
>> Reading through this, I don't think this is the model we should be exposing
>> at the RSP level, and requiring servers to support. The hiding of the
>> child fork until the users resumes is a current detail that we may want to
>> change in the future. It seems better to me to _not_ hide the child from
>> GDB, and then implement the hide-child-until-resume detail in GDB. That is,
>> I think we should model fork events at the RSP level similarly to
>> how ptrace and ttrace expose them. So, e.g., I think switching to the
>> child to write to its memory should be done with the regular Hg packet.
>> Handling detach_fork would be done by GDB calling the regular
>> detach packet (D;PID), etc.
>
> I have updated this patch to eliminate the vFollowFork packet and to handle
> detach_fork using a regular detach packet as you describe above.
>
>>> 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.
>
> 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.
>
>> I'm not even seeing a fundamental need
>> to keep this for "extended-remote" alone, given gdb nowadays supports
>> the multiprocess extensions with "target remote" too.
>
> I agree that we should do this. I've implemented just extended-remote
> support in this patch, then added 'target remote' support in the
> patch immediately after this one in the new version of the series.
>
> Updated patch and commit message 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.
This may need to be adjusted a bit when rebased on mainline.
handle_extended_wait now never resumes any LWP.
>
> * 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.
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.
>
> * 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/gdbserver/
> 2015-01-25 Don Breazeal <donb@codesourcery.com>
>
> * gdbthread.h (struct thread_info) <pending_follow>: New member.
> * 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.
> (initialize_low): Pass PTRACE_O_TRACEFORK to set_additional_flags.
> * remote-utils.c (prepare_resume_reply): Implement stop reason
> "fork" for "T" stop message.
> * server.h (report_fork_events): Declare global flag.
>
> gdb/
> 2015-01-25 Don Breazeal <donb@codesourcery.com>
>
> * nat/linux-ptrace.c (linux_ptrace_clear_additional_flags): New
> function.
> * nat/linux-ptrace.h: (linux_ptrace_clear_additional_flags):
> Declare new function.
> * remote.c (remote_follow_fork): 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_parse_stop_reply): Handle new "T" stop reason "fork".
> (remote_pid_to_str): Print "process" strings for pid/0/0 ptids.
> (init_extended_remote_ops): Initialize to_follow_fork.
>
> gdb/testsuite/
> 2015-01-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.
> * lib/gdb.exp (server_spawn_id): Declare new global
> variable.
> * lib/gdbserver-support.exp (server_spawn_id): Make
> variable global.
>
> ---
> gdb/gdbserver/gdbthread.h | 4 +
> gdb/gdbserver/linux-low.c | 129 +++++++++++++++++++++++++++---
> gdb/gdbserver/remote-utils.c | 14 +++-
> gdb/gdbserver/server.h | 1 +
> gdb/nat/linux-ptrace.c | 10 +++
> gdb/nat/linux-ptrace.h | 1 +
> gdb/remote.c | 89 +++++++++++++++++++---
> gdb/testsuite/gdb.base/multi-forks.exp | 15 ++++-
> gdb/testsuite/lib/gdb.exp | 2 +
> gdb/testsuite/lib/gdbserver-support.exp | 2 +-
> 10 files changed, 241 insertions(+), 26 deletions(-)
>
> 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).
> /* Given `while-stepping', a thread may be collecting data for more
> than one tracepoint simultaneously. E.g.:
>
> diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
> index e31844c..9fda25d 100644
> --- a/gdb/gdbserver/linux-low.c
> +++ b/gdb/gdbserver/linux-low.c
> @@ -20,6 +20,7 @@
> #include "linux-low.h"
> #include "nat/linux-osdata.h"
> #include "agent.h"
> +#include "tdesc.h"
>
> #include "nat/linux-nat.h"
> #include "nat/linux-waitpid.h"
> @@ -370,22 +371,23 @@ linux_add_process (int pid, int attached)
> static CORE_ADDR get_pc (struct lwp_info *lwp);
>
> /* Handle a GNU/Linux extended wait response. If we see a clone
> - event, we need to add the new LWP to our list (and not report the
> - trap to higher layers). */
> + event, we need to add the new LWP to our list (and return 0 so as
> + not to report the trap to higher layers). */
>
> -static void
> +static int
> handle_extended_wait (struct lwp_info *event_child, int wstat)
> {
> int event = linux_ptrace_get_extended_event (wstat);
> struct thread_info *event_thr = get_lwp_thread (event_child);
> struct lwp_info *new_lwp;
>
> - if (event == PTRACE_EVENT_CLONE)
> + if ((event == PTRACE_EVENT_FORK) || (event == PTRACE_EVENT_CLONE))
> {
> ptid_t ptid;
> unsigned long new_pid;
> int ret, status;
>
> + /* Get the pid of the new lwp. */
> ptrace (PTRACE_GETEVENTMSG, lwpid_of (event_thr), (PTRACE_TYPE_ARG3) 0,
> &new_pid);
>
> @@ -405,6 +407,52 @@ handle_extended_wait (struct lwp_info *event_child, int wstat)
> warning ("wait returned unexpected status 0x%x", status);
> }
>
> + if (event == PTRACE_EVENT_FORK)
> + {
> + struct process_info *parent_proc;
> + struct process_info *child_proc;
> + struct lwp_info *child_lwp;
> + struct target_desc *tdesc;
> +
> + ptid = ptid_build (new_pid, new_pid, 0);
> +
> + if (debug_threads)
> + {
> + debug_printf ("HEW: Got fork event from LWP %ld, "
> + "new child is %d\n",
> + ptid_get_lwp (ptid_of (event_thr)),
> + ptid_get_pid (ptid));
> + }
> +
> + /* Add the new process to the tables and clone the breakpoint
> + lists of the parent. We need to do this even if the new process
> + will be detached, since we will need the process object and the
> + breakpoints to remove any breakpoints from memory when we
> + detach, and the host side will access registers. */
> + child_proc = linux_add_process (new_pid, 0);
> + gdb_assert (child_proc != NULL);
> + child_lwp = add_lwp (ptid);
> + gdb_assert (child_lwp != NULL);
> + child_lwp->stopped = 1;
> + parent_proc = get_thread_process (event_thr);
> + child_proc->attached = parent_proc->attached;
> + clone_all_breakpoints (&child_proc->breakpoints,
> + &child_proc->raw_breakpoints,
> + parent_proc->breakpoints);
> +
> + tdesc = xmalloc (sizeof (struct target_desc));
> + copy_target_description (tdesc, parent_proc->tdesc);
> + child_proc->tdesc = tdesc;
> + child_lwp->must_set_ptrace_flags = 1;
> +
> + /* Save fork info in the parent thread. */
> + event_thr->pending_follow.kind = TARGET_WAITKIND_FORKED;
> + event_thr->pending_follow.value.related_pid = ptid;
> +
> + /* Report the event. */
> + return 0;
> + }
> +
> if (debug_threads)
> debug_printf ("HEW: Got clone event "
> "from LWP %ld, new child is LWP %ld\n",
> @@ -451,7 +499,12 @@ handle_extended_wait (struct lwp_info *event_child, int wstat)
> threads, it will have a pending SIGSTOP; we may as well
> collect it now. */
> linux_resume_one_lwp (event_child, event_child->stepping, 0, NULL);
> +
> + /* Don't report the event. */
> + return 1;
> }
> +
> + internal_error (__FILE__, __LINE__, _("unknown ptrace event %d"), event);
> }
>
> /* Return the PC as read from the regcache of LWP, without any
> @@ -1810,6 +1863,20 @@ check_stopped_by_watchpoint (struct lwp_info *child)
> return child->stop_reason == LWP_STOPPED_BY_WATCHPOINT;
> }
>
> +/* Wrapper for linux_enable_event_reporting that supports disabling
> + supported events if we have determined we don't want to report
> + them. This will not be needed once follow fork is implemented
> + for target remote as well as extended-remote. */
> +
> +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.
Alternatively, always enable the events at the ptrace
level, and then filter them out if the connected
GDB doesn't support them.
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.
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.
(Even with that, I think it'd be good to keep GDB / GDBserver
broadcasting the support in qSupported.)
> +
> + linux_enable_event_reporting (pid, attached);
> +}
> +
> /* Do low-level handling of the event, and check if we should go on
> and pass it to caller code. Return the affected lwp if we are, or
> NULL otherwise. */
> @@ -1898,7 +1965,7 @@ linux_low_filter_event (int lwpid, int wstat)
> {
> struct process_info *proc = find_process_pid (pid_of (thread));
>
> - linux_enable_event_reporting (lwpid, proc->attached);
> + linux_low_enable_events (lwpid, proc->attached);
> child->must_set_ptrace_flags = 0;
> }
>
> @@ -1908,8 +1975,14 @@ linux_low_filter_event (int lwpid, int wstat)
> && linux_is_extended_waitstatus (wstat))
> {
> child->stop_pc = get_pc (child);
> - handle_extended_wait (child, wstat);
> - return NULL;
> + if (handle_extended_wait (child, wstat))
> + return NULL;
> + else
> + {
> + child->status_pending_p = 1;
> + child->status_pending = wstat;
> + return child;
> + }
> }
>
> if (WIFSTOPPED (wstat) && WSTOPSIG (wstat) == SIGTRAP
> @@ -2444,6 +2517,18 @@ ignore_event (struct target_waitstatus *ourstatus)
> return null_ptid;
> }
>
> +/* 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?
> +
> +static int
> +extended_event_reported (const struct target_waitstatus *waitstatus)
> +{
> + if (waitstatus == NULL)
> + return 0;
> +
> + return (waitstatus->kind == TARGET_WAITKIND_FORKED);
> +}
> +
> /* Wait for process, returns status. */
>
> static ptid_t
> @@ -2778,7 +2863,8 @@ linux_wait_1 (ptid_t ptid,
> && !bp_explains_trap && !trace_event)
> || (gdb_breakpoint_here (event_child->stop_pc)
> && gdb_condition_true_at_breakpoint (event_child->stop_pc)
> - && gdb_no_commands_at_breakpoint (event_child->stop_pc)));
> + && gdb_no_commands_at_breakpoint (event_child->stop_pc))
> + || extended_event_reported (¤t_thread->pending_follow));
>
> run_breakpoint_commands (event_child->stop_pc);
>
> @@ -2800,6 +2886,13 @@ linux_wait_1 (ptid_t ptid,
> paddress (event_child->stop_pc),
> paddress (event_child->step_range_start),
> paddress (event_child->step_range_end));
> + if (extended_event_reported (¤t_thread->pending_follow))
> + {
> + char *str = target_waitstatus_to_string (ourstatus);
> + debug_printf ("LWP %ld: extended event with waitstatus %s\n",
> + lwpid_of (get_lwp_thread (event_child)), str);
> + xfree (str);
> + }
> }
>
> /* We're not reporting this breakpoint to GDB, so apply the
> @@ -2909,7 +3002,17 @@ linux_wait_1 (ptid_t ptid,
> unstop_all_lwps (1, event_child);
> }
>
> - ourstatus->kind = TARGET_WAITKIND_STOPPED;
> + if (extended_event_reported (¤t_thread->pending_follow))
> + {
> + /* If the reported event is a fork, vfork or exec, let GDB know. */
> + ourstatus->kind = current_thread->pending_follow.kind;
> + ourstatus->value = current_thread->pending_follow.value;
> +
> + /* Reset the event lwp's waitstatus since we handled it already. */
> + current_thread->pending_follow.kind = TARGET_WAITKIND_SPURIOUS;
> + }
> + else
> + ourstatus->kind = TARGET_WAITKIND_STOPPED;
>
> /* Now that we've selected our final event LWP, un-adjust its PC if
> it was a software breakpoint. */
> @@ -2940,7 +3043,7 @@ linux_wait_1 (ptid_t ptid,
> but, it stopped for other reasons. */
> ourstatus->value.sig = gdb_signal_from_host (WSTOPSIG (w));
> }
> - else
> + else if (ourstatus->kind == TARGET_WAITKIND_STOPPED)
> {
> ourstatus->value.sig = gdb_signal_from_host (WSTOPSIG (w));
> }
> @@ -4738,8 +4841,8 @@ linux_write_memory (CORE_ADDR memaddr, const unsigned char *myaddr, int len)
> val = val & 0xffff;
> else if (len == 3)
> val = val & 0xffffff;
> - debug_printf ("Writing %0*x to 0x%08lx\n", 2 * ((len < 4) ? len : 4),
> - val, (long)memaddr);
> + debug_printf ("Writing %0*x to 0x%08lx in process %d\n",
> + 2 * ((len < 4) ? len : 4), val, (long)memaddr, pid);
> }
>
> /* Fill start and end extra bytes of buffer with existing memory data. */
> @@ -6128,6 +6231,6 @@ initialize_low (void)
> initialize_low_arch ();
>
> /* Enable any extended ptrace events that are supported. */
> - linux_ptrace_set_additional_flags (0);
> + linux_ptrace_set_additional_flags (PTRACE_O_TRACEFORK);
> linux_test_for_event_reporting ();
> }
> diff --git a/gdb/gdbserver/remote-utils.c b/gdb/gdbserver/remote-utils.c
> index 8854c4c..f0cc882 100644
> --- a/gdb/gdbserver/remote-utils.c
> +++ 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?
> + {
> + enum gdb_signal signal = GDB_SIGNAL_TRAP;
> + const char *event = "fork";
> +
> + 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?
> + }
> + else
> + sprintf (buf, "T%02x", status->value.sig);
> +
> buf += strlen (buf);
>
> saved_thread = current_thread;
> diff --git a/gdb/gdbserver/server.h b/gdb/gdbserver/server.h
> index dbf31d5..ce6ffb9 100644
> --- a/gdb/gdbserver/server.h
> +++ b/gdb/gdbserver/server.h
> @@ -84,6 +84,7 @@ extern int disable_packet_qfThreadInfo;
>
> extern int run_once;
> extern int multi_process;
> +extern int report_fork_events;
> extern int non_stop;
>
> extern int disable_randomization;
> diff --git a/gdb/nat/linux-ptrace.c b/gdb/nat/linux-ptrace.c
> index 3e1fcd5..dcdf89e 100644
> --- a/gdb/nat/linux-ptrace.c
> +++ b/gdb/nat/linux-ptrace.c
> @@ -607,6 +607,16 @@ linux_ptrace_set_additional_flags (int flags)
> additional_flags = flags;
> }
>
> +/* Clear FLAGS in current_ptrace_options. This will not be needed once
> + follow fork et al are supported by target remote as well as extended-
> + remote. */
> +
> +void
> +linux_ptrace_clear_additional_flags (int flags)
> +{
> + current_ptrace_options &= ~flags;
> +}
> +
> /* Extract extended ptrace event from wait status. */
>
> int
> diff --git a/gdb/nat/linux-ptrace.h b/gdb/nat/linux-ptrace.h
> index d75b37c..830372e 100644
> --- a/gdb/nat/linux-ptrace.h
> +++ b/gdb/nat/linux-ptrace.h
> @@ -106,6 +106,7 @@ extern int linux_supports_traceclone (void);
> extern int linux_supports_tracevforkdone (void);
> extern int linux_supports_tracesysgood (void);
> extern void linux_ptrace_set_additional_flags (int);
> +extern void linux_ptrace_clear_additional_flags (int flags);
> extern int linux_ptrace_get_extended_event (int wstat);
> extern int linux_is_extended_waitstatus (int wstat);
>
> diff --git a/gdb/remote.c b/gdb/remote.c
> index ec1f1d2..eb540d9 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -24,6 +24,7 @@
> #include <fcntl.h>
> #include "inferior.h"
> #include "infrun.h"
> +#include "inf-child.h"
That's a layering violation. inf-child.h is the
base/prototype target for default child (native) targets.
> #include "bfd.h"
> #include "symfile.h"
> #include "target.h"
> @@ -1438,7 +1439,6 @@ remote_multi_process_p (struct remote_state *rs)
> return packet_support (PACKET_multiprocess_feature) == PACKET_ENABLE;
> }
>
> -#if PTRACE_FORK_EVENTS
> /* Returns true if fork events are supported. */
>
> static int
> @@ -1447,6 +1447,7 @@ remote_fork_event_p (struct remote_state *rs)
> return packet_support (PACKET_fork_event_feature) == PACKET_ENABLE;
> }
>
> +#if PTRACE_FORK_EVENTS
> /* Returns true if vfork events are supported. */
>
> static int
> @@ -1456,6 +1457,53 @@ remote_vfork_event_p (struct remote_state *rs)
> }
> #endif
>
> +/* Target follow-fork function for remote targets. On entry, and
> + at return, the current inferior is the fork parent.
> +
> + Note that although this is currently only used for extended-remote,
> + it is named remote_follow_fork in anticipation of using it for the
> + remote target as well. */
> +
> +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?
> + 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 ?
> +
> + 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?
> + }
> + }
> + return 0;
> +}
> +
> /* Tokens for use by the asynchronous signal handlers for SIGINT. */
> static struct async_signal_handler *async_sigint_remote_twice_token;
> static struct async_signal_handler *async_sigint_remote_token;
> @@ -4402,10 +4450,12 @@ remote_open_1 (const char *name, int from_tty,
> die when it hits one. */
>
> static void
> -remote_detach_1 (const char *args, int from_tty, int extended)
> +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?
> + }
> }
>
> static void
> remote_detach (struct target_ops *ops, const char *args, int from_tty)
> {
> - remote_detach_1 (args, from_tty, 0);
> + remote_detach_1 (ops, args, from_tty, 0);
> }
>
> static void
> extended_remote_detach (struct target_ops *ops, const char *args, int from_tty)
> {
> - remote_detach_1 (args, from_tty, 1);
> + remote_detach_1 (ops, args, from_tty, 1);
> }
>
> /* Same as remote_detach, but don't send the "D" packet; just disconnect. */
> @@ -5546,11 +5605,12 @@ remote_parse_stop_reply (char *buf, struct stop_reply *event)
> pnum and set p1 to point to the character following it.
> Otherwise p1 points to p. */
>
> - /* If this packet is an awatch packet, don't parse the 'a'
> - as a register number. */
> + /* If this packet's stop reason starts with a hex digit,
> + don't parse it 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);
> @@ -5602,6 +5662,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. */
> @@ -9413,8 +9478,11 @@ 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));
> + 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));
> @@ -11662,6 +11730,7 @@ Specify the serial device it is connected to (e.g. /dev/ttya).";
> 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;
> }
>
> static int
> 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.
> while { ($seen_done < 16 || ! $seen_prompt) && ! $seen_timeout } {
> # We don't know what order the interesting things will arrive in.
> # Using a pattern of the form 'x|y|z' instead of -re x ... -re y
> @@ -70,7 +82,8 @@ proc continue_to_exit_bp_loc {} {
> # first in the script that occurs anywhere in the input, so that
> # we don't skip anything.
> gdb_expect {
> - -re "($decimal done)|(Breakpoint)|($gdb_prompt)" {
> + -i "$current_spawn_id $server_spawn_id" \
> + -re "($decimal done)|(Breakpoint)|($gdb_prompt)" {
> if {[info exists expect_out(1,string)]} {
> incr seen_done
> } elseif {[info exists expect_out(2,string)]} {
> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
> index d3a3350..4d7557c 100644
> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
> @@ -29,6 +29,8 @@ load_lib libgloss.exp
> load_lib cache.exp
> load_lib gdb-utils.exp
>
> +global server_spawn_id
This should have a describing comment.
> +
> global GDB
>
> if [info exists TOOL_EXECUTABLE] {
> diff --git a/gdb/testsuite/lib/gdbserver-support.exp b/gdb/testsuite/lib/gdbserver-support.exp
> index 4a59154..9238ee3 100644
> --- a/gdb/testsuite/lib/gdbserver-support.exp
> +++ 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.
> # gdb_target_cmd
> # Send gdb the "target" command
> @@ -270,6 +269,7 @@ proc gdbserver_start { options arguments } {
> append gdbserver_command " $arguments"
> }
>
> + global server_spawn_id
> set server_spawn_id [remote_spawn target $gdbserver_command]
>
> # Wait for the server to open its TCP socket, so that GDB can connect.
>
Thanks,
Pedro Alves