Re: [PATCH] breakpoint always inserted in record target

[Pedro Alves <palves@redhat.com>]

On 04/11/2012 02:06 PM, Yao Qi wrote:
> So far, breakpoint always-inserted is not supported in target record.
> This patch is to teach GDB to support breakpoint always-inserted in
> record.  When switch to target record, the existing breakpoints are
> unknown to record target, so this causes errors when removing breakpoint.
> In this patch, we sync existing breakpoints to record_breakpoints
> when change to record target.
>
> Tested on native gdb and remote with native gdbserver, with setting this
> line in testsuite/site.exp
>
>   set GDBFLAGS "-ex \"set breakpoint always-inserted on\""
>
> `record' is new to me, am I missing something else to support break
> aways-inserted in record?

Thanks.

Indeed this is a missing step.  I see we're now internal-erroring,
which I missed at:
<http://sourceware.org/ml/gdb-patches/2012-01/msg00653.html>

We weren't before, always-inserted mode would just get ignored.
So one thing you're missing is to revert 9572dfe7e9b04046543bd2007f3beb11a9049ba4 :

2011-12-05  Pedro Alves  <pedro@codesourcery.com>

        * breakpoint.c: Include record.h.
        (breakpoints_always_inserted_mode): Return false when the record
        target is in use.

as otherwise the record target doesn't really do always inserted.  :-)

On 04/13/2012 02:39 AM, Yao Qi wrote:

> +/* Invoke CALLBACK for each of bp_location.  */
> +
> +void
> +iterate_over_bp_locations (walk_bp_location_callback callback)
> +{
> +  struct bp_location *loc, **loc_tmp;
> +
> +  ALL_BP_LOCATIONS (loc, loc_tmp)
> +    {
> +      callback (loc);
> +    }
> +}
> +


>  extern void breakpoint_auto_delete (bpstat);
>  
> +typedef void (*walk_bp_location_callback) (void *data);
> +
> +extern void iterate_over_bp_locations (walk_bp_location_callback);
> +


> +static void

> +record_sync_record_breakpoints (void *data)
> +{
> +  struct bp_location *loc = (struct bp_location *) data;



You misunderstood Doug's comment.  Functions that take callbacks
generally _also_ take a void* to pass to the callback, in addition
to the iterated object.

extern struct breakpoint *iterate_over_breakpoints (int (*) (struct breakpoint *,
                                                             void *), void *);

struct thread_info *
iterate_over_threads (int (*callback) (struct thread_info *, void *),
                      void *data)

etc.  So you'd have:

static void
record_sync_record_breakpoints (struct bp_location *bploc, void *data)
{

and you'd call the iteration function like:

iterate_over_bp_locations (record_sync_record_breakpoints, NULL);

or

iterate_over_bp_locations (some_other_callback_that_needs_additional_data, &additional_data);


> +static void
> +record_sync_record_breakpoints (void *data)
> +{
> +  struct bp_location *loc = (struct bp_location *) data;
> +
> +  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);
> +    }
> +}
> +

This creates a struct record_breakpoint for locations that aren't
really breakpoints.  E.g., what about catchpoints and tracepoints?
At least those should be skipped.  Haven't thought properly if
we need to do something else for watchpoints too.

-- 
Pedro Alves