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

[Pedro Alves <palves at redhat dot com>]

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?


> @@ -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);


> +      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.

> +      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 *.

> +
> +	    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.

> +}
> +
> +/* 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.

> +}
> +
> +/* 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.

> +  if (old_file != NULL)
> +    xfree (old_file);

No need for NULL check.

> +
> +  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?

> +  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.

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.

> +
> +      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,
Pedro Alves