From patchwork Thu Aug 17 15:10:50 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pedro Alves X-Patchwork-Id: 22192 Received: (qmail 117665 invoked by alias); 17 Aug 2017 15:10:58 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 116951 invoked by uid 89); 17 Aug 2017 15:10:57 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RP_MATCHES_RCVD, SPF_HELO_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 17 Aug 2017 15:10:53 +0000 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 660C48553E; Thu, 17 Aug 2017 15:10:52 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 660C48553E Authentication-Results: ext-mx04.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx04.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=palves@redhat.com Received: from [127.0.0.1] (ovpn04.gateway.prod.ext.ams2.redhat.com [10.39.146.4]) by smtp.corp.redhat.com (Postfix) with ESMTP id 8D9D3649AA; Thu, 17 Aug 2017 15:10:51 +0000 (UTC) Subject: Re: [RFA] Remove save_inferior_ptid To: Tom Tromey , gdb-patches@sourceware.org References: <20170817024601.25136-1-tom@tromey.com> From: Pedro Alves Message-ID: <1cc87d09-8538-341b-0976-98f35a02edc0@redhat.com> Date: Thu, 17 Aug 2017 16:10:50 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <20170817024601.25136-1-tom@tromey.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 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 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 (¤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