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

Pedro Alves
2015-04-15T15:38:57Z

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