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

["Breazeal, Don" <donb at codesourcery dot com>]

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