> But not vise versa. I misunderstood the comment as if the new engine
> should not be notified if it is attached by another task while target
> is inside callback.
That is indeed what happens in that case. But that one is not a
specific "should not", it's just what happens to be true given what we
say about the "asynchronous" attach case in general. That is, that an
"asynchronous" attach + set_events makes no guarantees about how
instantly you start to get event reports. It might be as long as the
time it takes to get back to user mode from whereever the thread is
now, or the time it takes it to process an interrupt and then get back
to user mode. It's like you did "thread->events |= events" but there
has not been any kind of memory barrier--it might see it or might not,
until you do something affirmative to make sure (i.e. put it through
UTRACE_STOP, or else get some other callback you're sure happens after
your utrace_set_events call).
For this purpose an "asynchronous attach" means one by a third task (not
the thread itself or the creator during its report_clone), and done when
that third task did not already have some engine that completed a UTRACE_STOP.
This applies even if it is literally synchronous, i.e. if a callback
arranged for the third task to do the attach and set_events and then
blocked waiting for the third task to report its success, we'd call this an
"asynchronous attach" because it didn't synchronize using UTRACE_STOP.
> Still can't understand... If (say) ->report_exec() attaches the new
> engine to the same task and does utrace_set_events(EXEC), then it looks
> logical the new engine gets the notification too. But OK, I agree, either
> way is correct, and perhaps the current behaviour is more intuitive.
As you can see in the cited thread, that's what I thought too. Jim
convinced me that the (new) current behavior is more useful. The most
important thing to me is that it's clearly specified one way or the
other for the synchronous case. It's obviously straightforward to do:
report_exec(engine, ...)
{
new_engine = utrace_attach_task(current, &new_ops);
utrace_set_events(new_engine, UTRACE_EVENT(EXEC));
new_ops.report_exec(new_engine, ...);
}
if you want one of your own callback functions to get another call
there. OTOH, it's much more cumbersome to make the report_exec
callback used by your new engine keep flags and whatnot to distinguish
the first exec event that preceded that engine's setup from the next
one (which is what the new engine is really there to respond to).
Jim's use seems fairly representative of situations where this might
come up. He's concerned with the EXEC event as the "old address space
is gone, new one is here" event. It's also the "my name changed"
event that may be triggering a new tracing setup. The former use just
wants report_exec to do "wipe out our state and go away" stuff. The
latter use might want to set up a new incarnation of that sort of
tracing setup--a new engine whose report_exec callback does clean up.
It's obvious how the new engine getting its "clean up now" callback
immediately as a consequence of where the call to set it up came from
is not helpful. I'm sure this sort of scenario will not be unique
either to Jim's work or to EXEC callbacks in particular.
> But this means that "When target == current it would be safe just to call
> splice_attaching() right here" part of the comment is not right, no?
> Except for report_reap() target == current.
It would be "safe", meaning it doesn't have race problems like the
target != current case does for touching ->attached here. That's what
the comment says (and that's what the code used to do). The reason we
don't do it (any more) is the explicit choice for API semantics, not
any implementation reason (in the implementation it is indeed an
obvious optimization if you are understanding the code). That's why
the comment is there.
> Yes, yes, I see. But I meant another case. Suppose that the debugger D
> attaches to T and does
>
> engine = utrace_attach_task(T, ...);
> utrace_set_events(T, engine, XXX);
>
> It is possible that ->report_xxx() is called before utrace_set_events()
> completes. But afaics currently this is not a problem.
As far as the API guarantees are concerned, there is no "completes".
When you call utrace_set_events, it becomes possible your callbacks
get made. The return value (a failure return, not -EINPROGRESS) can
say that you are now sure no callback was made or will be. But when
you called, you wanted it to be possible. If you didn't, then you
should have made sure it was fully stopped via UTRACE_STOP before you
called utrace_set_events.
Thanks,
Roland
