* [patch 1/2] Displaced stepping across fork/vfork @ 2011-09-11 0:55 Yao Qi 2011-09-12 14:59 ` Pedro Alves 0 siblings, 1 reply; 6+ messages in thread From: Yao Qi @ 2011-09-11 0:55 UTC (permalink / raw) To: gdb-patches [-- Attachment #1: Type: text/plain, Size: 901 bytes --] Hi, The current displaced stepping doesn't consider the case that we set a breakpoint on syscall insn, which will cause fork/vfork, and single step over it with displaced stepping. In this case, syscall insn is executed in scratch pad, and GDB will receive TARGET_WAITKIND_FORKED and TARGET_WAITKIND_VFORKED before it cleans up displaced stepping state. The other problem, in this case, is that the PC of child process points to the scratch pad instead of original program location. This patch is to address these two problems. When GDB receives TARGET_WAITKIND_FORKED or TARGET_WAITKIND_VFORKED, it will check whether it has been in displaced stepping. If it is, that means GDB executed a syscall insn in scratch pad, then, we should cleanup the displaced stepping state, such as clean up scratch pad, and set the PC of child process to the original location. -- Yao (é½å°§) [-- Attachment #2: 0014-disp-step-cross-vfork.patch --] [-- Type: text/x-patch, Size: 6895 bytes --] 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 | 83 ++++++++++++++++++++++++++++++++++++++++++++++++------- gdb/regcache.c | 31 +++++++++++++------- gdb/regcache.h | 3 ++ 3 files changed, 95 insertions(+), 22 deletions(-) diff --git a/gdb/infrun.c b/gdb/infrun.c index 729f148..2788b08 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,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. */ + { + 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. */ + 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, + 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 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. */ + + child_regcache + = get_thread_arch_aspace_regcache (ecs->ws.value.related_pid, + gdbarch, + parent_inf->aspace); + /* Read PC value of father 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..c64bcb8 100644 --- a/gdb/regcache.c +++ b/gdb/regcache.c @@ -450,17 +450,34 @@ 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 +488,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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch 1/2] Displaced stepping across fork/vfork 2011-09-11 0:55 [patch 1/2] Displaced stepping across fork/vfork Yao Qi @ 2011-09-12 14:59 ` Pedro Alves 2011-09-14 11:10 ` Yao Qi 0 siblings, 1 reply; 6+ messages in thread From: Pedro Alves @ 2011-09-12 14:59 UTC (permalink / raw) To: gdb-patches; +Cc: Yao Qi On Sunday 11 September 2011 01:45:39, Yao Qi wrote: > Hi, > The current displaced stepping doesn't consider the case that we set a > breakpoint on syscall insn, which will cause fork/vfork, and single step > over it with displaced stepping. > > In this case, syscall insn is executed in scratch pad, and GDB will > receive TARGET_WAITKIND_FORKED and TARGET_WAITKIND_VFORKED before it > cleans up displaced stepping state. The other problem, in this case, is > that the PC of child process points to the scratch pad instead of > original program location. > > This patch is to address these two problems. When GDB receives > TARGET_WAITKIND_FORKED or TARGET_WAITKIND_VFORKED, it will check whether > it has been in displaced stepping. If it is, that means GDB executed a > syscall insn in scratch pad, then, we should cleanup the displaced > stepping state, such as clean up scratch pad, and set the PC of child > process to the original location. 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? Do we leave stale displaced stepping state behind in the later case? > > -- > Yao (齐尧) > 0014-disp-step-cross-vfork.patch > 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 | 83 ++++++++++++++++++++++++++++++++++++++++++++++++------- > gdb/regcache.c | 31 +++++++++++++------- > gdb/regcache.h | 3 ++ > 3 files changed, 95 insertions(+), 22 deletions(-) > > diff --git a/gdb/infrun.c b/gdb/infrun.c > index 729f148..2788b08 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,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. */ > + { > + 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/. > + 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 > + because their pages are shared. */ Spurious double-space after because. > + 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. */ > + > + 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. */ > + /* Read PC value of father process. */ s/father/parent > + 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..c64bcb8 100644 > --- a/gdb/regcache.c > +++ b/gdb/regcache.c > @@ -450,17 +450,34 @@ 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) > +{ > + Spurious newline. > + 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 +488,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 -- Pedro Alves ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch 1/2] Displaced stepping across fork/vfork 2011-09-12 14:59 ` Pedro Alves @ 2011-09-14 11:10 ` Yao Qi 2011-09-14 11:55 ` Pedro Alves 0 siblings, 1 reply; 6+ messages in thread From: Yao Qi @ 2011-09-14 11:10 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches [-- Attachment #1: Type: text/plain, Size: 5738 bytes --] 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 (é½å°§) [-- Attachment #2: 0014-disp-step-cross-vfork.patch --] [-- Type: text/x-patch, Size: 6994 bytes --] 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch 1/2] Displaced stepping across fork/vfork 2011-09-14 11:10 ` Yao Qi @ 2011-09-14 11:55 ` Pedro Alves 2011-09-14 15:39 ` Yao Qi 0 siblings, 1 reply; 6+ messages in thread From: Pedro Alves @ 2011-09-14 11:55 UTC (permalink / raw) To: Yao Qi; +Cc: gdb-patches On Wednesday 14 September 2011 11:50:24, Yao Qi wrote: > 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. Hmm, but I think we do? :-) We'll leave the displaced stepping state in the displaced_step_inferior_states list? I can't check as one can't place breakpoints on the vsyscall on x86, it seems. > 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 think LOADED would need it too, but no target both uses displaced stepping and emits that event currently. > 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. Sure, thanks for that. > Hope this patch can go in first. Yep. It's a good improvement already. > > /* 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. :) You forgot to copy a couple "the"s though. :-) Does the arm_displaced_step_fixup handler ever do more than just adjusting the PC for syscalls? I see there's a ->cleanup callback there that does some things with registers. If so, we'll need to do more than just adjusting the PC of the child. (A more general fix would run the fixup on the child too instead.) Otherwise looks okay. On Wednesday 14 September 2011 11:50:24, Yao Qi wrote: > + regcache_write_pc (child_regcache, parent_pc); > + > + } You have a spurious empty line here. -- Pedro Alves ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch 1/2] Displaced stepping across fork/vfork 2011-09-14 11:55 ` Pedro Alves @ 2011-09-14 15:39 ` Yao Qi 2011-09-17 13:43 ` [committed] : " Yao Qi 0 siblings, 1 reply; 6+ messages in thread From: Yao Qi @ 2011-09-14 15:39 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches On 09/14/2011 07:32 PM, Pedro Alves wrote: >> > The past tense ("was executed") is better here. Copied your suggestion >> > directly. :) > You forgot to copy a couple "the"s though. :-) > Oops. I missed two "the" in the first line of comment. Added them. > Does the arm_displaced_step_fixup handler ever do more than > just adjusting the PC for syscalls? I see there's a ->cleanup > callback there that does some things with registers. If so, > we'll need to do more than just adjusting the PC of the child. > (A more general fix would run the fixup on the child too > instead.) > A general cleanup routine does some registers restores, but cleanup routines for svc (arm-linux-tdep.c:arm_linux_cleanup_svc and arm-tdep.c:cleanup_svc) only adjust PC. > Otherwise looks okay. > > On Wednesday 14 September 2011 11:50:24, Yao Qi wrote: >> > + regcache_write_pc (child_regcache, parent_pc); >> > + >> > + } > You have a spurious empty line here. I'll check in with these fixes. -- Yao (é½å°§) ^ permalink raw reply [flat|nested] 6+ messages in thread
* [committed] : [patch 1/2] Displaced stepping across fork/vfork 2011-09-14 15:39 ` Yao Qi @ 2011-09-17 13:43 ` Yao Qi 0 siblings, 0 replies; 6+ messages in thread From: Yao Qi @ 2011-09-17 13:43 UTC (permalink / raw) To: gdb-patches On 09/14/2011 10:25 PM, Yao Qi wrote: > > I'll check in with these fixes. Patch is committed. http://sourceware.org/ml/gdb-cvs/2011-09/msg00109.html -- Yao (é½å°§) ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-09-17 13:42 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2011-09-11 0:55 [patch 1/2] Displaced stepping across fork/vfork Yao Qi 2011-09-12 14:59 ` Pedro Alves 2011-09-14 11:10 ` Yao Qi 2011-09-14 11:55 ` Pedro Alves 2011-09-14 15:39 ` Yao Qi 2011-09-17 13:43 ` [committed] : " Yao Qi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox