Re: [RFC v2] fix regressions with target-async

[Pedro Alves <palves@redhat.com>]

On 02/28/2014 05:48 PM, Tom Tromey wrote:
> A patch in the target cleanup series caused a regression when using
> record with target-async.  This is a new version of this patch:
> 
>     https://sourceware.org/ml/gdb-patches/2014-02/msg00758.html
> 
> The immediate problem is that record supplies to_can_async_p and
> to_is_async_p methods, but does not supply a to_async method.  So,
> when target-async is set, record claims to support async -- but if the
> underlying target does not support async, then the to_async method
> call will end up in that method's default implementation, namely
> tcomplain.
> 
> This worked previously because the record target used to provide a
> to_async method; one that (erroneously, only at push time) checked the
> other members of the target stack, and then simply dropped to_async
> calls in the "does not implement async" case.
> 
> My first thought was to simply drop tcomplain as the default for
> to_async.  This works, but Pedro pointed out that the only reason
> record has to supply to_can_async_p and to_is_async_p is that these
> default to using the find_default_run_target machinery -- and these
> defaults are only needed by "run" and "attach".
> 
> So, a nicer solution presents itself: change run and attach to
> explicitly call into the default run target when needed; and change
> to_is_async_p and to_can_async_p to default to "return 0".  This makes
> the target stack simpler to use and lets us remove the method
> implementations from record.  This is also in harmony with other plans
> for the target stack; namely trying to reduce the impact of
> find_default_run_target.  This approach makes it clear that
> find_default_is_async_p is not needed -- it is asking whether a target
> that may not even be pushed is actually async, which seems like a
> nonsensical question.
> 
> While an improvement, this approach proved to introduce the same bug
> when using the core target.  Looking a bit deeper, the issue is that
> code in "attach" and "run" may need to use either the current target
> stack or the default run target -- but different calls into the target
> API in those functions could wind up querying different targets.
> 
> This new patch makes the target to use more explicit in "run" and
> "attach".  Then these commands explicitly make the needed calls
> against that target.  This ensures that a single target is used for
> all relevant operations.  This lets us remove a couple find_default_*
> functions from various targets, including the dummy target.  I think
> this is a decent understandability improvement.
> 
> One issue I see with this patch is that the new calls in "run" and
> "attach" are not very much like the rest of the target API.  I think
> fundamentally this is due to bad factoring in the target API, which
> may need to be fixed for multi-target.  Tackling that seemed ambitious
> for a regression fix.
> 

Looks like a nice solution to me.  I'm happy to see the find_default_...
default methods disappear.

OK.

For kicks, I wondered whether we had any target != process_stratum
that implemented to_attach / to_create_inferior, and lo, found
aix-thread.c...

/* Attach to process specified by ARGS.  */

static void
aix_thread_attach (struct target_ops *ops, char *args, int from_tty)
{
  struct target_ops *beneath = find_target_beneath (ops);

  beneath->to_attach (beneath, args, from_tty);
  pd_activate (1);
}

But this looks bogus to me -- it's only reachable if you already
have the target pushed because you're debugging a threaded core,
and then do "attach"...

-- 
Pedro Alves