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

Breazeal, Don
2014-11-13T18:53:17Z

On 11/13/2014 5:00 AM, Pedro Alves wrote:
> 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.

Thanks for the review.
>
> 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.

Yes, I assume you saw the check in remote_parse_stop_reply next to the
checks for "core" and "awatch".

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

I ran into the same thing with "exec". Maybe these stop reason strings
should all have a common prefix, like "stop_fork" and "stop_core".

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

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?

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

To goal was to make the messages printed in the remote case match the
messages printed in the native case by printing "Process" ID info
instead of "Thread" ID info given a process-style ptid (pid, 0, 0).
It was not entirely successful, but if I recall correctly it was
sufficient for matching test results.

BTW, sorry for the extra noise on the list with my duplicated
response to your comments on patch #4. I have had some
mail server issues today, and mistakenly thought the first
copy had not gone out.

The changes requested here will probably take me a little while to
implement and test.
Thanks
--Don