On 03/15, Roland McGrath wrote:
>
> > Then we re-do this (well, almost) check under ->siglock,
> >
> > } else if (task_is_stopped(target)) {
> > if (!(target->utrace_flags & UTRACE_EVENT(JCTL)))
> > utrace->stopped = stopped = true;
> > }
> >
> > But this is not nice. Let's suppose the task is already stopped, we do
> > UTRACE_ATTACH + utrace_set_events(JCTL).
>
> This is exactly why utrace_set_events() sets ->stopped preemptively for
> that case.
Yes, thanks. I saw this code in utrace_set_events(), but then forgot.
> > REPORT(task, utrace, &report, UTRACE_EVENT(JCTL),
> > report_jctl, what, notify);
> >
> > instead.
>
> There is a bug, but your fix changes a key API choice.
> I've put in this fix:
>
> - report_jctl, was_stopped ? CLD_STOPPED : CLD_CONTINUED, what);
> + report_jctl, was_stopped ? CLD_STOPPED : CLD_CONTINUED,
> + notify ? what : 0);
>
> The @type argument shows the state we will be in after the callback.
> If the state changes, there will be another callback. That's what a
> state-tracking tracer needs, e.g. to keep a little light on the screen red
> while the thread is stopped and green while it's running.
>
> The @notify argument shows what SIGCHLD the parent sees (if it were
> dequeuing all possible SIGCHLD postings as quickly as they come). That's
> what an event-tracking tracer needs, e.g. to match up with what SIGCHLDs
> are expected in the parent.
I see, thanks.
> > With the first patch, we call utrace_report_jctl() before we actually
> > stop. do_signal_stop() can fail then, but I think this is OK, we can
> > pretend that SIGCONT/SIGKILL happened after we stopped. It is not complete,
> > and with this patch we always call ->report_jctl with notify == 0. Just for
> > discussion.
>
> I think I sort of understand the intent of your patch. If we change the
> calling convention for tracehook_notify_jctl, I think we want to preserve
> the aspect that the hook decides about sending the notification. That's
> how the ptrace quirks can be reimplemented differently later without
> changing the tracehook layer again. Also, we certainly don't want one
> tracehook call with two different locking conditions.
Agreed, "bool sig_locked" is awful. But we can avoid it. The real problem
is how to figure out the correct "notify" argument. I'll try to think more,
but I am not sure I will find the clean solution :(
Just in case. We can introduce PF_SIGCONTED flag and change
prepare_signal(SIGCONT) and signal_wake_up(resume => 1) to set this flag.
Since the task never changes its ->flags in finish_stop() path, it is safe
to do this under ->siglock. This way utrace_report_jctl() can drop
TASK_STOPPED before REPORT() and then check !PF_SIGCONTED before restoring
the ->state. But yes sure, this is very, very ugly.
Oleg.
