On Wed, 2007-12-12 at 19:48 -0800, Roland McGrath wrote: > I think there are some problems with the current s390 implementation of > single-step. In particular, this code: > > case PTRACE_SINGLESTEP: > /* set the trap flag. */ > if (!valid_signal(data)) > 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); > /* give it a chance to run. */ > wake_up_process(child); > return 0; > > The canonical version of that logic does set_single_step unconditionally. > This difference prevents cleaning up s390 ptrace to share more common code > for the non-arch magic parts of ptrace. But I think that in fact this > difference also creates a bug in the current implementation, which would be > fixed by reorganizing things so that the PTRACE_SINGLESTEP code could > indeed match other machines (and so disappear in favor of common code). Ah, the single step code. Lovely isn't it ? > The s390 TIF_SINGLE_STEP flag means "single-step trap has occurred". Once > set, it causes traps.c:do_single_step to be called before the next return to > user mode--that is what calls notifiers (kprobes) and/or sends SIGTRAP. The > PER exception handler (entry64.S:pgm_per_std, pgm_svcper) sets the flag. It > is not clear to me why pgm_per_std doesn't call some code that leads to > do_single_step directly, as the single-step hardware exception handler does > on other machines. But I'm happy to assume there is some s390-specific > reason it can't or shouldn't be done that way. This is the background as I > understand it, please correct me as needed. I would say that TIF_SINGLE_STEP means "deliver a SIGTRAP". The bit is set after the program check handler got a per event (pgm_per_std and pgm_svcper) and if the ptrace parent did a PTRACE_SINGLESTEP call with data != 0. The reason why we do not deliver the SIGTRAP directly in the program check handler is the SVC instruction. If a s390 processor hits a single stepped svc it first does the PSW swap for the svc, immediately followed by the PSW swap for the per-event program check. The trouble is that the svc has not been executed yet, but the ptrace parent should get the SIGTRAP only *after* the system call function has been executed, so that the debugger can see the result of the system call. This means that the call to do_single_step has to be done one the *exit* path of the program/system call check handler. The code is a bit convoluted there. > The case where the current s390 PTRACE_SINGLESTEP code differs from the > norm is data!=0, i.e. single-step with a signal delivery. My guess is that > this code is intended to ensure that single-step into a signal handler > generates a SIGTRAP with the PC still at the very first instruction of the > handler, not the second. That is indeed the desired behavior for that > case. However, I suspect this implementation is broken for other cases. > That is, when the signal is ignored, or default-ignored, or blocked, > i.e. data!=0 but the target thread is going to resume at its existing PC as > it would for data==0, not at a new PC after signal handler setup. In that > case, it looks to me like the thread would get an immediate SIGTRAP without > executing another instruction--a single-step that actually goes zero steps. > (I have not actually tested this scenario to verify my analysis. A simple > test would be a breakpoint or other trap at the beginning of a basic block > several insns long, and then: wait; A=PEEKUSR,PSWADDR; SINGLESTEP,SIGWINCH; > wait; B=PEEKUSR,PSWADDR; assert(B != A).) Indeed that PTRACE_SINGLESTEP sets the TIF_SINGLE_STEP bit for data!=0 fixes the bug that the debugger has to get control while the PSW still points to the first instruction of the signal handler. I cannot quite follow the line of though with zero-step single-steps. On the contrary if the signal is not delivered in the ptrace child then the single step bit will not be set in the PSW. That would cause LOTS of instruction to get executed instead of just one. > If I'm right, this needs to be fixed in the current code. Personally, my > actual motivation is to normalize the s390 code path with all others so we > can clean up ptrace to have much less work going on in arch code. If we don't break s390 in the process I have no objections in general but we have to be careful with it. This stuff is subtle. > On x86, ptrace uses a TIF_SINGLESTEP flag, which has a different meaning > from s390's TIF_SINGLE_STEP. x86 TIF_SINGLESTEP means "should single-step", > and is set in addition to the hardware state that makes the next user > instruction take a hardware single-step trap. The step-into-handler problem > is solved using this: after handler setup it does: > > if (test_thread_flag(TIF_SINGLESTEP)) > ptrace_notify(SIGTRAP); For s390 this test is done by the TIF_SINGLESTEP check on the kernel exit path for the child. There is a difference between the hardware "should single-step each instructions" and the software "the debugger should stop here". The debugger has to make an additional stop for the first instruction of the signal handler. This event is in addition to the stops after each instruction. The TIF_SINGLESTEP bit could have a better name, TIF_"kick the debugger" or something.. > The flag is also used for making single-step into the system call > instruction cause a trap immediately after the syscall completes, rather > than one user instruction later as would otherwise happen, because the x86 > single-step hardware does not treat the syscall like a normal user insn from > the user-mode perspective. If the s390 system call instruction works such > that the pgm_per* stuff gets called as for a normal user instruction, then > you don't need anything like this for the step-into-system call case. > > So it might be enough to add after signal handler setup: > > if (current->thread.per_info.single_step) > ptrace_notify(SIGTRAP); > > Then PTRACE_SINGLESTEP need not touch TIF_SINGLE_STEP, and can always call > set_single_step, matching the style of other machines. This will make it > possible to just rename some functions and define a macro and then use > ptrace_request for CONT, SINGLESTEP, et al following my -mm patches for that. So this boils down to the above check at the end of the signal handler setup, after the system call function returned and the direct call to do_single_step if the traced instruction is not an svc. The s390 solution is to do all three with a single TIF_SINGLE_STEP check on the common exit path for system calls and program checks. Does this make some sense ? -- 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
