Re: [patch 1/2] Displaced stepping across fork/vfork

Yao Qi
2011-09-14T10:50:51Z

[-- 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