This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] Fix "is a record target open" checks.
- From: Tom Tromey <tromey at redhat dot com>
- To: Pedro Alves <palves at redhat dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Tue, 14 Jan 2014 08:30:50 -0700
- Subject: Re: [PATCH] Fix "is a record target open" checks.
- Authentication-results: sourceware.org; auth=none
- References: <1389640367-5571-1-git-send-email-tromey at redhat dot com> <1389640367-5571-3-git-send-email-tromey at redhat dot com> <52D523FD dot 8060205 at redhat dot com>
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
>> RECORD_IS_USED and find_record_target 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.
Pedro> This rationale doesn't look right for find_record_target.
Yeah, good point. I've changed the description on my branch. Now it
says:
RECORD_IS_USED looks 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 this, 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 that uses it more
heavily.
This new function lets us clean up find_record_target a bit as well.
I consider the change to find_record_target to be a minor cleanup.
It seemed cleaner to me to have this kind of target stack iteration
isolated in target.c when possible.
This mattered more on the multi-target branch, where I converted the
target stack to be an array rather than a linked list -- it made the
conversion simpler. However, I am not sure I am going to keep that
change.
Pedro> Instead, I'd rather apply a patch that fixes these record issues
Pedro> completely, and only does that. (IMO this one could go in
Pedro> immediately). WDYT?
I think it looks great, though I have a few nits :)
Pedro> +/* The shortnames of the record full targets. */
Pedro> +static char record_full_shortname[] = "record-full";
Pedro> +static char record_full_core_shortname[] = "record-core";
Pedro> +
Pedro> +/* See record-full.h. */
Pedro> +
Pedro> +int
Pedro> +record_full_is_used (void)
Pedro> +{
Pedro> + struct target_ops *t;
Pedro> +
Pedro> + t = find_record_target ();
Pedro> + return (t != NULL
Pedro> + && (t->to_shortname == record_full_shortname
Pedro> + || t->to_shortname == record_full_core_shortname));
Could this check against record_full_*ops directly rather than checking
the shortname? I'm curious about the rationale for this choice -- it's
all fine by me in the end, but if kept I think could use a comment.
Pedro> +void
Pedro> +record_preopen (void)
Pedro> +{
Need a "/* See record.h. */"
Pedro> + /* Check if a record target is already running. */
Pedro> + if (find_record_target ())
!= NULL
Pedro> +/* Find the record_stratum target in the target stack. */
Pedro> +extern struct target_ops *find_record_target (void);
I think this should mention that it can return NULL.
Tom