On Wed, 2007-12-19 at 23:44 -0800, Roland McGrath wrote:
> > If we always set the single step bit in the PTRACE_SINGLESTEP call and
> > have a signal to deliver (data!=0) then the we will do a SIGTRAP and at
> > the same time the single step bit would be set. To make it perfect we
> > should clear the bit in this case again. I'd probably add the
> > ptrace_notify(SIGTRAP) + clear_single_step calls to handle_signal /
> > handle_signal32.
>
> I don't see any reason to clear per_info.single_step in this case.
> The thread is stopped at the handler entry point. Resuming it will
> always explicitly set or clear single-step, i.e. PTRACE_SINGLESTEP
> sets and PTRACE_CONT et al (including ptrace_disable for sudden
> disconnection) clear it. It's no different from a normal single-step
> stop--the bit remains set then too, doesn't it?
That is true. The debugger did call PTRACE_SINGLESTEP after all. One way
to think of it is that the entry to the signal handler is an implicit
branch that is single-stepped.
> Since you already have the TIF_SINGLE_STEP behavior on the way back to
> user mode, I think it's preferable just to set TIF_SINGLE_STEP in
> handle_signal. (ptrace_notify is a bit of a kludge, and by setting
> TIF_SINGLE_STEP you are making it exactly like a normal single-step
> trap from the userland perspective.)
>
> > Somelike like the patch below ?
>
> That patch doesn't make sense to me. It tests TIF_SINGLE_STEP in
> handler setup, but that won't be set for the case of PTRACE_SINGLESTEP
> with a handled signal. I think what handler setup wants is:
Don't know what I was thinking, I just removed the setting of the
TIF_SINGLE_STEP from the PTRACE_SINGLESTEP call. May I claim lack of
coffee?
> if (current->thread.per_info.single_step)
> set_thread_flag(TIF_SINGLE_STEP);
This make much more sense. And it is right on the same track to make the
signal handler entry look like a single step of an implicit branch.
> If I'm not mistaken, the patch below (wholly untested) is about right.
> I renamed the functions to the (new) canonical names and added the
> arch_has_single_step macro so that after my ptrace_request changes
> (now in -mm) land, s390 can drop its PTRACE_{CONT,SINGLESTEP,etc} code
> altogether and use the new generic code in ptrace_request.
On first glance looks good to me, I'll test it. Only why do you rename
set_single_step and clear_single_step? Is that required for the utrace
patches?
--
blue skies,
Martin.
"Reality continues to ruin my life." - Calvin.
-
To unsubscribe from this list: send the line "unsubscribe linux-s390" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
