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

[Pedro Alves <palves@redhat.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) (&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