> 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?
The latter is e.g. PTRACE_SINGLESTEP after ptrace stop at syscall entry.
I see. finish_resume_report() will only do utrace_stop() and then we'll
go directly into the syscall unless someone used UTRACE_REPORT.
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.
I think this maybe wouldn't hurt anything:
@@ -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);
(In the omitted part of the patch, the new flag to utrace_stop tells it
never to bother with setting TIF_NOTIFY_RESUME for < UTRACE_REPORT.)
Then even for the real resume-to-user cases, we skip a cycle out to arch
asm and back into utrace_resume() for utrace_control(,,UTRACE_SINGLESTEP)
after UTRACE_STOP. (That microoptimization does not really matter, but it
falls naturally out of the change.)
> > - } else if (utrace->resume == UTRACE_REPORT) {
> > + } else if (utrace->resume <= UTRACE_REPORT) {
>
> perhaps, you meant ">= UTRACE_REPORT" ?
No, it's to catch UTRACE_INTERRUPT as requesting a report too,
as you noticed in the other email thread should be done.
> OK, probably I missed the implemenation details, but I think I got
> the idea, and this should fix the step-after-syscall_enter problem.
Right. My implementation details are not quite right either. But this
seems like the right idea for how to get tracehook_report_syscall_exit
called in the cases where it is in old ptrace.
> But. We have the same problem with utrace_finish_vfork(), no?
Yes. But, hmm.
utrace_stop(task, utrace, UTRACE_RESUME); /* XXX */
This XXX was there about passing UTRACE_RESUME, because that's not really
right. I guess I was actually thinking of this very case, but didn't know
at the time quite what the case was.
> 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". Your suggestion is
equivalent to making utrace_finish_vfork call finish_resume_report instead
of utrace_stop, isn't it? The callers of utrace_stop() are:
* finish_resume_report
-> the main guy
-> proposed (above) to also handle utrace->resume immediately after stop
* utrace_report_exit
-> doesn't matter (?), nothing but UTRACE_STOP and possibly
TIF_SIGPENDING are meaningful any more
* do_report_syscall_entry
-> proposed to call finish_resume_report instead
* utrace_finish_vfork
Ok. So I can see a bit of cleanup to be done in splitting
finish_resume_report and utrace_finish_vfork calling the parts of it,
as will the "We only got here to process utrace->resume." case.
But it will be functionally equivalent to finish_resume_report
with {.spurious=false, .action=utrace->resume}.
That seems like it would cover it all, no? But I am still hesitant
because I don't really understand your "return to synchronous..." comment.
Thanks,
Roland
