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 (齐尧)