From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 7771 invoked by alias); 14 Sep 2011 10:50:51 -0000 Received: (qmail 7760 invoked by uid 22791); 14 Sep 2011 10:50:48 -0000 X-SWARE-Spam-Status: No, hits=-1.3 required=5.0 tests=AWL,BAYES_00,TW_EG X-Spam-Check-By: sourceware.org Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 14 Sep 2011 10:50:31 +0000 Received: from nat-jpt.mentorg.com ([192.94.33.2] helo=PR1-MAIL.mgc.mentorg.com) by relay1.mentorg.com with esmtp id 1R3n30-0000a5-Qw from Yao_Qi@mentor.com for gdb-patches@sourceware.org; Wed, 14 Sep 2011 03:50:31 -0700 Received: from [172.30.8.86] ([172.30.8.86]) by PR1-MAIL.mgc.mentorg.com with Microsoft SMTPSVC(6.0.3790.1830); Wed, 14 Sep 2011 19:50:28 +0900 Message-ID: <4E7086F0.8020902@codesourcery.com> Date: Wed, 14 Sep 2011 11:10:00 -0000 From: Yao Qi User-Agent: Mozilla/5.0 (X11; Linux i686; rv:6.0.2) Gecko/20110906 Thunderbird/6.0.2 MIME-Version: 1.0 To: Pedro Alves CC: gdb-patches@sourceware.org Subject: Re: [patch 1/2] Displaced stepping across fork/vfork References: <4E6C04B3.90106@codesourcery.com> <201109121536.01756.pedro@codesourcery.com> In-Reply-To: <201109121536.01756.pedro@codesourcery.com> Content-Type: multipart/mixed; boundary="------------020007010509010006030908" X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2011-09/txt/msg00251.txt.bz2 This is a multi-part message in MIME format. --------------020007010509010006030908 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Content-length: 5732 On 09/12/2011 10:36 PM, Pedro Alves wrote: > On Sunday 11 September 2011 01:45:39, Yao Qi wrote: > > Thanks. I think other TARGET_WAITKIND_... events can also leave the > PC pointing to the scratchpad. E.g., what if you do "catch syscall", > and then step over a breakpoint set at the syscall insn? > Or "catch exec", and step over the syscall insn of an exec syscall? Yeah, that is true, and I didn't realize that other TARGET_WAITKIND_xxx events should be handled as well. This patch comes from my observation that watch-vfork.exp fails on arm, and after investigation I realize that it is not a target-specific problem. So only TARGET_WAITKIND_{FORKED, VFORKED} is considered in this patch. > Do we leave stale displaced stepping state behind in the later case? > No. I hope all stale displaced stepping state of all TARGET_WAITKIND_XXX events can be fixed clearly. I go through the definition of `enum target_waitkind', and I think beside STOPPED, FORKED and VFORKED, we still have to handle EXECD and SYSCALL_ENTRY. Except these five events (FORKED, VFORKED, EXECD, STOPPED and SYSCALL_ENTRY), we don't have to worry about displaced stepping state in other events. Is it correct? I am looking at cleaning up displaced stepping state for event TARGET_WAITKIND_EXECD and TARGET_WAITKIND_SYSCALL_ENTRY, but may need some time more to investigate other related problems. Hope this patch can go in first. >> >> /* Did the instruction complete successfully? */ >> if (signal == TARGET_SIGNAL_TRAP) >> @@ -3407,6 +3414,60 @@ handle_inferior_event (struct execution_control_state *ecs) >> if (debug_infrun) >> fprintf_unfiltered (gdb_stdlog, "infrun: TARGET_WAITKIND_FORKED\n"); >> >> + /* Check inferior is in displaced stepping. */ > > /* Check whether the inferior is displaced stepping. */ > > or: > > /* Check whether the inferior is doing a displaced step. */ > > Fixed. >> + { >> + struct regcache *regcache = get_thread_regcache (ecs->ptid); >> + struct gdbarch *gdbarch = get_regcache_arch (regcache); >> + struct displaced_step_inferior_state *displaced >> + = get_displaced_stepping_state (ptid_get_pid (ecs->ptid)); >> + >> + /* If checking displaced stepping is supported, and thread >> + ecs->ptid is being in displaced stepping. */ > > The use_displaced_stepping check is redundant. When displaced stepping > isn't supported, you'll always get a NULL DISPLACED. The resulting comment > just reads what the code is doing, so I suggest just removing it. > Otherwise s/is being in displaced stepping/is displaced stepping/. > OK, use_displaced_stepping check is removed, and comment is adjusted as you suggested. I thought "displaced stepping" is a noun, and sorry about these grammar errors in comments. >> + if (use_displaced_stepping (gdbarch)&& displaced >> +&& ptid_equal (displaced->step_ptid, ecs->ptid)) >> + { >> + struct inferior *parent_inf >> + = find_inferior_pid (ptid_get_pid (ecs->ptid)); >> + struct regcache *child_regcache; >> + CORE_ADDR parent_pc; >> + >> + /* GDB has got TARGET_WAITKIND_FORKED or TARGET_WAITKIND_VFORKED, >> + indicating that the displaced stepping of syscall instruction >> + has been done. Perform cleanup for parent process here. Note >> + that this operation also clean up child process for vfork, > > also cleans up the child > Fixed. >> + because their pages are shared. */ > > Spurious double-space after because. > Removed one space. >> + displaced_step_fixup (ecs->ptid, TARGET_SIGNAL_TRAP); >> + >> + if (ecs->ws.kind == TARGET_WAITKIND_FORKED) >> + { >> + /* Restore scratch pad for child process. */ >> + displaced_step_restore (displaced, ecs->ws.value.related_pid); >> + } >> + >> + /* Since vfork/fork syscall instruction is executed in scratch pad, >> + so the PC value of child is still within the range of scratchpad. >> + Set PC of child process back to original place, that is identical >> + to father's PC value. */ > > Not the original place --- to where the PC should point after the fork syscall. > Suggest writing: > > /* Since the vfork/fork syscall instruction was executed in the scratch pad, > the child's PC is also within the scratchpad. Set the child's PC > to the parent's PC value, which has already been fixed up. */ > The past tense ("was executed") is better here. Copied your suggestion directly. :) >> + >> + child_regcache >> + = get_thread_arch_aspace_regcache (ecs->ws.value.related_pid, >> + gdbarch, >> + parent_inf->aspace); > > Ugh, IIUC, you have to use parent_inf->aspace and the parent's gdb (the child's > gdbarch should always be the same as the parent's, but still) here because the child > has not been added to the inferior list yet... Please add a FIXME comment > mentioning this. It's ugly, but better than leaving that unjustified. Something > like: > > /* FIXME: we use the parent's aspace here, although we're touching the child, > because the child hasn't been added to the inferior list yet at this > point. */ > OK. >> + /* Read PC value of father process. */ > > s/father/parent > Fixed. >> + >> +struct regcache * >> +get_thread_arch_regcache (ptid_t ptid, struct gdbarch *gdbarch) >> +{ >> + > > Spurious newline. > Fixed. -- Yao (齐尧) --------------020007010509010006030908 Content-Type: text/x-patch; name="0014-disp-step-cross-vfork.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="0014-disp-step-cross-vfork.patch" Content-length: 6994 gdb/ * infrun.c (displaced_step_fixup): Move some code ... (displaced_step_restore): ... here. New function. (handle_inferior_event): Cleanup displaced stepping state for both child and parent when get forked or vforked event. * regcache.c (get_thread_arch_aspace_regcache): New function. get_thread_arch_regcache (): Call it. --- gdb/infrun.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++------- gdb/regcache.c | 30 ++++++++++++------- gdb/regcache.h | 3 ++ 3 files changed, 95 insertions(+), 22 deletions(-) diff --git a/gdb/infrun.c b/gdb/infrun.c index 729f148..08fa314 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -1377,6 +1377,23 @@ write_memory_ptid (ptid_t ptid, CORE_ADDR memaddr, do_cleanups (ptid_cleanup); } +/* Restore the contents of the copy area for thread PTID. */ + +static void +displaced_step_restore (struct displaced_step_inferior_state *displaced, + ptid_t ptid) +{ + ULONGEST len = gdbarch_max_insn_length (displaced->step_gdbarch); + + write_memory_ptid (ptid, displaced->step_copy, + displaced->step_saved_copy, len); + if (debug_displaced) + fprintf_unfiltered (gdb_stdlog, "displaced: restored %s %s\n", + target_pid_to_str (ptid), + paddress (displaced->step_gdbarch, + displaced->step_copy)); +} + static void displaced_step_fixup (ptid_t event_ptid, enum target_signal signal) { @@ -1395,17 +1412,7 @@ displaced_step_fixup (ptid_t event_ptid, enum target_signal signal) old_cleanups = make_cleanup (displaced_step_clear_cleanup, displaced); - /* Restore the contents of the copy area. */ - { - ULONGEST len = gdbarch_max_insn_length (displaced->step_gdbarch); - - write_memory_ptid (displaced->step_ptid, displaced->step_copy, - displaced->step_saved_copy, len); - if (debug_displaced) - fprintf_unfiltered (gdb_stdlog, "displaced: restored %s\n", - paddress (displaced->step_gdbarch, - displaced->step_copy)); - } + displaced_step_restore (displaced, displaced->step_ptid); /* Did the instruction complete successfully? */ if (signal == TARGET_SIGNAL_TRAP) @@ -3407,6 +3414,61 @@ handle_inferior_event (struct execution_control_state *ecs) if (debug_infrun) fprintf_unfiltered (gdb_stdlog, "infrun: TARGET_WAITKIND_FORKED\n"); + /* Check whether the inferior is displaced stepping. */ + { + struct regcache *regcache = get_thread_regcache (ecs->ptid); + struct gdbarch *gdbarch = get_regcache_arch (regcache); + struct displaced_step_inferior_state *displaced + = get_displaced_stepping_state (ptid_get_pid (ecs->ptid)); + + /* If checking displaced stepping is supported, and thread + ecs->ptid is displaced stepping. */ + if (displaced && ptid_equal (displaced->step_ptid, ecs->ptid)) + { + struct inferior *parent_inf + = find_inferior_pid (ptid_get_pid (ecs->ptid)); + struct regcache *child_regcache; + CORE_ADDR parent_pc; + + /* GDB has got TARGET_WAITKIND_FORKED or TARGET_WAITKIND_VFORKED, + indicating that the displaced stepping of syscall instruction + has been done. Perform cleanup for parent process here. Note + that this operation also cleans up the child process for vfork, + because their pages are shared. */ + displaced_step_fixup (ecs->ptid, TARGET_SIGNAL_TRAP); + + if (ecs->ws.kind == TARGET_WAITKIND_FORKED) + { + /* Restore scratch pad for child process. */ + displaced_step_restore (displaced, ecs->ws.value.related_pid); + } + + /* Since vfork/fork syscall instruction was executed in scratchpad, + the child's PC is also within the scratchpad. Set the child's PC + to the parent's PC value, which has already been fixed up. + FIXME: we use the parent's aspace here, although we're touching + the child, because the child hasn't been added to the inferior + list yet at this point. */ + + child_regcache + = get_thread_arch_aspace_regcache (ecs->ws.value.related_pid, + gdbarch, + parent_inf->aspace); + /* Read PC value of parent process. */ + parent_pc = regcache_read_pc (regcache); + + if (debug_displaced) + fprintf_unfiltered (gdb_stdlog, + "displaced: write child pc from %s to %s\n", + paddress (gdbarch, + regcache_read_pc (child_regcache)), + paddress (gdbarch, parent_pc)); + + regcache_write_pc (child_regcache, parent_pc); + + } + } + if (!ptid_equal (ecs->ptid, inferior_ptid)) { context_switch (ecs->ptid); diff --git a/gdb/regcache.c b/gdb/regcache.c index 0af93e8..37092f8 100644 --- a/gdb/regcache.c +++ b/gdb/regcache.c @@ -450,17 +450,33 @@ struct regcache_list static struct regcache_list *current_regcache; struct regcache * -get_thread_arch_regcache (ptid_t ptid, struct gdbarch *gdbarch) +get_thread_arch_aspace_regcache (ptid_t ptid, struct gdbarch *gdbarch, + struct address_space *aspace) { struct regcache_list *list; struct regcache *new_regcache; - struct address_space *aspace; for (list = current_regcache; list; list = list->next) if (ptid_equal (list->regcache->ptid, ptid) && get_regcache_arch (list->regcache) == gdbarch) return list->regcache; + new_regcache = regcache_xmalloc_1 (gdbarch, aspace, 0); + new_regcache->ptid = ptid; + + list = xmalloc (sizeof (struct regcache_list)); + list->regcache = new_regcache; + list->next = current_regcache; + current_regcache = list; + + return new_regcache; +} + +struct regcache * +get_thread_arch_regcache (ptid_t ptid, struct gdbarch *gdbarch) +{ + struct address_space *aspace; + /* For the benefit of "maint print registers" & co when debugging an executable, allow dumping the regcache even when there is no thread selected (target_thread_address_space internal-errors if @@ -471,15 +487,7 @@ get_thread_arch_regcache (ptid_t ptid, struct gdbarch *gdbarch) ? NULL : target_thread_address_space (ptid)); - new_regcache = regcache_xmalloc_1 (gdbarch, aspace, 0); - new_regcache->ptid = ptid; - - list = xmalloc (sizeof (struct regcache_list)); - list->regcache = new_regcache; - list->next = current_regcache; - current_regcache = list; - - return new_regcache; + return get_thread_arch_aspace_regcache (ptid, gdbarch, aspace); } static ptid_t current_thread_ptid; diff --git a/gdb/regcache.h b/gdb/regcache.h index 7f7dc10..5531f39 100644 --- a/gdb/regcache.h +++ b/gdb/regcache.h @@ -28,6 +28,9 @@ struct address_space; extern struct regcache *get_current_regcache (void); extern struct regcache *get_thread_regcache (ptid_t ptid); extern struct regcache *get_thread_arch_regcache (ptid_t, struct gdbarch *); +extern struct regcache *get_thread_arch_aspace_regcache (ptid_t, + struct gdbarch *, + struct address_space *); void regcache_xfree (struct regcache *regcache); struct cleanup *make_cleanup_regcache_xfree (struct regcache *regcache); -- 1.7.0.4 --------------020007010509010006030908--