On 10/27, Roland McGrath wrote:
>
> [I have no idea why you appended this to that message introducing patches
> that are not related to this at all. Please use separate threads for
> separate topics, and choose clearly apropos Subject: lines.]
OK.
> > I am thinking how can we fix utrace_control(SINGLESTEP). I don't
> > have good ideas so far. But, perhaps we can add
> > utrace->please_enable_single_step:1 ?
>
> If anything like this, it would be an enum utrace_resume_action field.
> Otherwise you are changing the API fundamentally.
Not sure I understand... This is like utrace->vfork_stop:1, it is
only visible to utrace code.
> > utrace_stop() and utrace_finish_jctl() can check this flag after wakeup
>
> What you mean here is an API where utrace_control's choice of resume_action
> takes effect without further callbacks when nothing else has requested any.
> As always, I want a discussion of the API semantics to be clear and
> separate from implementation details.
Yes. please see below.
> > Or, we can use ENGINE_SINGLESTEP, which probably makes more sense.
> > Like ENGINE_STOP, it lives both in engine->flags and ->utrace_flags.
>
> This takes us back to the old old utrace API where we had sticky state
> bits for the things that are now represented in utrace_resume_action.
> That is a very bad API choice IMHO, and its inherent unpleasantnesses
> are why we abandoned it before.
Agreed. I tried to think about ENGINE_SINGLESTEP more and came to the
same conclusion.
So. I am still not sure this is the best solution (in fact this doesn't
even look like a good solution to me), but afaics we can preserve the
current API and fix the problem if we add utrace->set_singlestep and
utrace->set_blockstep.
utrace_control()
case UTRACE_RESUME:
if (likely(reset)) {
user_disable_single_step(target);
utrace->set_singlestep = utrace->set_blockstep
= 0;
}
break;
case UTRACE_SINGLESTEP:
if (likely(reset))
utrace->set_singlestep = true;
else
ret = -EAGAIN;
break;
void utrace_finish_stop(...)
{
if (!(utrace->stop || utrace->set_singlestep))
return;
set_singlestep = utrace->set_singlestep;
spin_lock(&utrace->lock);
utrace->stopped = trace->set_singlestep = false;
spin_unlock(&utrace->lock);
if (set_singlestep)
user_enable_single_step(current);
}
utrace_finish_jctl() and utrace_stop() should call this new helper
instead of "if (utrace->stopped) {}" code.
> > I no longer think utrace_control() should just turn SINGLESTEP into
> > REPORT silently.
>
> IMHO this characterization misrepresents what the API is today, and
> glosses over the real complexities that are why it is that way.
>
> There is no "silent" about it. If things are going on that affect what
> you asked for, then you get a callback to decide what you really want.
> The inability to react to other engines' effects this way was one of the
> major failings of the old "sticky action bits" API.
Yes I see. And I agree, the current behaviour of utrace_control(SINGLESTEP)
which ->set_blockstep hack tries to preserve is not very good.
As for ptrace. If utrace_control(SINGLESTEP) doesn't set TIF_SINGLESTEP,
then we need more (probably nasty) changes. report_quiesce/interrupt should
somehow figure out whether we need send_sigtrap() if ->resume == XXXSTEP.
So. What do you think we should do for V1 ?
Oleg.
