Re: s390 user_enable_single_step() (Was: odd utrace testing results on s390x)
Martin, sorry for delay, On 01/07, Martin Schwidefsky wrote: On Wed, 6 Jan 2010 13:13:29 -0800 (PST) Roland McGrath rol...@redhat.com wrote: However, with or without CONFIG_UTRACE, 6580807da14c423f0d0a708108e6df6ebc8bc83d is needed on s390 too, otherwise the child gets unnecessary traps. This confuses me. user_disable_single_step on non-current doesn't do anything not already done by the memset in copy_thread. Ooh, except perhaps it does not clear PSW_MASK_PER. Maybe that matters. That's the only thing I can think of. Maybe Martin can make sense of it. I am confused as well. Yes, I thought about regs-psw.mask change too, but I don't understand why it helps.. The additional traps should not happen anymore with this patch: -- Subject: [PATCH] clear TIF_SINGLE_STEP for new process. From: Martin Schwidefsky schwidef...@de.ibm.com Clear the TIF_SINGLE_STEP bit in copy_thread. If the new process is not auto-attached by the tracer it is wrong to delivere SIGTRAP to the new process. Signed-off-by: Martin Schwidefsky schwidef...@de.ibm.com --- arch/s390/kernel/process.c |1 + 1 file changed, 1 insertion(+) diff -urpN linux-2.6/arch/s390/kernel/process.c linux-2.6-patched/arch/s390/kernel/process.c --- linux-2.6/arch/s390/kernel/process.c 2009-12-03 04:51:21.0 +0100 +++ linux-2.6-patched/arch/s390/kernel/process.c 2010-01-07 09:25:53.0 +0100 @@ -217,6 +217,7 @@ int copy_thread(unsigned long clone_flag p-thread.mm_segment = get_fs(); /* Don't copy debug registers */ memset(p-thread.per_info, 0, sizeof(p-thread.per_info)); + clear_tsk_thread_flag(p, TIF_SINGLE_STEP); Even if I don't understand s390, I think this patch makes sense anyway. Or, user_disable_single_step() can clear this bit. But. Acoording to the testing I did (unless I did something wrong again) this patch doesn't make any difference in this particular case. 6580807da14c423f0d0a708108e6df6ebc8bc83d does. And. Please note that the test-case triggers 799 false step, but TIF_SINGLE_STEP is surely cleared (by the caller) after the first invocation of do_single_step(). Oleg.
Re: s390 user_enable_single_step() (Was: odd utrace testing results on s390x)
On 01/07, Martin Schwidefsky wrote: On Wed, 6 Jan 2010 13:08:12 -0800 (PST) Roland McGrath rol...@redhat.com wrote: That's what tracehook_signal_handler is for. You're both doing it yourself in the arch code (by setting TIF_SINGLE_STEP), and then telling the generic code to do it (by passing stepping=1 to tracehook_signal_handler). Ok, so with the full utrace the semantics of tracehook_signal_handler is more than just causing a SIGTRAP. It is an indication for a signal AND a SIGTRAP if single-stepping is active. To make both cases work we should stop setting TIF_SINGLE_STEP in do_signal and pass current-thread.per_info.single_step to tracehook_signal_handler instead of test_thread_flag(TIF_SINGLE_STEP). Can't understand why do we need TIF_SINGLE_STEP at all. Just pass current-thread.per_info.single_step to tracehook_signal_handler() ? Oleg. --- a/arch/s390/kernel/signal.c +++ b/arch/s390/kernel/signal.c @@ -504,14 +504,8 @@ void do_signal(struct pt_regs *regs) * 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); - - /* -* Let tracing know that we've done the handler setup. -*/ tracehook_signal_handler(signr, info, ka, regs, -test_thread_flag(TIF_SINGLE_STEP)); +current-thread.per_info.single_step); } return; }
Re: s390 user_enable_single_step() (Was: odd utrace testing results on s390x)
Right. That means we should leave the status quo of not clearing TIF_SINGLE_STEP in user_disable_single_step. Ok, although it seems a bit strange not to do it. Perhaps I should add a comment about it. It doesn't seem strange to me, but then I've just been through all this. user_*_step is about what the task will do next. TIF_SINGLE_STEP is about what the task has done recently. Of course more good comments always help. I might be inclined to change the name of TIF_SINGLE_STEP so that its true purpose is more obvious. AFAICT, in fact it is not even about single-step per se. It means some PER trap happened and should produce SIGTRAP. Don't you get the same thing if you haven't used single-step, but instead used PTRACE_POKEUSR to set up per_struct with bits that say to trigger for some other reason? How about calling it TIF_PER_PENDING? So after everthing has been converted to utrace we always will load the control registers in FixPerRegisters. Right. (This could well still change in the future. But that's how it is in utrace now. And regardless of possible future implementation changes it will always be the case that sometimes it will be called on current.) We could use the same compare of the control registers as the code in __switch_to. See below. Yes, sounds good. The PSW_MASK_PER is the global PER enablement switch, the PER_EM_MASK bits enable the different PER events. We check for the PER_EM_MASK bits because it is easier to access in __switch_to. The return PSW is stored in the pt_regs structure, we would have to get a pointer to it (what regs = task_pt_regs(taks) does in FixPerRegisters). In FixPerRegisters we can as well use the PSW_MASK_PER bit. I see. Thanks for the explanation. Thanks, Roland
Re: s390 user_enable_single_step() (Was: odd utrace testing results on s390x)
Hmm, command for tracehook_signal_handler say this for stepping: @stepping: nonzero if debugger single-step or block-step in use Are you saying you would like me to clarify that wording somehow? It's meant to be implicit that the arch code is not doing any special fakery about single-step for signal handlers, only processing real single-step traps (and faking them for a syscall instruction if the arch requires that). No other arch does it, so it didn't occur to me that s390 would. Before tracehook some had ptrace_notify calls there, and the call to tracehook_signal_handler replaced that call. In ptrace (including utrace-based ptrace), this winds up with sending a SIGTRAP. So when we finally do get out of do_signal and TIF_SINGLE_STEP causes a second SIGTRAP, it's already pending and the second one makes no difference. So we have been lucky so far. Actually, Oleg rightly points out: Confused again, perhaps I just misunderstood what you mean... Without utrace, tracehook_signal_handler() doesn't send SIGTRAP, it merely does ptrace_notify(SIGTRAP), this means that [...] even without utrace we can have unexpected SIGTRAP. That is quite true, and I just misremembered when writing that paragraph. So indeed we have been lucky, but it's not the luck of the problem not happening on s390, but the luck of nobody ever caring. :-) Ok, so with the full utrace the semantics of tracehook_signal_handler is more than just causing a SIGTRAP. It is an indication for a signal AND a SIGTRAP if single-stepping is active. In short, it is the indication of a signal handler having been set up, just like its kerneldoc description says. Whatever that should mean to tracing (SIGTRAP or otherwise) is in the purview of the generic tracing layer, not the arch layer. To make both cases work we should stop setting TIF_SINGLE_STEP in do_signal and pass current-thread.per_info.single_step to tracehook_signal_handler instead of test_thread_flag(TIF_SINGLE_STEP). Correct. Thanks, Roland
Re: s390 user_enable_single_step() (Was: odd utrace testing results on s390x)
Clear the TIF_SINGLE_STEP bit in copy_thread. If the new process is not auto-attached by the tracer it is wrong to delivere SIGTRAP to the new process. The change is right, but this log entry is confusing. auto-attached has nothing to do with it, nor does anything about tracing the new process or not. The new process has not experienced a PER trap of its own, so it is wrong to deliver a SIGTRAP that is meant for its creator. Thanks, Roland
Re: s390 user_enable_single_step() (Was: odd utrace testing results on s390x)
I am confused as well. Yes, I thought about regs-psw.mask change too, but I don't understand why it helps.. [...] But. Acoording to the testing I did (unless I did something wrong again) this patch doesn't make any difference in this particular case. 6580807da14c423f0d0a708108e6df6ebc8bc83d does. Those results are quite mysterious to me. I think we'll have to get Martin to sort it out definitively. Thanks, Roland
