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

[Pedro Alves <pedro at codesourcery dot com>]

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