[v3,1/4] Extended-remote follow exec

  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?

> 
> 
>> @@ -571,6 +598,50 @@ handle_extended_wait (struct lwp_info *event_lwp, int wstat)
>>        /* Report the event.  */
>>        return 0;
>>      }
>> +  else if (event == PTRACE_EVENT_EXEC && report_exec_events)
>> +    {
>> +      struct process_info *proc;
>> +      ptid_t event_ptid;
>> +      pid_t event_pid;
>> +
>> +      if (debug_threads)
>> +	{
>> +	  debug_printf ("HEW: Got exec event from LWP %ld\n",
>> +			lwpid_of (event_thr));
>> +	}
>> +
>> +      /* Get the event ptid.  */
>> +      event_ptid = event_thr->entry.id;
> 
>       event_ptid = ptid_of (event_thr);
> 

Made this change.

> 
>> +      event_pid = ptid_get_pid (event_ptid);
>> +
>> +      /* Delete the execing process and all its threads.  */
>> +      proc = get_thread_process (event_thr);
>> +      linux_mourn (proc);
>> +      current_thread = NULL;
>> +
>> +      /* Create a new process/lwp/thread.  */
>> +      proc = linux_add_process (event_pid, 0);
>> +      event_lwp = add_lwp (event_ptid);
>> +      event_thr = get_lwp_thread (event_lwp);
>> +      gdb_assert (current_thread == event_thr);
>> +      linux_arch_setup_thread (event_thr);
>> +
>> +      /*set the event status*/
> 
> Uppercase, period at end, double space.

Done.

> 
>> +      event_lwp->waitstatus.kind = TARGET_WAITKIND_EXECD;
>> +      event_lwp->waitstatus.value.execd_pathname
>> +	= xstrdup (linux_proc_pid_to_exec_file (lwpid_of (event_thr)));
>> +
>> +      /* 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;
>> +      event_thr->last_status.kind = TARGET_WAITKIND_IGNORE;
>> +
>> +      /* Report the event.  */
>> +      *orig_event_lwp = event_lwp;
>> +      return 0;
>> +    }
>>  
>>    internal_error (__FILE__, __LINE__, _("unknown ptrace event %d"), event);
>>  }
> 
> 
> 
>> @@ -1134,6 +1135,25 @@ prepare_resume_reply (char *buf, ptid_t ptid,
>>  	    buf = write_ptid (buf, status->value.related_pid);
>>  	    strcat (buf, ";");
>>  	  }
>> +	else if (status->kind == TARGET_WAITKIND_EXECD && multi_process)
>> +	  {
>> +	    enum gdb_signal signal = GDB_SIGNAL_TRAP;
>> +	    const char *event = "exec";
>> +	    char hexified_pathname[PATH_MAX*2];
> 
> Spaces around *.

Done.

> 
>> +
>> +	    sprintf (buf, "T%02x%s:", signal, event);
>> +	    buf += strlen (buf);
>> +
>> +	    /* Encode pathname to hexified format.  */
>> +	    bin2hex ((const gdb_byte *) status->value.execd_pathname,
>> +		     hexified_pathname,
>> +		     strlen (status->value.execd_pathname));
>> +
>> +	    sprintf (buf, "%s;", hexified_pathname);
>> +	    xfree (status->value.execd_pathname);
>> +	    status->value.execd_pathname = NULL;
>> +	    buf += strlen (buf);
>> +	  }
>>  	else
>>  	  sprintf (buf, "T%02x", status->value.sig);
>>  
> 
> 
> 
> 
>> @@ -619,6 +622,62 @@ get_remote_state (void)
>>    return get_remote_state_raw ();
>>  }
>>  
>> +/* Cleanup routine for the remote module's pspace data.  */
>> +
>> +static void
>> +remote_pspace_data_cleanup (struct program_space *pspace, void *arg)
>> +{
>> +  char *remote_exec_file = arg;
>> +
>> +  if (remote_exec_file != NULL)
>> +    xfree (remote_exec_file);
> 
> No need to check for NULL before calling xfree.

Done.

> 
>> +}
>> +
>> +/* Fetch the remote exec-file from the current program space.  */
>> +
>> +static char *
> 
> Can this be const char * ?
> 
>> +get_remote_exec_file (void)
>> +{
>> +  char *remote_exec_file;
>> +
>> +  remote_exec_file = program_space_data (current_program_space,
>> +					 remote_pspace_data);
> 
> How about adding:
> 
>   if (remote_exec_file == NULL)
>     return "";
> 
>> +  return remote_exec_file;
> 
> avoiding callers having to do it.
> 

Done.

>> +}
>> +
>> +/* Set the remote exec file for the current program space.  */
>> +
>> +static void
>> +set_remote_exec_file_1 (char *remote_exec_file)
>> +{
>> +  char *old_file = get_remote_exec_file ();
>> +
> 
> Here's you'd use
>      old_file = program_space_data (current_program_space,
> 			            remote_pspace_data);
> 
> directly.  It would seem super fine to me given the
> set_program_space_data just below.

Done.

> 
>> +  if (old_file != NULL)
>> +    xfree (old_file);
> 
> No need for NULL check.

Done.

> 
>> +
>> +  set_program_space_data (current_program_space, remote_pspace_data,
>> +			  xstrdup (remote_exec_file));
>> +}
>> +
>> +/* The "set/show remote exec-file" set hook.  */
>> +
>> +static void
>> +set_remote_exec_file (char *ignored, int from_tty,
>> +		      struct cmd_list_element *c)
>> +{
>> +  gdb_assert (*(char **) c->var != NULL);
>> +  set_remote_exec_file_1 (*(char **) c->var);
> 
> Use temp var please.  E.g.:
> 
>     char *file = *(char **) c->var;
> 
>     gdb_assert (file != NULL);
>     set_remote_exec_file_1 (file);
> 
> Or see another suggestion below.
> 
>> +}
>> +
>> +/* The "set/show remote exec-file" show hook.  */
>> +
>> +static void
>> +show_remote_exec_file (struct ui_file *file, int from_tty,
>> +		       struct cmd_list_element *cmd, const char *value)
>> +{
>> +  fprintf_filtered (file, "%s\n", get_remote_exec_file ());
>> +}
>> +
>>  static int
>>  compare_pnums (const void *lhs_, const void *rhs_)
>>  {
> 
> 
> 
> 
> 
>> @@ -4779,6 +4842,28 @@ remote_follow_fork (struct target_ops *ops, int follow_child,
>>    return 0;
>>  }
>>  
>> +/* Target follow-exec function for remote targets.  Save EXECD_PATHNAME
>> +   in the program space of the new inferior.  On entry and at return the
>> +   current inferior is the exec'ing inferior.  INF is the new exec'd
>> +   inferior, which may be the same as the exec'ing inferior unless
>> +   follow-exec-mode is "new".  */
>> +
>> +static void
>> +remote_follow_exec (struct target_ops *ops,
>> +		    struct inferior *inf, char *execd_pathname)
>> +{
>> +  struct cleanup *old_chain = save_current_program_space ();
>> +
>> +  /* We know that this is a target file name, so if it has the "target:"
>> +     prefix we strip it off before saving it in the program space.  */
>> +  if (is_target_filename (execd_pathname))
>> +    execd_pathname += strlen (TARGET_SYSROOT_PREFIX);
>> +
>> +  set_current_program_space (inf->pspace);
> 
> Why not pass down the pspace as parameter to set_remote_exec_file_1
> etc., avoiding this?
> 

Done.

>> +  set_remote_exec_file_1 (execd_pathname);
>> +  do_cleanups (old_chain);
>> +}
>> +
>>  /* Same as remote_detach, but don't send the "D" packet; just disconnect.  */
>>  
>>  static void
>> @@ -5977,6 +6062,7 @@ remote_parse_stop_reply (char *buf, struct stop_reply *event)
>>    struct remote_arch_state *rsa = get_remote_arch_state ();
>>    ULONGEST addr;
>>    char *p;
>> +  int skipregs = 0;
>>  
>>    event->ptid = null_ptid;
>>    event->rs = get_remote_state ();
> 
> 
> 
> 
>> @@ -13340,12 +13479,21 @@ Transfer files to and from the remote target system."),
>>  	   _("Delete a remote file."),
>>  	   &remote_cmdlist);
>>  
>> -  remote_exec_file = xstrdup ("");
>> -  add_setshow_string_noescape_cmd ("exec-file", class_files,
>> -				   &remote_exec_file, _("\
>> +    {
>> +      /* Pass a NULL (by reference) as the 'var' argument, since we do
>> +	 not have a single variable in which to store the value.  The
>> +	 value is set per-inferior, stored in the program space.  */
>> +      char *nullptr = NULL;
> 
> Passing the address of a local variable can't be good.  In:
> 
>> +static void
>> +set_remote_exec_file (char *ignored, int from_tty,
>> +		      struct cmd_list_element *c)
>> +{
>> +  gdb_assert (*(char **) c->var != NULL);
>> +  set_remote_exec_file_1 (*(char **) c->var);
> 
> c->var points to nullptr.  So this set command ends up poking
> memory to something random on the stack.

Oh, ugh.  :-P

> 
> I'd prefer leaving the old command variable global, but rename it
> remote_exec_file_var, like:
> 
> /* The variable registered as control variable the set/show
>    remote exec-file commands.  Necessary because ... */
> static char *remote_exec_file_var = "";
> 
> There's some precedent for that, in e.g., record_insn_history_size_setshow_var
> and history_size_setshow_var.
> 
> Note that that way, referencing remote_exec_file_var directly in
> set_remote_exec_file avoids the casts.

I've made these changes.

> 
>> +
>> +      add_setshow_string_noescape_cmd ("exec-file", class_files,
>> +				       &nullptr, _("\
>>  Set the remote pathname for \"run\""), _("\
>> -Show the remote pathname for \"run\""), NULL, NULL, NULL,
>> -				   &remote_set_cmdlist, &remote_show_cmdlist);
>> +Show the remote pathname for \"run\""), NULL,
>> +				       set_remote_exec_file,
>> +				       show_remote_exec_file,
>> +				       &remote_set_cmdlist,
>> +				       &remote_show_cmdlist);
>> +    }

Thanks for the review, Pedro.  An updated patch follows below.

I tested the changes by running the exec-related tests on Linux x86_64
with extended-remote.

How does this look?
--Don

---
 gdb/gdbserver/linux-low.c    | 147
++++++++++++++++++++++++++++++++++++-------
 gdb/gdbserver/lynx-low.c     |   1 +
 gdb/gdbserver/remote-utils.c |  20 ++++++
 gdb/gdbserver/server.c       |  11 ++++
 gdb/gdbserver/server.h       |   1 +
 gdb/gdbserver/target.h       |   7 +++
 gdb/gdbserver/win32-low.c    |   1 +
 gdb/infrun.c                 |  17 +++--
 gdb/nat/linux-ptrace.c       |  11 ++++
 gdb/nat/linux-ptrace.h       |   1 +
 gdb/remote.c                 | 147
++++++++++++++++++++++++++++++++++++++++---
 gdb/target-debug.h           |   2 +
 gdb/target-delegates.c       |  30 +++++++++
 gdb/target.c                 |   8 +++
 gdb/target.h                 |   7 +++
 15 files changed, 370 insertions(+), 41 deletions(-)

    catchpoint for such events.  They return  0 for success, 1 if the

[v3,1/4] Extended-remote follow exec

Commit Message

Comments

Patch