Re: [PATCH 06/16 v3] Extended-remote Linux follow fork

Pedro Alves
2014-11-13T13:00:25Z

On 10/31/2014 11:28 PM, Don Breazeal wrote:
> 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. Sufficient extended-remote
> functionality has been implemented here to pass gdb.base/foll-fork.exp with
> the catchpoint tests commented out.
>

Thanks.

Let's try to shake out the higher-level concepts first before
focusing on specific details.

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

"f" is an hexadecimal digit. Last time we added such a packet,
"core", we broke backwards compatibility with older GDBs... :-/
We need to careful with that.

>
> * 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'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.

Also, I don't see how this packet could work correctly with non-stop
mode. You're assuming only one thread/process has stopped for a fork.

> else
> {
> /* Silently skip unknown optional info. */
> @@ -9418,8 +9447,11 @@ remote_pid_to_str (struct target_ops *ops, ptid_t ptid)
> if (ptid_equal (magic_null_ptid, ptid))
> xsnprintf (buf, sizeof buf, "Thread ");
> 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);

Can you explain this bit?

Thanks,
Pedro Alves