On 11/18, Roland McGrath wrote: > > > Hmm. not sure I understand how this can work. I mean, this won't > > enable stepping (in this particular case we are discussing). > > I might be confused. I'm thinking of two cases: the report_syscall_entry > pass yields UTRACE_SINGLESTEP, or that it yields UTRACE_STOP and then > utrace_control() sets utrace->resume to UTRACE_SINGLESTEP. > > The former is e.g. PTRACE_SINGLESTEP while an unrelated engine uses > UTRACE_EVENT(SYSCALL_ENTRY). The ptrace engine's report_quiesce returns > UTRACE_SINGLESTEP. finish_resume_report() calls user_enable_single_step(). > That seems fine. Right?
In this case ptrace_report_quiesce(event) returns UTRACE_RESUME, note that it does return event ? UTRACE_RESUME : ctx->resume; and event == SYSCALL_ENTRY. This looks correct. > The latter is e.g. PTRACE_SINGLESTEP after ptrace stop at syscall entry. Yes, this is the case I was talking about. > I see. finish_resume_report() will only do utrace_stop() and then we'll > go directly into the syscall unless someone used UTRACE_REPORT. Yes, > utrace_stop() will only set TIF_NOTIFY_RESUME and utrace->resume. In > the real resume-to-user cases, that's fine because we'll handle that in > utrace_resume() or utrace_get_signal() before doing anything else. Not sure I understand. utrace_stop() will not set TIF_NOTIFY_RESUME? We are talking about the case when the tracee stops in SYSCALL_ENTER, and then the tracer does utrace_control(UTRACE_SINGLESTEP) to resume. In this case utrace_control() sets ->resume/TIF_NOTIFY_RESUME, yes. > @@ -1794,11 +1794,13 @@ static void finish_resume_report(struct > { > finish_report_reset(task, utrace, report); > > - switch (report->action) { > - case UTRACE_STOP: > - utrace_stop(task, utrace, report->resume_action); > - break; > + if (report->action == UTRACE_STOP) { > + utrace_stop(task, utrace, report->resume_action, true); > + report->action = start_report(utrace); > + WARN_ON(report->action == UTRACE_STOP); > + } > > + switch (report->action) { > case UTRACE_INTERRUPT: > if (!signal_pending(task)) > set_tsk_thread_flag(task, TIF_SIGPENDING); Yes, this means the tracee gets TIF_SINGLESTEP right after resume, and > this > seems like the right idea for how to get tracehook_report_syscall_exit > called in the cases where it is in old ptrace. Afaics yes. But, what if another engine does utrace_control(UTRACE_INTERRUPT) ? > > Instead of fixing utrace_report_syscall_entry(), perhaps we should > > change utrace_stop() path to enable the stepping if needed, but this > > means we return to "synchronous" utrace_control(UTRACE_XXXSTEP). > > I don't really follow what you mean by "synchronous". Argh. I meant, from the caller pov, utrace_control(UTRACE_SINGLESTEP) does enable_step() "immediately", like it did before utrace-cleanup changes. IOW, suppose the tracee is stopped, and ptrace does UTRACE_SINGLESTEP. The tracee resumes, processes ->resume, and does enable_step(). >From the ptrace pov, it looks as if utrace_control() does enable_step() before utrace_wakeup(). > Your suggestion is > equivalent to making utrace_finish_vfork call finish_resume_report instead > of utrace_stop, isn't it? I meant, every time the tracee stops, it should process ->resume after wakeup. Looks like, the patch you sent in another thread (which adds apply_resume_action()) does something like this, right? I'll try to read it carefully now. Oleg.