[patch] Add watchpoint support to save_breakpoints.py (and add supporting methods to python-breakpoint.c)

[patch] Add watchpoint support to save_breakpoints.py (and add supporting methods to python-breakpoint.c)

[Phil Muldoon]

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

Something that has been on my mind for awhile is that 
save_breakpoints.py does not handle watchpoints very well.  I like 
watchpoints, and use them all the time! So I propose this patch:

This patch adds watchpoint support to the save_breakpoints.py command.  
I added two new methods to python_breakpoint.c to achieve this goal.  As 
watchpoints only seemed to be represented as an expression, then the 
bppy_get_expression function had to be included to interrogate the 
expression behind the gdb breakpoint.  To differentiate between the 
types of watchpoint (and other breakpoints ...), I added a bppy_get_type 
function along with supporting constants.  Included are documentation 
entries also.  I elected not to include catchpoint support as catchpoint 
"type" seems to be hidden behind the struct bp_ops function pointers in 
the breakpoint struct. If there is another way to differentiate between 
the different catchpoints by type, I'd be happy to add them into this 
patch, too.

Regards

Phil

ChangeLog

2009-06-11  Phil Muldoon <pmuldoon@redhat.com>

     * python/python-breakpoint.c (bppy_get_expression): New Function.
     (bppy_get_type): Likewise.
     (gdbpy_initialize_breakpoints): Initialize breakpoint constants.
     * python/lib/gdb/command/save_breakpoints.py
     (SaveBreakpointsCommand.breakpoint_type): New function.
     (SaveBreakpointsCommand.invoke): Use breakpoint_type.

doc/ChangeLog

2009-06-11  Phil Muldoon <pmuldoon@redhat.com>

     * gdb.texinfo (Breakpoints In Python): Add expression, type
     variable text. Add breakpoint constants.



[-- Attachment #2: py_watchpoints.patch --]
[-- Type: text/plain, Size: 6860 bytes --]

diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 958a74f..ee2634b 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -19511,6 +19511,42 @@ This attribute holds the breakpoint's number -- the identifier used by
 the user to manipulate the breakpoint.  This attribute is not writable.
 @end defivar
 
+@defivar Breakpoint type
+This attribute holds the breakpoint's type -- the identifier used to
+determine the actual breakpoint type or use-case.  This attribute is not
+writable.
+@end defivar
+
+The available types are represented by constants defined in the @code{gdb}
+module:
+
+@table @code
+@findex BP_BREAKPOINT
+@findex gdb.BP_BREAKPOINT
+@item BP_BREAKPOINT
+Normal code breakpoint.
+
+@findex BP_WATCHPOINT
+@findex gdb.BP_WATCHPOINT
+@item BP_WATCHPOINT
+Watchpoint breakpoint.
+
+@findex BP_HARDWARE_WATCHPOINT
+@findex gdb.BP_HARDWARE_WATCHPOINT
+@item BP_HARDWARE_WATCHPOINT
+Hardware assisted watchpoint.
+
+@findex BP_READ_WATCHPOINT
+@findex gdb.BP_READ_WATCHPOINT
+@item BP_READ_WATCHPOINT
+Hardware assisted read watchpoint.
+
+@findex BP_ACCESS_WATCHPOINT
+@findex gdb.BP_ACCESS_WATCHPOINT
+@item BP_ACCESS_WATCHPOINT
+Hardware assisted access watchpoint.
+@end table
+
 @defivar Breakpoint hit_count
 This attribute holds the hit count for the breakpoint, an integer.
 This attribute is writable, but currently it can only be set to zero.
@@ -19521,6 +19557,11 @@ This attribute holds the location of the breakpoint, as specified by
 the user.  It is a string.  This attribute is not writable.
 @end defivar
 
+@defivar Breakpoint expression
+This attribute holds the breakpoint expression, as specified by
+the user.  It is a string.  This attribute is not writable.
+@end defivar
+
 @defivar Breakpoint condition
 This attribute holds the condition of the breakpoint, as specified by
 the user.  It is a string.  If there is no condition, this attribute's
diff --git a/gdb/python/lib/gdb/command/save_breakpoints.py b/gdb/python/lib/gdb/command/save_breakpoints.py
index 90e07db..c08f69f 100644
--- a/gdb/python/lib/gdb/command/save_breakpoints.py
+++ b/gdb/python/lib/gdb/command/save_breakpoints.py
@@ -36,6 +36,19 @@ The breakpoints can be restored using the 'source' command."""
                                                        gdb.COMMAND_SUPPORT,
                                                        gdb.COMPLETE_FILENAME)
 
+    def breakpoint_type (self, b):
+        if b.type == gdb.BP_BREAKPOINT:
+            s = "break " + b.location
+        elif b.type == gdb.BP_WATCHPOINT or b.type == gdb.BP_HARDWARE_WATCHPOINT:
+            s = "watch " + b.expression
+        elif b.type == gdb.BP_READ_WATCHPOINT:
+            s = "rwatch " + b.expression
+        elif b.type == gdb.BP_ACCESS_WATCHPOINT:
+            s = "awatch " + b.expression
+        else:
+            s = ""
+        return s
+
     def invoke (self, arg, from_tty):
         self.dont_repeat ()
         bps = gdb.breakpoints ()
@@ -43,7 +56,7 @@ The breakpoints can be restored using the 'source' command."""
             raise RuntimeError, 'No breakpoints to save'
         with open (arg.strip (), 'w') as f:
             for bp in bps:
-                print >> f, "break", bp.location,
+                print >> f, self.breakpoint_type (bp),
                 if bp.thread is not None:
                     print >> f, " thread", bp.thread,
                 if bp.condition is not None:
diff --git a/gdb/python/python-breakpoint.c b/gdb/python/python-breakpoint.c
index 5e87af6..99df498 100644
--- a/gdb/python/python-breakpoint.c
+++ b/gdb/python/python-breakpoint.c
@@ -93,6 +93,26 @@ struct breakpoint_object
 	}								\
     } while (0)
 
+/* This is used to initialize various gdb.bp_* constants.  */
+struct pybp_code
+{
+  /* The name.  */
+  const char *name;
+  /* The code.  */
+  enum type_code code;
+};
+
+/* Entries related to the type of user set breakpoints.  */
+static struct pybp_code pybp_codes[] =
+{
+  {"BP_NONE", bp_none},
+  {"BP_BREAKPOINT", bp_breakpoint},
+  {"BP_WATCHPOINT", bp_watchpoint},
+  {"BP_HARDWARE_WATCHPOINT", bp_hardware_watchpoint},
+  {"BP_READ_WATCHPOINT", bp_read_watchpoint},
+  {"BP_ACCESS_WATCHPOINT", bp_access_watchpoint},
+};
+
 /* Python function which checks the validity of a breakpoint object.  */
 static PyObject *
 bppy_is_valid (PyObject *self, PyObject *args)
@@ -291,6 +311,19 @@ bppy_get_location (PyObject *self, void *closure)
   return PyString_Decode (str, strlen (str), host_charset (), NULL);
 }
 
+/* Python function to get the breakpoint expression.  */
+static PyObject *
+bppy_get_expression (PyObject *self, void *closure)
+{
+  char *str;
+
+  BPPY_REQUIRE_VALID ((breakpoint_object *) self);
+  str = ((breakpoint_object *) self)->bp->exp_string;
+  if (! str)
+    str = "";
+  return PyString_Decode (str, strlen (str), host_charset (), NULL);
+}
+
 /* Python function to get the condition expression of a breakpoint.  */
 static PyObject *
 bppy_get_condition (PyObject *self, void *closure)
@@ -373,6 +406,16 @@ bppy_get_commands (PyObject *self, void *closure)
   return result;
 }
 
+/* Python function to get the breakpoint type.  */
+static PyObject *
+bppy_get_type (PyObject *self, void *closure)
+{
+
+  breakpoint_object *self_bp = (breakpoint_object *) self;
+  BPPY_REQUIRE_VALID (self_bp);
+  return PyInt_FromLong (self_bp->bp->type);
+}
+
 /* Python function to get the breakpoint's number.  */
 static PyObject *
 bppy_get_number (PyObject *self, void *closure)
@@ -578,6 +621,8 @@ gdbpy_breakpoint_deleted (int num)
 void
 gdbpy_initialize_breakpoints (void)
 {
+  int i;
+
   breakpoint_object_type.tp_new = bppy_new;
   if (PyType_Ready (&breakpoint_object_type) < 0)
     return;
@@ -588,6 +633,15 @@ gdbpy_initialize_breakpoints (void)
 
   observer_attach_breakpoint_created (gdbpy_breakpoint_created);
   observer_attach_breakpoint_deleted (gdbpy_breakpoint_deleted);
+
+  for (i = 0; pybp_codes[i].name; ++i)
+    {
+      if (PyModule_AddIntConstant (gdb_module,
+				   /* Cast needed for Python 2.4.  */
+				   (char *) pybp_codes[i].name,
+				   pybp_codes[i].code) < 0)
+	return;
+    }
 }
 
 \f
@@ -613,11 +667,15 @@ Can be set to zero to clear the count. No other value is valid\n\
 when setting this property.", NULL },
   { "location", bppy_get_location, NULL,
     "Location of the breakpoint, as specified by the user.", NULL},
+  { "expression", bppy_get_expression, NULL,
+    "Expression of the breakpoint, as specified by the user.", NULL},
   { "condition", bppy_get_condition, bppy_set_condition,
     "Condition of the breakpoint, as specified by the user,\
 or None if no condition set."},
   { "commands", bppy_get_commands, NULL,
     "Commands of the breakpoint, as specified by the user."},
+  { "type", bppy_get_type, NULL,
+    "Type of breakpoint."},
   { NULL }  /* Sentinel.  */
 };
 

Re: [patch] Add watchpoint support to save_breakpoints.py (and add supporting methods to python-breakpoint.c)

[Phil Muldoon]

On 06/11/2009 02:53 PM, Phil Muldoon wrote:
> Something that has been on my mind for awhile is that 
> save_breakpoints.py does not handle watchpoints very well.  I like 
> watchpoints, and use them all the time! So I propose this patch:
>
> This patch adds watchpoint support to the save_breakpoints.py command. 

Ping on this

Phil

Re: [patch] Add watchpoint support to save_breakpoints.py (and add supporting methods to python-breakpoint.c)

[Phil Muldoon]

On 06/29/2009 04:45 PM, Phil Muldoon wrote:
> On 06/11/2009 02:53 PM, Phil Muldoon wrote:
>> Something that has been on my mind for awhile is that 
>> save_breakpoints.py does not handle watchpoints very well.  I like 
>> watchpoints, and use them all the time! So I propose this patch:
>>
>> This patch adds watchpoint support to the save_breakpoints.py command. 
>
> Ping on this
>
> Phil
It's been a bit since the last ping, so another! ;)

Regards

Phil

Re: [patch] Add watchpoint support to save_breakpoints.py (and add supporting methods to python-breakpoint.c)

[Tom Tromey]

I'm sorry for the long delay on this.

Phil> +@defivar Breakpoint expression
Phil> +This attribute holds the breakpoint expression, as specified by
Phil> +the user.  It is a string.  This attribute is not writable.
Phil> +@end defivar

This should mention that the expression is only valid for watchpoints.

And, I think this patch should update the documentation to mention that
the location field is only valid for breakpoints.

Then, the corresponding getter functions should enforce this constraint.

It seems a little strange to have both address and expression.  I wonder
if we should just make up a new attribute name encompassing both.

It seems like there should be a BP_ constant for a temporary breakpoint.
What do you think?

Phil> +  {"BP_NONE", bp_none},
Phil> +  {"BP_BREAKPOINT", bp_breakpoint},
Phil> +  {"BP_WATCHPOINT", bp_watchpoint},
Phil> +  {"BP_HARDWARE_WATCHPOINT", bp_hardware_watchpoint},
Phil> +  {"BP_READ_WATCHPOINT", bp_read_watchpoint},
Phil> +  {"BP_ACCESS_WATCHPOINT", bp_access_watchpoint},

Add a space after each "{" and before each "}".

Phil> +  BPPY_REQUIRE_VALID ((breakpoint_object *) self);
Phil> +  str = ((breakpoint_object *) self)->bp->exp_string;

The repeated casts here look ugly.


I agree that omitting catches and other thing is ok.  I wonder, however,
whether we should simply skip those in gdbpy_breakpoint_created.

Tom

Re: [patch] Add watchpoint support to save_breakpoints.py (and add supporting methods to python-breakpoint.c)

[Phil Muldoon]

On 07/31/2009 07:21 PM, Tom Tromey wrote:
> Phil>  +@defivar Breakpoint expression
> Phil>  +This attribute holds the breakpoint expression, as specified by
> Phil>  +the user.  It is a string.  This attribute is not writable.
> Phil>  +@end defivar
>
> This should mention that the expression is only valid for watchpoints.
>    

Ok.

> And, I think this patch should update the documentation to mention that
> the location field is only valid for breakpoints.
>
> Then, the corresponding getter functions should enforce this constraint.
>    

Ok.

> It seems a little strange to have both address and expression.  I wonder
> if we should just make up a new attribute name encompassing both.
>    


The original patch I wrote did this, but I elected to just follow 
convention and mirror GDB as closely as possible in this case. Not that 
I am advocating this position as preferable, but it was just a choice at 
the time. I can't think of a case where one would field would be 
explicitly required; can you?


> It seems like there should be a BP_ constant for a temporary breakpoint.
> What do you think?
>    


There are lots of BP_ constants, but I only added what I needed in the 
watchpoint context. I can add the BP_ constant for a temporary 
breakpoint or I can add all of the sane BP_ constants if needed? What do 
you think here?


> Phil>  +  {"BP_NONE", bp_none},
> Phil>  +  {"BP_BREAKPOINT", bp_breakpoint},
> Phil>  +  {"BP_WATCHPOINT", bp_watchpoint},
> Phil>  +  {"BP_HARDWARE_WATCHPOINT", bp_hardware_watchpoint},
> Phil>  +  {"BP_READ_WATCHPOINT", bp_read_watchpoint},
> Phil>  +  {"BP_ACCESS_WATCHPOINT", bp_access_watchpoint},
>
> Add a space after each "{" and before each "}".
>    

Ok.

> Phil>  +  BPPY_REQUIRE_VALID ((breakpoint_object *) self);
> Phil>  +  str = ((breakpoint_object *) self)->bp->exp_string;
>
> The repeated casts here look ugly.
>
>    


I'm guilty here of cut and paste coding from the other bppy_get * 
functions. I'll fix up this cast, and others.


> I agree that omitting catches and other thing is ok.  I wonder, however,
> whether we should simply skip those in gdbpy_breakpoint_created.
>    


In the longer term I really would like to support catchpoints. 
Especially with Sergio's catch syscall feature. I think for now, they 
should be skipped. They are not supported, and there is no need to alloc 
space for them if they are never touched. What if it should be very 
restrictive, there are lots of random internal/maintenance breakpoints 
we don't support in the save function.

Regards

Phil