Re: [PATCH v3 1/4] Extended-remote follow exec

Pedro Alves
2015-09-11T08:34:58Z

Hi Don,

Other than the nits below, it LGTM. Fix those and you're
good to go. Please push.

On 09/10/2015 11:56 PM, Don Breazeal wrote:
> On 9/10/2015 5:43 AM, Pedro Alves wrote:
>> On 09/10/2015 12:05 AM, Don Breazeal wrote:
>>> Hi Pedro,
>>>
>>> This is an updated version of the patch previously submitted here:
>>> https://sourceware.org/ml/gdb-patches/2015-07/msg00924.html. Changes
>>> from the previous version include:
>>>
>>> * In gdbserver, when an exec event occurs, gdbserver deletes all
>>> of the data (inferior, lwps, threads) associated with the execing
>>> process and replaces it with a new set of data.
>>>
>>> * In GDB, the remote exec-file is now stored per-inferior in the
>>> inferior's program space as a REGISTRY field.
>>>
>>> * In GDB, a new target hook, target_follow_exec, is used to enable
>>> storing the remote exec-file as per-inferior data.
>>>
>>> * In GDB, follow_exec now calls add_inferior_with_spaces for mode
>>> "new" in place of add_inferior and the calls to set up the program
>>> and address spaces.
>>>
>>> Some of the things that were part of the previous patchset were
>>> eliminated as a result of these changes, including:
>>>
>>> * Deleting "vanished" lwps in gdbserver/linux-low.c:send_sigstop.
>>>
>>> * Fiddling with the regcache and r_debug in
>>> gdbserver/linux-low.c:handle_extended_wait.
>>>
>>> * Fiddling with the inferior's architecture in
>>> remote.c:remote_parse_stop_reply.
>>>
>>> A couple of your questions about the previous version of the patch still
>>> apply, in spite of the rework. Regarding the handling of the exec event
>>> in linux-low.c:handle_extended_wait:
>>>
>>>>> + /* Mark the exec status as pending. */
>>>>> + event_lwp->stopped = 1;
>>>>> + event_lwp->status_pending_p = 1;
>>>>> + event_lwp->status_pending = wstat;
>>>>> + event_thr->last_resume_kind = resume_stop;
>>>>
>>>> Shouldn't this be resume_continue?
>>>
>>> My thinking here is that as far as gdbserver is concerned, we *do* want
>>> to use resume_stop, so that we stop and report the event to GDB. It will
>>> be up to GDB whether to continue from this point. Does that make sense?
>>
>> Not really -- putting exec events out of the picture, consider:
>>
>> If you simply continue a thread (vCont;c) and it hits a breakpoint, it'll
>> have last_resume_kind==resume_continue, and we still report the event
>> to gdb, and it's still up to GDB whether to continue past the breakpoint.
>>
>> So if you set last_resume_kind to resume_continue, and drop this hunk:
>>
>>> @@ -3373,7 +3463,8 @@ linux_wait_1 (ptid_t ptid,
>>> ourstatus->value.sig = GDB_SIGNAL_0;
>>> }
>>> else if (current_thread->last_resume_kind == resume_stop
>>> - && WSTOPSIG (w) != SIGSTOP)
>>> + && WSTOPSIG (w) != SIGSTOP
>>> + && ourstatus->kind != TARGET_WAITKIND_EXECD)
>>> {
>>> /* A thread that has been requested to stop by GDB with vCont;t,
>>> but, it stopped for other reasons. */
>>> @@ -5801,6 +5892,14 @@ linux_supports_vfork_events (void)
>>> return linux_supports_tracefork ();
>>> }
>>
>> ... what doesn't work?
>
> That works just fine. I've made that change.
>
> Clearly I didn't understand the purpose of resume_stop. Is that only
> used when GDB requests a stop, and/or when an inferior is just starting
> up or being attached? As opposed to when the inferior is stopped by an
> event?

Yeah, lots of different state flags, and several layers of state machines
involved. gdb's, core gdbserver's, and linux-low's. The flags we have
today have come into being through code evolution, rather than design...
Probably, we could probably merge/simplify them, but it'd require lots of
careful analysis.

Anyway, from a high level, thread->last_resume_kind indicates the last
resume state from _gdb_'s perspective. So resume_stop is used:

- when GDB requests an explicit stop with vCont;t. The thread gets
set to resume_stop even if it is still running.

- when a thread that was last continued/stepped (resume_continue/resume_step)
hits an event and _after_ gdbserver reports the stop to gdb, _then_ it's
last resume kind is set to resume_stop.

> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -75,6 +75,14 @@
> static char *target_buf;
> static long target_buf_size;
>
> +/* Per-program-space data key. */
> +static const struct program_space_data *remote_pspace_data;
> +
> +/* The variable registered as the control variable used by the
> + remote exec-file commands. Used by the set/show machinery
> + as the location of the remote exec-file value. */
> +static char *remote_exec_file_var;

I think we should mention the per-program-space aspect. Something like:

/* The variable registered as the control variable used by the
remote exec-file commands. While the remote exec-file setting is
per-program-space, the set/show machinery uses this as location
of the remote exec-file value. */

> +/* Fetch the remote exec-file from the current program space. */
> +
> +static const char *
> +get_remote_exec_file (void)
> +{
> + char *remote_exec_file;
> +
> + remote_exec_file = program_space_data (current_program_space,
> + remote_pspace_data);
> + if (remote_exec_file == NULL)
> + return "";
> +
> + return remote_exec_file;
> +}
> +
> +/* Set the remote exec file for the current program space. */

/* Set the remote exec file for PSPACE. */

> +
> +static void
> +set_remote_exec_file_1 (struct program_space *pspace,
> + char *remote_exec_file)

I think we can now rename this for clarity. E.g.,
set_program_space_remote_exec_file / set_pspace_remote_exec_file.

> +{
> + char *old_file = program_space_data (pspace, remote_pspace_data);
> +
> + xfree (old_file);
> + set_program_space_data (pspace, remote_pspace_data,
> + xstrdup (remote_exec_file));
> +}

Thanks,
Pedro Alves