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: Mon, 28 Oct 2013 16:05:40 +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>
On 10/22/2013 06:59 PM, Tom Tromey wrote:
> This patch replaces some code in the record targets with target method
> delegation.
>
> Right now there are two latent problems in the record target.
>
> First, record-full.c stores pointers to many target methods when the
> record target is pushed. Then it later delegates some calls via
> these. This is wrong because it violates the target stack contract.
> In particular it is ok to unpush a target at any stratum, but
> record-full does not keep track of this, so it could potentially call
> into an unpushed target.
>
> Second, RECORD_IS_USED and some other spots look at
> current_target.to_stratum to determine whether a record target is in
> use. This is bad because arch_stratum is greater than record_stratum.
>
> To fix the first problem, this patch introduces a handful of
> target_delegate_* functions, which forward calls further down the
> target stack.
>
> To fix the second problem, this patch adds find_target_at to determine
> whether a target appears at a given stratum. This may seem like
> overkill somehow, but I have a subsequent patch series (see archer.git
> tromey/multi-target) that uses it more heavily.
Hmm, I think this is trying to do too much at once.
Could you please split out the patch for the second problem? I
think it'll be a small one.
Then changes like these:
> #define target_stopped_by_watchpoint() \
> - ((*current_target.to_stopped_by_watchpoint) (¤t_target))
> + (target_delegate_stopped_by_watchpoint (¤t_target))
> +
> +/* Delegate "target_stopped_by_watchpoint" to a target beneath SELF. */
> +
> +extern int target_delegate_stopped_by_watchpoint (struct target_ops *self);
switch from using the inheritance scheme to the run-time walk mechanism,
but leave update_current_target still doing the INHERIT/de_fault business.
What's the plan for the existing target methods that
currently already do a similar beneath lookup? I'd like it that
there's be at least a plan, so we don't end up with yet another
way of doing things, two incomplete transitions, and no clear direction.
> + gdb_assert_not_reached (_("reached end of target stack during delegation"));
> +}
This appears in several places. Whenever I see the same string
repeated over and over, I tend to think it'd be good to add a
utility helper function:
static void
assert_end_of_stack_not_reached_or_something (void)
{
gdb_assert_not_reached (_("reached end of target stack during delegation"));
}
Some of the delegation methods have that assert, while others don't
have anything at the tail end. What's the story there?
> +
> +int
> +target_delegate_insert_breakpoint (struct target_ops *self,
> + struct gdbarch *gdbarch,
> + struct bp_target_info *bp_tgt)
> +{
> + struct target_ops *t;
> +
> + for (t = self->beneath; t != NULL; t = t->beneath)
> + {
> + if (t->to_insert_breakpoint)
if (t->to_insert_breakpoint != NULL)
--
Pedro Alves