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

[Pedro Alves <palves@redhat.com>]

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