On Fri, 2007-12-14 at 19:41 -0800, Roland McGrath wrote:
> In the child, get_signal_to_deliver returns 0, having ignored SIGWINCH as
> is its default. do_signal returns to its call in sysc_sigpending. It
> tests TIF_SINGLE_STEP and jumps to sysc_singlestep->do_single_step. This
> posts the SIGTRAP, and goes around again to report that signal to the
> debugger. All the while, the child's user PSW has never changed from what
> it was when the debugger called PTRACE_SINGLESTEP. This is the "zero-step"
> scenario. Am I mistaken about it happening this way?
Ok, I got it now. Your analysis is sound.
> Next, I was talking alternatively about what the situation would be if
> s390's PTRACE_SINGLESTEP code looked like the normal case, i.e.:
>
> clear_tsk_thread_flag(child,TIF_SYSCALL_TRACE);
> set_single_step(child);
> child->exit_code = data;
> /* give it a chance to run. */
> wake_up_process(child);
>
> If you did that by itself, then in the case I described above, everything
> would be fine--there is no signal handler, the hardware single-step bit is
> set, and the next user instruction gets stepped properly just the same as
> when data==0. However, in the signal handler case it would step over the
> first instruction of the handler instead of taking a SIGTRAP immediately
> after the handler setup as we want.
Correct.
> So, think we now understand both what is wrong with the current code and
> what will be wrong with the "generic" code if we don't change anything else.
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 do believe I fully understand how the existing s390 code works. I am not
> suggesting it should match the style of the x86 code, only both that it
> should behave correctly in all cases and that it should try to do so in a
> way that does not require deviation from the canonical code pattern for the
> PTRACE_SINGLESTEP case (those few lines above). If I am not mistaken in
> the scenario I described above, it is wrong in one case now. If I haven't
> overlooked or misunderstood something about the code paths, using the
> ptrace_notify conditional on per_info.single_step instead of the variant
> PTRACE_SINGLESTEP implementation would solve both issues.
Somelike like the patch below ?
--
blue skies,
Martin.
"Reality continues to ruin my life." - Calvin.
---
Subject: [PATCH] single step vs. ignored signals.
From: Martin Schwidefsky <schwidefsky@xxxxxxxxxx>
Signed-off-by: Martin Schwidefsky <schwidefsky@xxxxxxxxxx>
---
diff -urpN linux-2.6/arch/s390/kernel/compat_signal.c linux-2.6-patched/arch/s390/kernel/compat_signal.c
--- linux-2.6/arch/s390/kernel/compat_signal.c 2007-12-19 11:27:23.000000000 +0100
+++ linux-2.6-patched/arch/s390/kernel/compat_signal.c 2007-12-19 11:29:57.000000000 +0100
@@ -580,6 +580,10 @@ handle_signal32(unsigned long sig, struc
sigaddset(¤t->blocked,sig);
recalc_sigpending();
spin_unlock_irq(¤t->sighand->siglock);
+ if (test_thread_flag(TIF_SINGLE_STEP)) {
+ clear_single_step(current);
+ ptrace_notify(SIGTRAP);
+ }
}
return ret;
}
diff -urpN linux-2.6/arch/s390/kernel/ptrace.c linux-2.6-patched/arch/s390/kernel/ptrace.c
--- linux-2.6/arch/s390/kernel/ptrace.c 2007-12-19 11:27:23.000000000 +0100
+++ linux-2.6-patched/arch/s390/kernel/ptrace.c 2007-12-19 11:26:42.000000000 +0100
@@ -86,13 +86,13 @@ FixPerRegisters(struct task_struct *task
per_info->control_regs.bits.storage_alt_space_ctl = 0;
}
-static void set_single_step(struct task_struct *task)
+void set_single_step(struct task_struct *task)
{
task->thread.per_info.single_step = 1;
FixPerRegisters(task);
}
-static void clear_single_step(struct task_struct *task)
+void clear_single_step(struct task_struct *task)
{
task->thread.per_info.single_step = 0;
FixPerRegisters(task);
@@ -675,10 +675,7 @@ do_ptrace(struct task_struct *child, lon
return -EIO;
clear_tsk_thread_flag(child, TIF_SYSCALL_TRACE);
child->exit_code = data;
- if (data)
- set_tsk_thread_flag(child, TIF_SINGLE_STEP);
- else
- set_single_step(child);
+ set_single_step(child);
/* give it a chance to run. */
wake_up_process(child);
return 0;
diff -urpN linux-2.6/arch/s390/kernel/signal.c linux-2.6-patched/arch/s390/kernel/signal.c
--- linux-2.6/arch/s390/kernel/signal.c 2007-12-19 11:27:23.000000000 +0100
+++ linux-2.6-patched/arch/s390/kernel/signal.c 2007-12-19 11:29:40.000000000 +0100
@@ -396,6 +396,10 @@ handle_signal(unsigned long sig, struct
sigaddset(¤t->blocked,sig);
recalc_sigpending();
spin_unlock_irq(¤t->sighand->siglock);
+ if (test_thread_flag(TIF_SINGLE_STEP)) {
+ clear_single_step(current);
+ ptrace_notify(SIGTRAP);
+ }
}
return ret;
diff -urpN linux-2.6/include/asm-s390/ptrace.h linux-2.6-patched/include/asm-s390/ptrace.h
--- linux-2.6/include/asm-s390/ptrace.h 2007-12-19 11:27:23.000000000 +0100
+++ linux-2.6-patched/include/asm-s390/ptrace.h 2007-12-19 11:26:42.000000000 +0100
@@ -469,7 +469,10 @@ struct user_regs_struct
#define instruction_pointer(regs) ((regs)->psw.addr & PSW_ADDR_INSN)
#define regs_return_value(regs)((regs)->gprs[2])
#define profile_pc(regs) instruction_pointer(regs)
-extern void show_regs(struct pt_regs * regs);
+void show_regs(struct pt_regs * regs);
+struct task_struct;
+void set_single_step(struct task_struct *task);
+void clear_single_step(struct task_struct *task);
#endif /* __KERNEL__ */
#endif /* __ASSEMBLY__ */
-
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
