On 04/13/2012 11:37 PM, Pedro Alves wrote:
> 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.  :-)
> 

OK, revert it in this new version.

> 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);
> 
> 

Got you.  Fixed.

>> > +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.

In this new version, catchpoints, tracepoints, and watchpoints are
skipped.  Current test cases in gdb.reverse/ doesn't cover the situation
that we have catchpoints/tracepoints/watchpoints installed before
switching to target record, and verify these points still work as
expected in record target.  This can be addressed by a separate patch.

Tested this new version on x86_64-linux in native gdb and
native-gdbserver.  No regression.

This version relies on this pending patch,

  [PATCH] ep_is_catchpoint -> is_catchpoint
  http://sourceware.org/ml/gdb-patches/2012-04/msg00731.html

-- 
Yao (齐尧)