This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Fix "is a record target open" checks.


>>>>> "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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]