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

Pedro Alves
2013-10-28T16:05:44Z

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) (&current_target))
> + (target_delegate_stopped_by_watchpoint (&current_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