This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFC v2] fix regressions with target-async
- 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, 28 Feb 2014 18:18:33 +0000
- Subject: Re: [RFC v2] fix regressions with target-async
- Authentication-results: sourceware.org; auth=none
- References: <1393609699-26407-1-git-send-email-tromey at redhat dot 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