On 10/28, Roland McGrath wrote:
>
> I've made a new branch, utrace-cleanup.
> This forks from utrace-indirect and has:
>
> 26fefca utrace: sticky resume action
> 28b2774 utrace: remove ->stopped field
I am not sure I understand the new code in details - too much changes.
Anyway, I can never understand the code after the first reading. At
first glance, imho this change is "right in general" but:
1. This breaks the current implementation of utrace-ptrace.
I guess I misunderstood you when we discussed this before, I thought
that the tracee should handle UTRACE_SINGLESTEP right after resume
and call user_enable_single_step(). However, enable_step() is called
at utrace_resume/utrace_get_signal time, this is too late for ptrace.
I guess this branch is the code which will be sent to lkml, right?
In this case utrace-cleanup should be merged into utrace-ptrace right
now, then I need to fix ptrace. Basically, ptrace_report_quiesce() and
ptrace_report_interrupt() should detect the case when the tracee was
resumed by PTRACE_XXXSTEP and then it passed syscall_trace_leave()
without TIF_SINGLESTEP, and synthesize a trap.
The main problem is that this is not consistent across the different
arch'es :[
2. Cosmetic, but the usage of utrace_task_alloc() looks a bit strange.
Why it returns bool, not struct utrace * ?
3. Now that we have utrace->resume, can't we kill report->resume_action ?
4. One of the changes in utrace_get_signal() doesn't look exactly right.
if (utrace->resume < UTRACE_RESUME || utrace->signal_handler) {
...
if (resume > UTRACE_REPORT) {
report.action = resume;
finish_resume_report(&report);
}
Yes, we need finish_resume_report() here, but since report->takers
is not set, finish_report_reset() will always call utrace_reset().
OTOH, we can't set ->takers before finish_resume_report(), we can
miss UTRACE_DETACH request. utrace_control(DETACH)->utrace_do_stop()
does not change utrace->resume != UTRACE_RESUME.
And since utrace_do_stop() never "upgrades" utrace->resume, we have
another problem: UTRACE_STOP request can be lost here.
5. utrace_resume() has the same problems. If report->action != REPORT
we do not call callbacks and finish_resume_report() is called with
->takers == F.
6. But! I think utrace_resume() was always wrong here. Because it
calls start_callback(events => 0) and thus we nevet set ->takers.
7. utrace_attach_task() has an implicit wmb() between "->utrace = new_utrace"
and "->utrace_flags = REAP", this is good.
But, for example, tracehook_force_sigpending() does not have rmb(),
this means utrace_interrupt_pending() can OOPS in theory.
8. Completely off-topic, but utrace_control() has a very strange comment
under "case UTRACE_INTERRUPT",
* When it's stopped, we know it's always going
* through utrace_get_signal() and will recalculate.
can't recall if it were ever true, but surely it is not now?
Oleg.
