[PATCH] breakpoint always inserted in record target

[PATCH] breakpoint always inserted in record target

[Yao Qi]

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?

gdb:

2012-04-11  Yao Qi  <yao@codesourcery.com>

	* breakpoint.c (iterate_over_bp_locations): New.
	* breakpoint.h: Declare.
	* record.c (record_open): Call record_init_record_breakpoints.
	(record_sync_record_breakpoints): New.
	(record_init_record_breakpoints): New.
---
 gdb/breakpoint.c |   13 +++++++++++++
 gdb/breakpoint.h |    2 ++
 gdb/record.c     |   30 ++++++++++++++++++++++++++++++
 3 files changed, 45 insertions(+), 0 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index be536bc..c5109d6 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -2425,6 +2425,19 @@ insert_breakpoints (void)
     insert_breakpoint_locations ();
 }
 
+/* Invoke CALLBACK for each of bp_location.  */
+
+void
+iterate_over_bp_locations (void (*callback) (struct bp_location *))
+{
+  struct bp_location *loc, **loc_tmp;
+
+  ALL_BP_LOCATIONS (loc, loc_tmp)
+    {
+      callback (loc);
+    }
+}
+
 /* This is used when we need to synch breakpoint conditions between GDB and the
    target.  It is the case with deleting and disabling of breakpoints when using
    always-inserted mode.  */
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index e0eeeaa..3fe6a63 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -1131,6 +1131,8 @@ extern void delete_breakpoint (struct breakpoint *);
 
 extern void breakpoint_auto_delete (bpstat);
 
+extern void iterate_over_bp_locations (void (*callback) (struct bp_location *));
+
 /* Return the chain of command lines to execute when this breakpoint
    is hit.  */
 extern struct command_line *breakpoint_commands (struct breakpoint *b);
diff --git a/gdb/record.c b/gdb/record.c
index 9b7ee2f..3429d83 100644
--- a/gdb/record.c
+++ b/gdb/record.c
@@ -896,6 +896,8 @@ record_open_1 (char *name, int from_tty)
   push_target (&record_ops);
 }
 
+static void record_init_record_breakpoints (void);
+
 /* "to_open" target method.  Open the process record target.  */
 
 static void
@@ -993,6 +995,8 @@ record_open (char *name, int from_tty)
   record_async_inferior_event_token
     = create_async_event_handler (record_async_inferior_event_handler,
 				  NULL);
+
+  record_init_record_breakpoints ();
 }
 
 /* "to_close" target method.  Close the process record target.  */
@@ -1744,6 +1748,32 @@ DEF_VEC_P(record_breakpoint_p);
    active.  */
 VEC(record_breakpoint_p) *record_breakpoints = NULL;
 
+static void
+record_sync_record_breakpoints (struct bp_location *loc)
+{
+  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);
+    }
+}
+
+/* Sync existing breakpoints to record_breakpoints.  */
+
+static void
+record_init_record_breakpoints (void)
+{
+  VEC_free (record_breakpoint_p, record_breakpoints);
+
+  iterate_over_bp_locations (record_sync_record_breakpoints);
+}
+
 /* Behavior is conditional on RECORD_IS_REPLAY.  We will not actually
    insert or remove breakpoints in the real target when replaying, nor
    when recording.  */
-- 
1.7.0.4

Re: [PATCH] breakpoint always inserted in record target

[Keith Seitz]

I don't normally chime in on patches, but I felt compelled to ask for 
one change in your patch (which otherwise looks okay to me):

On 04/11/2012 06:06 AM, Yao Qi wrote:
 >
> +/* Invoke CALLBACK for each of bp_location.  */
> +
> +void
> +iterate_over_bp_locations (void (*callback) (struct bp_location *))
> +{
> +  struct bp_location *loc, **loc_tmp;
> +
> +  ALL_BP_LOCATIONS (loc, loc_tmp)
> +    {
> +      callback (loc);
> +    }
> +}
> +

Can you please define a typedef for this CALLBACK parameter?

Keith

Re: [PATCH] breakpoint always inserted in record target

[Yao Qi]

[-- Attachment #1: Type: text/plain, Size: 797 bytes --]

On 04/12/2012 01:32 AM, Keith Seitz wrote:
> I don't normally chime in on patches, but I felt compelled to ask for
> one change in your patch (which otherwise looks okay to me):
> 
> On 04/11/2012 06:06 AM, Yao Qi wrote:
>>
>> +/* Invoke CALLBACK for each of bp_location.  */
>> +
>> +void
>> +iterate_over_bp_locations (void (*callback) (struct bp_location *))
>> +{
>> +  struct bp_location *loc, **loc_tmp;
>> +
>> +  ALL_BP_LOCATIONS (loc, loc_tmp)
>> +    {
>> +      callback (loc);
>> +    }
>> +}
>> +
> 
> Can you please define a typedef for this CALLBACK parameter?
> 

This updated patch addresses both Keith's and Doug's comments.  I
realize that this change is user visible, so add one entry in NEWS.

If no objections, I'll check it in in three or four days.

-- 
Yao (齐尧)

[-- Attachment #2: 0001-breakpoint-always-inserted-in-record-target.patch --]
[-- Type: text/x-patch, Size: 3768 bytes --]

gdb:

2012-04-12  Yao Qi  <yao@codesourcery.com>

	* breakpoint.c (iterate_over_bp_locations): New.
	* breakpoint.h: Declare.
	New typedef walk_bp_location_callback.
	* record.c (record_open): Call record_init_record_breakpoints.
	(record_sync_record_breakpoints): New.
	(record_init_record_breakpoints): New.
	* NEWS: Mention supporting breakpoint always-inserted mode in
	record target.
---
 gdb/NEWS         |    2 ++
 gdb/breakpoint.c |   13 +++++++++++++
 gdb/breakpoint.h |    4 ++++
 gdb/record.c     |   32 ++++++++++++++++++++++++++++++++
 4 files changed, 51 insertions(+), 0 deletions(-)

diff --git a/gdb/NEWS b/gdb/NEWS
index c287ffa..9b190e4 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -82,6 +82,8 @@
 
 * Ada support for GDB/MI Variable Objects has been added.
 
+* GDB can now support breakpoint always-inserted mode in record target.
+
 * New commands
 
   ** "catch load" and "catch unload" can be used to stop when a shared
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index be536bc..0df0a81 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -2425,6 +2425,19 @@ insert_breakpoints (void)
     insert_breakpoint_locations ();
 }
 
+/* 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);
+    }
+}
+
 /* This is used when we need to synch breakpoint conditions between GDB and the
    target.  It is the case with deleting and disabling of breakpoints when using
    always-inserted mode.  */
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index e0eeeaa..c0b1bad 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -1131,6 +1131,10 @@ extern void delete_breakpoint (struct breakpoint *);
 
 extern void breakpoint_auto_delete (bpstat);
 
+typedef void (*walk_bp_location_callback) (void *data);
+
+extern void iterate_over_bp_locations (walk_bp_location_callback);
+
 /* Return the chain of command lines to execute when this breakpoint
    is hit.  */
 extern struct command_line *breakpoint_commands (struct breakpoint *b);
diff --git a/gdb/record.c b/gdb/record.c
index 9b7ee2f..946ca95 100644
--- a/gdb/record.c
+++ b/gdb/record.c
@@ -896,6 +896,8 @@ record_open_1 (char *name, int from_tty)
   push_target (&record_ops);
 }
 
+static void record_init_record_breakpoints (void);
+
 /* "to_open" target method.  Open the process record target.  */
 
 static void
@@ -993,6 +995,8 @@ record_open (char *name, int from_tty)
   record_async_inferior_event_token
     = create_async_event_handler (record_async_inferior_event_handler,
 				  NULL);
+
+  record_init_record_breakpoints ();
 }
 
 /* "to_close" target method.  Close the process record target.  */
@@ -1744,6 +1748,34 @@ DEF_VEC_P(record_breakpoint_p);
    active.  */
 VEC(record_breakpoint_p) *record_breakpoints = NULL;
 
+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);
+    }
+}
+
+/* Sync existing breakpoints to record_breakpoints.  */
+
+static void
+record_init_record_breakpoints (void)
+{
+  VEC_free (record_breakpoint_p, record_breakpoints);
+
+  iterate_over_bp_locations (record_sync_record_breakpoints);
+}
+
 /* Behavior is conditional on RECORD_IS_REPLAY.  We will not actually
    insert or remove breakpoints in the real target when replaying, nor
    when recording.  */
-- 
1.7.0.4

Re: [PATCH] breakpoint always inserted in record target

[Eli Zaretskii]

> Date: Fri, 13 Apr 2012 09:39:16 +0800
> From: Yao Qi <yao@codesourcery.com>
> CC: Eli Zaretskii <eliz@gnu.org>
> 
> +* GDB can now support breakpoint always-inserted mode in record target.

'breakpoint always-inserted mode' and 'record', with quotes, please.
These are proper names in GDB.

Otherwise, OK.

Thanks.

Re: [PATCH] breakpoint always inserted in record target

[Pedro Alves]

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

Re: [PATCH] breakpoint always inserted in record target

[Yao Qi]

[-- Attachment #1: Type: text/plain, Size: 3629 bytes --]

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 (齐尧)

[-- Attachment #2: 0002-breakpoint-always-inserted-in-record-target.patch --]
[-- Type: text/x-patch, Size: 5015 bytes --]

gdb:
2012-04-23  Yao Qi  <yao@codesourcery.com>

	Revert this patch to allow breakpoint always-inserted
	in record target.
	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.

	* breakpoint.c (iterate_over_bp_locations): New.
	* breakpoint.h: Declare.
	New typedef walk_bp_location_callback.
	* record.c (record_open): Call record_init_record_breakpoints.
	(record_sync_record_breakpoints): New.
	(record_init_record_breakpoints): New.
	* NEWS: Mention supporting breakpoint always-inserted mode in
	record target.
---
 gdb/NEWS         |    3 +++
 gdb/breakpoint.c |   19 +++++++++++++++----
 gdb/breakpoint.h |    4 ++++
 gdb/record.c     |   38 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 60 insertions(+), 4 deletions(-)

diff --git a/gdb/NEWS b/gdb/NEWS
index 72b4d90..72ce6b8 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -82,6 +82,9 @@
 
 * Ada support for GDB/MI Variable Objects has been added.
 
+* GDB can now support 'breakpoint always-inserted mode' in 'record'
+  target.
+
 * New commands
 
   ** "catch load" and "catch unload" can be used to stop when a shared
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index ceca221..4204e36 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -64,7 +64,6 @@
 #include "continuations.h"
 #include "stack.h"
 #include "skip.h"
-#include "record.h"
 #include "gdb_regex.h"
 #include "ax-gdb.h"
 
@@ -404,9 +403,8 @@ show_always_inserted_mode (struct ui_file *file, int from_tty,
 int
 breakpoints_always_inserted_mode (void)
 {
-  return ((always_inserted_mode == always_inserted_on
-	   || (always_inserted_mode == always_inserted_auto && non_stop))
-	  && !RECORD_IS_USED);
+  return (always_inserted_mode == always_inserted_on
+	  || (always_inserted_mode == always_inserted_auto && non_stop));
 }
 
 static const char condition_evaluation_both[] = "host or target";
@@ -2425,6 +2423,19 @@ insert_breakpoints (void)
     insert_breakpoint_locations ();
 }
 
+/* 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, NULL);
+    }
+}
+
 /* This is used when we need to synch breakpoint conditions between GDB and the
    target.  It is the case with deleting and disabling of breakpoints when using
    always-inserted mode.  */
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index e676659..75b62d2 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -1131,6 +1131,10 @@ extern void delete_breakpoint (struct breakpoint *);
 
 extern void breakpoint_auto_delete (bpstat);
 
+typedef void (*walk_bp_location_callback) (struct bp_location *, void *);
+
+extern void iterate_over_bp_locations (walk_bp_location_callback);
+
 /* Return the chain of command lines to execute when this breakpoint
    is hit.  */
 extern struct command_line *breakpoint_commands (struct breakpoint *b);
diff --git a/gdb/record.c b/gdb/record.c
index 9b7ee2f..08e5958 100644
--- a/gdb/record.c
+++ b/gdb/record.c
@@ -896,6 +896,8 @@ record_open_1 (char *name, int from_tty)
   push_target (&record_ops);
 }
 
+static void record_init_record_breakpoints (void);
+
 /* "to_open" target method.  Open the process record target.  */
 
 static void
@@ -993,6 +995,8 @@ record_open (char *name, int from_tty)
   record_async_inferior_event_token
     = create_async_event_handler (record_async_inferior_event_handler,
 				  NULL);
+
+  record_init_record_breakpoints ();
 }
 
 /* "to_close" target method.  Close the process record target.  */
@@ -1744,6 +1748,40 @@ DEF_VEC_P(record_breakpoint_p);
    active.  */
 VEC(record_breakpoint_p) *record_breakpoints = NULL;
 
+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.  */
+  if (is_tracepoint (loc->owner) || is_catchpoint (loc->owner)
+      || is_watchpoint (loc->owner))
+      return;
+
+  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);
+    }
+}
+
+/* Sync existing breakpoints to record_breakpoints.  */
+
+static void
+record_init_record_breakpoints (void)
+{
+  VEC_free (record_breakpoint_p, record_breakpoints);
+
+  iterate_over_bp_locations (record_sync_record_breakpoints);
+}
+
 /* Behavior is conditional on RECORD_IS_REPLAY.  We will not actually
    insert or remove breakpoints in the real target when replaying, nor
    when recording.  */
-- 
1.7.0.4

Re: [PATCH] breakpoint always inserted in record target

[Pedro Alves]

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

[committed]: [PATCH] breakpoint always inserted in record target

[Yao Qi]

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

Comment is removed.  I'll file some PR if I can expose them from
 existing test cases in gdb.reverse/

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

> Okay with these changes.
> 

This is what I committed.
http://sourceware.org/ml/gdb-cvs/2012-04/msg00191.html

-- 
Yao (齐尧) 

2012-04-24  Yao Qi  <yao@codesourcery.com>

	Revert this patch to allow breakpoint always-inserted
	in record target.
	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.

	* breakpoint.c (iterate_over_bp_locations): New.
	* breakpoint.h: Declare.
	New typedef walk_bp_location_callback.
	* record.c (record_open): Call record_init_record_breakpoints.
	(record_sync_record_breakpoints): New.
	(record_init_record_breakpoints): New.
	* NEWS: Mention supporting breakpoint always-inserted mode in
	record target.
---
 gdb/NEWS         |    3 +++
 gdb/breakpoint.c |   19 +++++++++++++++----
 gdb/breakpoint.h |    4 ++++
 gdb/record.c     |   33 +++++++++++++++++++++++++++++++++
 4 files changed, 55 insertions(+), 4 deletions(-)

diff --git a/gdb/NEWS b/gdb/NEWS
index 72b4d90..72ce6b8 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -82,6 +82,9 @@
 
 * Ada support for GDB/MI Variable Objects has been added.
 
+* GDB can now support 'breakpoint always-inserted mode' in 'record'
+  target.
+
 * New commands
 
   ** "catch load" and "catch unload" can be used to stop when a shared
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index ceca221..4204e36 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -64,7 +64,6 @@
 #include "continuations.h"
 #include "stack.h"
 #include "skip.h"
-#include "record.h"
 #include "gdb_regex.h"
 #include "ax-gdb.h"
 
@@ -404,9 +403,8 @@ show_always_inserted_mode (struct ui_file *file, int from_tty,
 int
 breakpoints_always_inserted_mode (void)
 {
-  return ((always_inserted_mode == always_inserted_on
-	   || (always_inserted_mode == always_inserted_auto && non_stop))
-	  && !RECORD_IS_USED);
+  return (always_inserted_mode == always_inserted_on
+	  || (always_inserted_mode == always_inserted_auto && non_stop));
 }
 
 static const char condition_evaluation_both[] = "host or target";
@@ -2425,6 +2423,19 @@ insert_breakpoints (void)
     insert_breakpoint_locations ();
 }
 
+/* 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, NULL);
+    }
+}
+
 /* This is used when we need to synch breakpoint conditions between GDB and the
    target.  It is the case with deleting and disabling of breakpoints when using
    always-inserted mode.  */
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index e676659..75b62d2 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -1131,6 +1131,10 @@ extern void delete_breakpoint (struct breakpoint *);
 
 extern void breakpoint_auto_delete (bpstat);
 
+typedef void (*walk_bp_location_callback) (struct bp_location *, void *);
+
+extern void iterate_over_bp_locations (walk_bp_location_callback);
+
 /* Return the chain of command lines to execute when this breakpoint
    is hit.  */
 extern struct command_line *breakpoint_commands (struct breakpoint *b);
diff --git a/gdb/record.c b/gdb/record.c
index 9b7ee2f..7db1527 100644
--- a/gdb/record.c
+++ b/gdb/record.c
@@ -896,6 +896,8 @@ record_open_1 (char *name, int from_tty)
   push_target (&record_ops);
 }
 
+static void record_init_record_breakpoints (void);
+
 /* "to_open" target method.  Open the process record target.  */
 
 static void
@@ -993,6 +995,8 @@ record_open (char *name, int from_tty)
   record_async_inferior_event_token
     = create_async_event_handler (record_async_inferior_event_handler,
 				  NULL);
+
+  record_init_record_breakpoints ();
 }
 
 /* "to_close" target method.  Close the process record target.  */
@@ -1744,6 +1748,35 @@ DEF_VEC_P(record_breakpoint_p);
    active.  */
 VEC(record_breakpoint_p) *record_breakpoints = NULL;
 
+static void
+record_sync_record_breakpoints (struct bp_location *loc, void *data)
+{
+  if (loc->loc_type != bp_loc_software_breakpoint)
+      return;
+
+  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);
+    }
+}
+
+/* Sync existing breakpoints to record_breakpoints.  */
+
+static void
+record_init_record_breakpoints (void)
+{
+  VEC_free (record_breakpoint_p, record_breakpoints);
+
+  iterate_over_bp_locations (record_sync_record_breakpoints);
+}
+
 /* Behavior is conditional on RECORD_IS_REPLAY.  We will not actually
    insert or remove breakpoints in the real target when replaying, nor
    when recording.  */
-- 
1.7.0.4

Re: [PATCH] breakpoint always inserted in record target

[Doug Evans]

On Wed, Apr 11, 2012 at 6:06 AM, Yao Qi <yao@codesourcery.com> wrote:
> 2012-04-11  Yao Qi  <yao@codesourcery.com>
>
>        * breakpoint.c (iterate_over_bp_locations): New.
>        * breakpoint.h: Declare.
>        * record.c (record_open): Call record_init_record_breakpoints.
>        (record_sync_record_breakpoints): New.
>        (record_init_record_breakpoints): New.
> ---
>  gdb/breakpoint.c |   13 +++++++++++++
>  gdb/breakpoint.h |    2 ++
>  gdb/record.c     |   30 ++++++++++++++++++++++++++++++
>  3 files changed, 45 insertions(+), 0 deletions(-)
>
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index be536bc..c5109d6 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -2425,6 +2425,19 @@ insert_breakpoints (void)
>     insert_breakpoint_locations ();
>  }
>
> +/* Invoke CALLBACK for each of bp_location.  */
> +
> +void
> +iterate_over_bp_locations (void (*callback) (struct bp_location *))
> +{
> +  struct bp_location *loc, **loc_tmp;
> +
> +  ALL_BP_LOCATIONS (loc, loc_tmp)
> +    {
> +      callback (loc);
> +    }
> +}

Hi.
Nit: Functions that take callbacks generally also take a void* to pass
to the callback.

Plus, can you write a testcase?

Re: [PATCH] breakpoint always inserted in record target

[Yao Qi]

On 04/12/2012 12:45 PM, Doug Evans wrote:
> Plus, can you write a testcase?

In existing test cases in gdb.reverse, breakpoints are inserted into
inferior.  Without this patch, when run gdb.reverse/i386-sse-reverse.exp
(turning on always inserted mode), we get a lot fails.  When this patch
applied, fails go away.  I think this patch is exercised by this case.
I should mention this fixed fails in my patch post.

What do you want to test beyond my description above?

-- 
Yao (齐尧)

Re: [PATCH] breakpoint always inserted in record target

[Doug Evans]

On Wed, Apr 11, 2012 at 10:16 PM, Yao Qi <yao@codesourcery.com> wrote:
> On 04/12/2012 12:45 PM, Doug Evans wrote:
>> Plus, can you write a testcase?
>
> In existing test cases in gdb.reverse, breakpoints are inserted into
> inferior.  Without this patch, when run gdb.reverse/i386-sse-reverse.exp
> (turning on always inserted mode), we get a lot fails.  When this patch
> applied, fails go away.  I think this patch is exercised by this case.
> I should mention this fixed fails in my patch post.
>
> What do you want to test beyond my description above?

No that's sufficient.
I don't want to unnecessarily expand the scope of your patch.

[for reference sake]
IWBN if we didn't have to run the testsuite in multiple ways as much
as we do now.

Re: [PATCH] breakpoint always inserted in record target

[Tom Tromey]

>>>>> "Doug" == Doug Evans <dje@google.com> writes:

Doug> IWBN if we didn't have to run the testsuite in multiple ways as much
Doug> as we do now.

Yeah.  Or if we had a public buildbot to do it for us.
I did set up a buildbot internally here, and I can share the code if
anybody wants to take it over.  It probably bit-rotted a little; it was
taken out during an office move and I haven't set it back up.

Tom