[RFA] Remove save_inferior_ptid
Commit Message
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 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(-)
@@ -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 (¤t_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