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

[Pedro Alves <palves at redhat dot com>]

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