Re: [PATCH v7 5/7] Remote follow vfork

[Pedro Alves <palves at redhat dot com>]

On 04/10/2015 06:09 PM, Don Breazeal wrote:
> Hi Pedro,
> 
> This version of the patch incorporates changes based on your comments on
> the previous version, as outlined below.
> 
> On 3/24/2015 5:28 AM, Pedro Alves wrote:
>> On 03/17/2015 08:56 PM, Don Breazeal wrote:
>>
>>> diff --git a/gdb/gdbserver/remote-utils.c b/gdb/gdbserver/remote-utils.c
>>> index dc43e38..42c3ec5 100644
>>> --- a/gdb/gdbserver/remote-utils.c
>>> +++ b/gdb/gdbserver/remote-utils.c
>>> @@ -1115,15 +1115,19 @@ prepare_resume_reply (char *buf, ptid_t ptid,
>>>      {
>>>      case TARGET_WAITKIND_STOPPED:
>>>      case TARGET_WAITKIND_FORKED:
>>> +    case TARGET_WAITKIND_VFORKED:
>>>        {
>>>  	struct thread_info *saved_thread;
>>>  	const char **regp;
>>>  	struct regcache *regcache;
>>>  
>>> -	if (status->kind == TARGET_WAITKIND_FORKED && report_fork_events)
>>> +	if ((status->kind == TARGET_WAITKIND_FORKED && report_fork_events)
>>> +	     || (status->kind == TARGET_WAITKIND_VFORKED
>>> +		 && report_vfork_events))
>>>  	  {
>>>  	    enum gdb_signal signal = GDB_SIGNAL_TRAP;
>>> -	    const char *event = "xfork";
>>> +	    const char *event = (status->kind == TARGET_WAITKIND_FORKED
>>> +				 ? "xfork" : "vfork");
>>>  
>>>  	    sprintf (buf, "T%02x%s:", signal, event);
>>>  	    buf += strlen (buf);
>>> @@ -1245,6 +1249,15 @@ prepare_resume_reply (char *buf, ptid_t ptid,
>>>        else
>>>  	sprintf (buf, "X%02x", status->value.sig);
>>>        break;
>>> +    case TARGET_WAITKIND_VFORK_DONE:
>>> +      if (multi_process)
>>> +	{
>>> +	  enum gdb_signal signal = GDB_SIGNAL_TRAP;
>>> +	  const char *event = "vforkdone";
>>> +
>>
>> Should only include vforkdone if report_vfork_events is true, I'd think.
> 
> If we get one of these events (fork, vfork, vforkdone) and report_xxx_events
> is not set, then it is a bug, so I put those flags into asserts for all
> three events to ensure proper behavior.

I don't think that's a good idea.  For instance, what if
a thread/lwp has already stopped for VFORK_DONE but the event is left
pending to report to the gdb.  Meanwhile, gdb disconnects before the
event is sent.  Now a new gdb reconnects, without support for vfork-done.
See server.c:handle_status, both non-stop and all-stop.  That would
trigger that assertion.

> +	    if (status->kind == TARGET_WAITKIND_FORKED)
> +	      {
> +		gdb_assert (report_fork_events);
> +		sprintf (buf, "T%02xfork:", signal);
> +	      }
> +	    else
> +	      {
> +		gdb_assert (report_vfork_events);
> +		sprintf (buf, "T%02xvfork:", signal);

Eh, I had asked to _remove_ the 'x's, not add them.  Could you
please drop them?

Thanks,
Pedro Alves