> Ah, the single step code. Lovely isn't it ? :-) I've read them all and yours ain't the worst. Thanks for the explanation about the interaction between single-step and the system call instruction in the hardware. > > 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. I think we are talking past each other a bit here. To clarify, first let's talk about just the current code as it is. With today's code, consider the case of PTRACE_SINGLESTEP,SIGWINCH when the child has no handler set for SIGWINCH. Before the PTRACE_SINGLESTEP call, the child was in ptrace_stop, called by get_signal_to_deliver. TIF_SIGPENDING is no longer set at this point. Now, PTRACE_SINGLESTEP sets TIF_SINGLE_STEP and wakes up the child. 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? 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. 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 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. Agreed. > > 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 ? 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. Thanks, Roland - 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
