> 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? 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: if (current->thread.per_info.single_step) set_thread_flag(TIF_SINGLE_STEP); 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. Thanks, Roland --- [PATCH] s390 single-step cleanup Signed-off-by: Roland McGrath <roland@xxxxxxxxxx> --- diff --git a/arch/s390/kernel/ptrace.c b/arch/s390/kernel/ptrace.c index 1d81bf9..0000000 100644 --- a/arch/s390/kernel/ptrace.c +++ b/arch/s390/kernel/ptrace.c @@ -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 user_enable_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 user_disable_single_step(struct task_struct *task) { task->thread.per_info.single_step = 0; FixPerRegisters(task); @@ -107,7 +107,7 @@ void ptrace_disable(struct task_struct *child) { /* make sure the single step bit is not set. */ - clear_single_step(child); + user_disable_single_step(child); } #ifndef CONFIG_64BIT @@ -651,7 +651,7 @@ do_ptrace(struct task_struct *child, lon clear_tsk_thread_flag(child, TIF_SYSCALL_TRACE); child->exit_code = data; /* make sure the single step bit is not set. */ - clear_single_step(child); + user_disable_single_step(child); wake_up_process(child); return 0; @@ -665,7 +665,7 @@ do_ptrace(struct task_struct *child, lon return 0; child->exit_code = SIGKILL; /* make sure the single step bit is not set. */ - clear_single_step(child); + user_disable_single_step(child); wake_up_process(child); return 0; @@ -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); + user_enable_single_step(child); /* give it a chance to run. */ wake_up_process(child); return 0; diff --git a/arch/s390/kernel/signal.c b/arch/s390/kernel/signal.c index d264671..0000000 100644 --- a/arch/s390/kernel/signal.c +++ b/arch/s390/kernel/signal.c @@ -471,6 +471,7 @@ void do_signal(struct pt_regs *regs) if (signr > 0) { /* Whee! Actually deliver the signal. */ + int ret; #ifdef CONFIG_COMPAT if (test_thread_flag(TIF_31BIT)) { extern int handle_signal32(unsigned long sig, @@ -478,15 +479,12 @@ void do_signal(struct pt_regs *regs) siginfo_t *info, sigset_t *oldset, struct pt_regs *regs); - if (handle_signal32( - signr, &ka, &info, oldset, regs) == 0) { - if (test_thread_flag(TIF_RESTORE_SIGMASK)) - clear_thread_flag(TIF_RESTORE_SIGMASK); - } - return; + ret = handle_signal32(signr, &ka, &info, oldset, regs); } + else #endif - if (handle_signal(signr, &ka, &info, oldset, regs) == 0) { + ret = handle_signal(signr, &ka, &info, oldset, regs); + if (!ret) { /* * A signal was successfully delivered; the saved * sigmask will have been stored in the signal frame, @@ -495,6 +493,14 @@ void do_signal(struct pt_regs *regs) */ if (test_thread_flag(TIF_RESTORE_SIGMASK)) clear_thread_flag(TIF_RESTORE_SIGMASK); + + /* + * If we would have taken a single-step trap + * for a normal instruction, act like we took + * one for the handler setup. + */ + if (current->thread.per_info.single_step) + set_thread_flag(TIF_SINGLE_STEP); } return; } diff --git a/include/asm-s390/ptrace.h b/include/asm-s390/ptrace.h index 332ee73..0000000 100644 --- a/include/asm-s390/ptrace.h +++ b/include/asm-s390/ptrace.h @@ -465,6 +465,13 @@ struct user_regs_struct #ifdef __KERNEL__ #define __ARCH_SYS_PTRACE 1 +/* + * These are defined as per linux/ptrace.h, which see. + */ +#define arch_has_single_step() (1) +extern void user_enable_single_step(struct task_struct *); +extern void user_disable_single_step(struct task_struct *); + #define user_mode(regs) (((regs)->psw.mask & PSW_MASK_PSTATE) != 0) #define instruction_pointer(regs) ((regs)->psw.addr & PSW_ADDR_INSN) #define regs_return_value(regs)((regs)->gprs[2]) - 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