Re: [PATCH v4 3/9] add target method delegation

Pedro Alves
2013-11-08T17:08:30Z

On 10/29/2013 08:55 PM, Tom Tromey wrote:
> Tom> But looking more closely at the code on the branch, there is an
> Tom> assertion in those methods returning something other than void.
>
> Tom> I'll think about it some more.
>
> I looked at all the delegation functions today.
>
> I think it would be fine to make nearly all of them assert.
>
> The two exceptions are target_delegate_xfer_partial (already does not
> assert) and target_delegate_wait (which does assert but which I think
> should not).
>
> In all other cases there is either a de_fault call for the method, or
> the dummy target implements the method.

The de_fault only applies to current_target. As the delegation
always starts at ops->beneath, the de_fault shouldn't ever come into
play.

So target methods that do the beneath walk either have the choice
of having a default in the target method itself, or installing
it in the dummy target. Off hand, I don't think there's a real
behavioral difference. Looks like the sort of thing that could
be normalized.

>
> target_delegate_wait is a tricky one, as it returns a value. Perhaps
> just throwing an exception is best. The current code isn't much of a
> guide because it throws the exception when the record target is pushed
> -- but as noted in the thread, this is not robust as the target stack
> can change even after a target is pushed.

So to take that example, if we made dummy_target.to_wait be the
current to_wait default, which is to call noprocess(), then
it'd be clear that target_delegate_wait shouldn't ever go past
the loop, and then it'd be clear that an assertion is appropriate.

target_wait would then be:

ptid_t
target_wait (ptid_t ptid, struct target_waitstatus *status, int options)
{
struct target_ops *t;
ptid_t retval;

retval = target_delegate_wait (&current_traget, ptid, status, options);

if (targetdebug)
{
char *status_string;
char *options_string;

status_string = target_waitstatus_to_string (status);
options_string = target_options_to_string (options);
fprintf_unfiltered (gdb_stdlog,
"target_wait (%d, status, options={%s})"
" = %d, %s\n",
ptid_get_pid (ptid), options_string,
ptid_get_pid (retval), status_string);
xfree (status_string);
xfree (options_string);
}
}

WDYT?

>
> Your comments on this would be much appreciated.
>
>
> Some thoughts the target vector.
>
> I think the underlying problem here is complex, so it is reasonable if
> the model is as well. That is, I think it's fine to combine inheritance
> (e.g., the various linux-* vectors) with delegation (the whole stack
> itself plus special hacks in record and maybe elsewhere). That in
> itself is tractable.
>
> However, I think the combination of using INHERIT, plus de_fault, plus
> the dummy target, plus special wrappers for some target APIs leads to
> madness.
>
> It's much too hard to navigate this. I think we should adopt some
> simpler rule.

Yes, agreed. That's why I'm trying to see if we can reuse the
delegation with the public API.

--
Pedro Alves