Re: [RFA] Remove save_inferior_ptid

[Pedro Alves <palves at redhat dot com>]

Hi Tom.

Nice!  See below for ideas for a couple spots, but the rest
seems fine to me.

On 08/17/2017 03:46 AM, Tom Tromey wrote:

> diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
> index b9c7d1f..9f62b12 100644
> --- a/gdb/linux-nat.c
> +++ b/gdb/linux-nat.c
> @@ -477,7 +477,6 @@ linux_child_follow_fork (struct target_ops *ops, int follow_child,
>      {
>        struct lwp_info *child_lp = NULL;
>        int status = W_STOPCODE (0);
> -      struct cleanup *old_chain;
>        int has_vforked;
>        ptid_t parent_ptid, child_ptid;
>        int parent_pid, child_pid;
> @@ -490,59 +489,61 @@ linux_child_follow_fork (struct target_ops *ops, int follow_child,
>        child_pid = ptid_get_lwp (child_ptid);
>  
>        /* We're already attached to the parent, by default.  */
> -      old_chain = save_inferior_ptid ();
> -      inferior_ptid = child_ptid;
> -      child_lp = add_lwp (inferior_ptid);
> -      child_lp->stopped = 1;
> -      child_lp->last_resume_kind = resume_stop;
> -
> -      /* Detach new forked process?  */
> -      if (detach_fork)
> -	{
> -	  make_cleanup (delete_lwp_cleanup, child_lp);
> -
> -	  if (linux_nat_prepare_to_resume != NULL)
> -	    linux_nat_prepare_to_resume (child_lp);
> -
> -	  /* When debugging an inferior in an architecture that supports
> -	     hardware single stepping on a kernel without commit
> -	     6580807da14c423f0d0a708108e6df6ebc8bc83d, the vfork child
> -	     process starts with the TIF_SINGLESTEP/X86_EFLAGS_TF bits
> -	     set if the parent process had them set.
> -	     To work around this, single step the child process
> -	     once before detaching to clear the flags.  */
> -
> -	  if (!gdbarch_software_single_step_p (target_thread_architecture
> -						   (child_lp->ptid)))
> -	    {
> -	      linux_disable_event_reporting (child_pid);
> -	      if (ptrace (PTRACE_SINGLESTEP, child_pid, 0, 0) < 0)
> -		perror_with_name (_("Couldn't do single step"));
> -	      if (my_waitpid (child_pid, &status, 0) < 0)
> -		perror_with_name (_("Couldn't wait vfork process"));
> -	    }
> -
> -	  if (WIFSTOPPED (status))
> -	    {
> -	      int signo;
> -
> -	      signo = WSTOPSIG (status);
> -	      if (signo != 0
> -		  && !signal_pass_state (gdb_signal_from_host (signo)))
> -		signo = 0;
> -	      ptrace (PTRACE_DETACH, child_pid, 0, signo);
> -	    }
> +      {
> +	scoped_restore save_inferior_ptid
> +	  = make_scoped_restore (&inferior_ptid);
>  

In this case, I don't see anything in the 'if (detach_fork)' "then" branch
that needs inferior_ptid swapped.  All the functions called within
it pass an explicit ptid argument down, if I'm reading it correctly,
which suggests that the scoped_restore is only needed
here:

      else
	{
	  /* Let the thread_db layer learn about this new process.  */
	  check_for_thread_db ();
	}

Did you try that?  Patch #1 below runs regression free here.  How about
putting that in first, avoiding reindenting the big block around twice?

> @@ -1710,11 +1710,12 @@ linux_corefile_thread (struct thread_info *info,
>  
>    regcache = get_thread_arch_regcache (info->ptid, args->gdbarch);
>  
> -  old_chain = save_inferior_ptid ();
> -  inferior_ptid = info->ptid;
> -  target_fetch_registers (regcache, -1);
> -  siginfo_data = linux_get_siginfo_data (args->gdbarch, &siginfo_size);
> -  do_cleanups (old_chain);
> +  {
> +    scoped_restore save_inferior_ptid = make_scoped_restore (&inferior_ptid);
> +    inferior_ptid = info->ptid;
> +    target_fetch_registers (regcache, -1);
> +    siginfo_data = linux_get_siginfo_data (args->gdbarch, &siginfo_size);
> +  }

Switching inferior_ptid for target_fetch_registers should not be
necessary nowadays, since:

 commit 3e00d44febb8093d8dc0e6842b975afb194c4fd1
 Author:     Simon Marchi <simon.marchi@ericsson.com>
 AuthorDate: Thu Mar 23 13:37:06 2017 -0400
 
     Remove some unnecessary inferior_ptid setting/restoring when fetching/storing registers

That leaves linux_get_siginfo_data.  Since this is a local
static function, it's easy to pass the thread as argument, pushing
the inferior_ptid switching further down.  See attached patch #2.
WDYT?

The rest seems fine to me.

Thanks,
Pedro Alves

>From 423cf1c79f06413e0b7576d9fa9e895782fd08e6 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Thu, 17 Aug 2017 15:27:42 +0100
Subject: [PATCH 1/2] linux_child_follow_fork

---
 gdb/linux-nat.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index b9c7d1f..cf56395 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -477,7 +477,6 @@ linux_child_follow_fork (struct target_ops *ops, int follow_child,
     {
       struct lwp_info *child_lp = NULL;
       int status = W_STOPCODE (0);
-      struct cleanup *old_chain;
       int has_vforked;
       ptid_t parent_ptid, child_ptid;
       int parent_pid, child_pid;
@@ -490,16 +489,15 @@ linux_child_follow_fork (struct target_ops *ops, int follow_child,
       child_pid = ptid_get_lwp (child_ptid);
 
       /* We're already attached to the parent, by default.  */
-      old_chain = save_inferior_ptid ();
-      inferior_ptid = child_ptid;
-      child_lp = add_lwp (inferior_ptid);
+      child_lp = add_lwp (child_ptid);
       child_lp->stopped = 1;
       child_lp->last_resume_kind = resume_stop;
 
       /* Detach new forked process?  */
       if (detach_fork)
 	{
-	  make_cleanup (delete_lwp_cleanup, child_lp);
+	  struct cleanup *old_chain
+	    = make_cleanup (delete_lwp_cleanup, child_lp);
 
 	  if (linux_nat_prepare_to_resume != NULL)
 	    linux_nat_prepare_to_resume (child_lp);
@@ -533,16 +531,19 @@ linux_child_follow_fork (struct target_ops *ops, int follow_child,
 	      ptrace (PTRACE_DETACH, child_pid, 0, signo);
 	    }
 
-	  /* Resets value of inferior_ptid to parent ptid.  */
 	  do_cleanups (old_chain);
 	}
       else
 	{
+	  struct cleanup *old_chain = save_inferior_ptid ();
+
+	  inferior_ptid = child_ptid;
+
 	  /* Let the thread_db layer learn about this new process.  */
 	  check_for_thread_db ();
-	}
 
-      do_cleanups (old_chain);
+	  do_cleanups (old_chain);
+	}
 
       if (has_vforked)
 	{
-- 
2.5.5

>From 28b97095a57ddbc567152130b27ce070bd7b0ed1 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Thu, 17 Aug 2017 15:28:08 +0100
Subject: [PATCH 2/2] linux_corefile_thread

---
 gdb/linux-tdep.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
index 5c7f8a0..b29b9f2 100644
--- a/gdb/linux-tdep.c
+++ b/gdb/linux-tdep.c
@@ -1649,14 +1649,15 @@ linux_collect_thread_registers (const struct regcache *regcache,
   return data.note_data;
 }
 
-/* Fetch the siginfo data for the current thread, if it exists.  If
+/* Fetch the siginfo data for the specified thread, if it exists.  If
    there is no data, or we could not read it, return NULL.  Otherwise,
    return a newly malloc'd buffer holding the data and fill in *SIZE
    with the size of the data.  The caller is responsible for freeing
    the data.  */
 
 static gdb_byte *
-linux_get_siginfo_data (struct gdbarch *gdbarch, LONGEST *size)
+linux_get_siginfo_data (thread_info *thread, struct gdbarch *gdbarch,
+			LONGEST *size)
 {
   struct type *siginfo_type;
   gdb_byte *buf;
@@ -1671,6 +1672,9 @@ linux_get_siginfo_data (struct gdbarch *gdbarch, LONGEST *size)
   buf = (gdb_byte *) xmalloc (TYPE_LENGTH (siginfo_type));
   cleanups = make_cleanup (xfree, buf);
 
+  scoped_restore save_inferior_ptid = make_scoped_restore (&inferior_ptid);
+  inferior_ptid = thread->ptid;
+
   bytes_read = target_read (&current_target, TARGET_OBJECT_SIGNAL_INFO, NULL,
 			    buf, 0, TYPE_LENGTH (siginfo_type));
   if (bytes_read == TYPE_LENGTH (siginfo_type))
@@ -1703,20 +1707,16 @@ static void
 linux_corefile_thread (struct thread_info *info,
 		       struct linux_corefile_thread_data *args)
 {
-  struct cleanup *old_chain;
   struct regcache *regcache;
   gdb_byte *siginfo_data;
   LONGEST siginfo_size = 0;
 
   regcache = get_thread_arch_regcache (info->ptid, args->gdbarch);
 
-  old_chain = save_inferior_ptid ();
-  inferior_ptid = info->ptid;
   target_fetch_registers (regcache, -1);
-  siginfo_data = linux_get_siginfo_data (args->gdbarch, &siginfo_size);
-  do_cleanups (old_chain);
+  siginfo_data = linux_get_siginfo_data (info, args->gdbarch, &siginfo_size);
 
-  old_chain = make_cleanup (xfree, siginfo_data);
+  struct cleanup *old_chain = make_cleanup (xfree, siginfo_data);
 
   args->note_data = linux_collect_thread_registers
     (regcache, info->ptid, args->obfd, args->note_data,
-- 
2.5.5