Re: [PATCH] breakpoint always inserted in record target

[Pedro Alves <palves@redhat.com>]

On 04/23/2012 07:49 AM, Yao Qi wrote:>

> +static void
> +record_sync_record_breakpoints (struct bp_location *loc, void *data)
> +{
> +  /* Catchpoints and tracepoints should be skipped.
> +     TODO: something else should be done for watchpoints, and test cases
> +     are needed in testsuite/gdb.reverse for tracepoints, catchpoints, and
> +     watchpoints.  */


It's not clear to me that something _is_ indeed needed.  If it is, best
would be a PR.  If not needed, then it's better not to add this comment
in the first place, as these tend to remain in the sources forever, misleading.
IOW, please drop the comment.

> +  if (is_tracepoint (loc->owner) || is_catchpoint (loc->owner)

> +      || is_watchpoint (loc->owner))
> +      return;


This function should only concert itself about what it knows to handle, instead
of skip what it doesn't know about.  We don't want to need to add handling for
other types here if/when they're added to breakpoint.c in the future.

But also, it's really the location's type that matters, not the breakpoint's.
target_insert_breakpoint concerns itself with software breakpoints, so this
would be better:

  if (loc->loc_type != bp_loc_software_breakpoint)
    return;

Okay with these changes.

> +
> +  if (loc->inserted)
> +    {
> +      struct record_breakpoint *bp = XNEW (struct record_breakpoint);
> +
> +      bp->addr = loc->target_info.placed_address;
> +      bp->address_space = loc->target_info.placed_address_space;
> +
> +      bp->in_target_beneath = 1;
> +
> +      VEC_safe_push (record_breakpoint_p, record_breakpoints, bp);
> +    }
> +}

-- 
Pedro Alves