This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH v4 3/9] add target method delegation
- From: Pedro Alves <palves at redhat dot com>
- To: Tom Tromey <tromey at redhat dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Fri, 08 Nov 2013 17:08:20 +0000
- Subject: Re: [PATCH v4 3/9] add target method delegation
- Authentication-results: sourceware.org; auth=none
- References: <1382464769-2465-1-git-send-email-tromey at redhat dot com> <1382464769-2465-4-git-send-email-tromey at redhat dot com> <526E8B54 dot 8040104 at redhat dot com> <87eh75cmig dot fsf at fleche dot redhat dot com> <87a9htcme3 dot fsf at fleche dot redhat dot com> <87habz7q6g dot fsf at fleche dot 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 (¤t_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