utrace_control(,,UTRACE_SINGLESTEP)

[Oleg Nesterov]

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.