[0/6] breakpoints_ops for all kinds of breakpoints

[0/6] breakpoints_ops for all kinds of breakpoints

[Pedro Alves]

I did yet a bit more work on:

 <http://sourceware.org/ml/gdb-patches/2011-06/msg00374.html>

This converts all breakpoint kinds to the breakpoint_ops
mechanism.

IMO, this is committable as is.  WDYT?

There's a lot left to do still.  E.g.: The print_one method
is still left behind; insert/removal could do with better
abstracting; a lot of tangling revealed by the need for 
special checks of tracepoint/watchpoint types at inner layers
of breakpoint creation; there are many places that still
switch on the breakpoint's type, as there's no breakpoint_ops
equivalent for what they're doing.

No regressions for me, native or gdbserver x86_64-linux.

-- 
Pedro Alves

Re: [0/6] breakpoints_ops for all kinds of breakpoints

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:

Pedro> I did yet a bit more work on:
Pedro>  <http://sourceware.org/ml/gdb-patches/2011-06/msg00374.html>
Pedro> This converts all breakpoint kinds to the breakpoint_ops
Pedro> mechanism.

Pedro> IMO, this is committable as is.  WDYT?

I read through them all.  I think this is a great series.  Please check
it in.

I'm not particularly fond of patch #4, but I understand why you did it,
and I agree that statically initialized vtables are a pain -- I am often
grepping for all instances and hoping I haven't missed one.  (I wish we
could require designated initializers for these so I could more easily
search for field names...)

I think we could make most of the vtable pointers (but not objects due
to the initialization approach) const.  I think that would help clarity
a bit.  I'm happy to do this after these patches go in.

Tom

Re: [0/6] breakpoints_ops for all kinds of breakpoints

[Phil Muldoon]

Pedro Alves <pedro@codesourcery.com> writes:

> I did yet a bit more work on:
>
>  <http://sourceware.org/ml/gdb-patches/2011-06/msg00374.html>
>
> This converts all breakpoint kinds to the breakpoint_ops
> mechanism.
>
> IMO, this is committable as is.  WDYT?

I think it is great.  It clears up several (dozen) ambiguities with
breakpoint operations.  I don't think this will affect the Python
breakpoint_ops work (other than some refactoring, but we'll come to that
bridge when your work is checked in).  

Thanks for this patch.

Cheers,

Phil

Re: [0/6] breakpoints_ops for all kinds of breakpoints

[Pedro Alves]

On Friday 22 July 2011 18:22:37, Tom Tromey wrote:
> >>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:
> 
> Pedro> I did yet a bit more work on:
> Pedro>  <http://sourceware.org/ml/gdb-patches/2011-06/msg00374.html>
> Pedro> This converts all breakpoint kinds to the breakpoint_ops
> Pedro> mechanism.
> 
> Pedro> IMO, this is committable as is.  WDYT?
> 
> I read through them all.  I think this is a great series.  Please check
> it in.

Great, thanks.  I've applied the series now.

> I think we could make most of the vtable pointers (but not objects due
> to the initialization approach) const.  I think that would help clarity
> a bit.  I'm happy to do this after these patches go in.

Yeah, please go ahead.

-- 
Pedro Alves

Re: [0/6] breakpoints_ops for all kinds of breakpoints

[Pedro Alves]

On Friday 22 July 2011 18:32:59, Phil Muldoon wrote:
> Pedro Alves <pedro@codesourcery.com> writes:
> 
> > I did yet a bit more work on:
> >
> >  <http://sourceware.org/ml/gdb-patches/2011-06/msg00374.html>
> >
> > This converts all breakpoint kinds to the breakpoint_ops
> > mechanism.
> >
> > IMO, this is committable as is.  WDYT?
> 
> I think it is great.  It clears up several (dozen) ambiguities with
> breakpoint operations.  I don't think this will affect the Python
> breakpoint_ops work (other than some refactoring, but we'll come to that
> bridge when your work is checked in).

Yeah, I don't think your current patch would need more than a
some trivial updates, but then again, I did leave the print_one
method behind in this conversion, which was the method that
I raised having the issue that doesn't seem to fit for gdb's own
breakpoints currently, so we should clean that up before
considering exposing it as public API.

-- 
Pedro Alves

Re: [0/6] breakpoints_ops for all kinds of breakpoints

[Phil Muldoon]

Pedro Alves <pedro@codesourcery.com> writes:

> On Friday 22 July 2011 18:32:59, Phil Muldoon wrote:
>> I think it is great.  It clears up several (dozen) ambiguities with
>> breakpoint operations.  I don't think this will affect the Python
>> breakpoint_ops work (other than some refactoring, but we'll come to that
>> bridge when your work is checked in).
>
> Yeah, I don't think your current patch would need more than a
> some trivial updates, but then again, I did leave the print_one
> method behind in this conversion, which was the method that
> I raised having the issue that doesn't seem to fit for gdb's own
> breakpoints currently, so we should clean that up before
> considering exposing it as public API.

I have been thinking on your comments quite a bit in the last few
weeks, and about external facing APIs in general.  I don't think it is
sensible to make 1:1 mappings of these breakpoint_ops APIs
to Python.  I think "print_one" "print_it" mean nothing to a Python
scripting user.  I've come to the conclusion that we should abstract
these functions to concepts a Python scripter can better understand.  So
"print_one" will not be directly exposed as before.  The Python API,
will end up calling print_one, sure, but there will (I hope) be
sufficient levels of abstraction that any future refactoring will be
coped with internally.  We already massage the data to-and-from GDB
extensively anyway, so it makes better sense to the user.  

So my plan is to submit a patch after yours is checked in, with the new
APIs.  We can look again, then, if it makes sense at that point, and if
any future refactorings of print_one will really matter to the user from
an API perspective.

Cheers,

Phil

Re: [0/6] breakpoints_ops for all kinds of breakpoints

[Tom Tromey]

Tom> I think we could make most of the vtable pointers (but not objects due
Tom> to the initialization approach) const.  I think that would help clarity
Tom> a bit.  I'm happy to do this after these patches go in.

Pedro> Yeah, please go ahead.

Patch appended.  Built and regtested by the buildbot.
I'm committing it.

Tom

2011-07-25  Tom Tromey  <tromey@redhat.com>

	* ada-lang.c (ada_exception_breakpoint_ops): Make return type
	const.
	(ada_exception_sal): Make 'ops' const.
	(ada_decode_exception_location): Likewise.
	(ada_decode_assert_location): Likewise.
	(catch_assert_command): Update.
	(catch_ada_exception_command): Update.
	(create_ada_exception_catchpoint): Make 'ops' const.
	* breakpoint.c (set_raw_breakpoint_without_location)
	(set_raw_breakpoint, momentary_breakpoint_from_master): Make 'ops'
	const.
	(create_internal_breakpoint): Update.
	(init_raw_breakpoint_without_location): Make 'ops' const.
	(init_raw_breakpoint, init_catchpoint)
	(create_fork_vfork_event_catchpoint)
	(create_syscall_event_catchpoint, init_breakpoint_sal)
	(create_breakpoint_sal, create_breakpoints_sal)
	(create_breakpoint, init_ada_exception_breakpoint): Likewise.
	* breakpoint.h (struct breakpoint) <ops>: Now const.
	(init_ada_exception_breakpoint, create_breakpoint): Make 'ops'
	const.

diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index d017800..02115de 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -10863,7 +10863,7 @@ ada_exception_name_addr (enum exception_catchpoint_kind ex,
 
 static struct symtab_and_line ada_exception_sal (enum exception_catchpoint_kind,
 						 char *, char **,
-						 struct breakpoint_ops **);
+						 const struct breakpoint_ops **);
 static char *ada_exception_catchpoint_cond_string (const char *excep_string);
 
 /* Ada catchpoints.
@@ -11538,7 +11538,7 @@ ada_exception_sym_name (enum exception_catchpoint_kind ex)
 /* Return the breakpoint ops "virtual table" used for catchpoints
    of the EX kind.  */
 
-static struct breakpoint_ops *
+static const struct breakpoint_ops *
 ada_exception_breakpoint_ops (enum exception_catchpoint_kind ex)
 {
   switch (ex)
@@ -11613,7 +11613,7 @@ ada_exception_catchpoint_cond_string (const char *excep_string)
 
 static struct symtab_and_line
 ada_exception_sal (enum exception_catchpoint_kind ex, char *excep_string,
-		   char **addr_string, struct breakpoint_ops **ops)
+		   char **addr_string, const struct breakpoint_ops **ops)
 {
   const char *sym_name;
   struct symbol *sym;
@@ -11676,7 +11676,7 @@ ada_exception_sal (enum exception_catchpoint_kind ex, char *excep_string,
 static struct symtab_and_line
 ada_decode_exception_location (char *args, char **addr_string,
                                char **excep_string,
-                               struct breakpoint_ops **ops)
+                               const struct breakpoint_ops **ops)
 {
   enum exception_catchpoint_kind ex;
 
@@ -11691,7 +11691,7 @@ create_ada_exception_catchpoint (struct gdbarch *gdbarch,
 				 struct symtab_and_line sal,
 				 char *addr_string,
 				 char *excep_string,
-				 struct breakpoint_ops *ops,
+				 const struct breakpoint_ops *ops,
 				 int tempflag,
 				 int from_tty)
 {
@@ -11716,7 +11716,7 @@ catch_ada_exception_command (char *arg, int from_tty,
   struct symtab_and_line sal;
   char *addr_string = NULL;
   char *excep_string = NULL;
-  struct breakpoint_ops *ops = NULL;
+  const struct breakpoint_ops *ops = NULL;
 
   tempflag = get_cmd_context (command) == CATCH_TEMPORARY;
 
@@ -11729,7 +11729,7 @@ catch_ada_exception_command (char *arg, int from_tty,
 
 static struct symtab_and_line
 ada_decode_assert_location (char *args, char **addr_string,
-                            struct breakpoint_ops **ops)
+                            const struct breakpoint_ops **ops)
 {
   /* Check that no argument where provided at the end of the command.  */
 
@@ -11754,7 +11754,7 @@ catch_assert_command (char *arg, int from_tty,
   int tempflag;
   struct symtab_and_line sal;
   char *addr_string = NULL;
-  struct breakpoint_ops *ops = NULL;
+  const struct breakpoint_ops *ops = NULL;
 
   tempflag = get_cmd_context (command) == CATCH_TEMPORARY;
 
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 4f09d3a..5afda31 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -108,17 +108,18 @@ static void mention (struct breakpoint *);
 
 static struct breakpoint *set_raw_breakpoint_without_location (struct gdbarch *,
 							       enum bptype,
-							       struct breakpoint_ops *);
+							       const struct breakpoint_ops *);
 /* This function is used in gdbtk sources and thus can not be made
    static.  */
 struct breakpoint *set_raw_breakpoint (struct gdbarch *gdbarch,
 				       struct symtab_and_line,
-				       enum bptype, struct breakpoint_ops *);
+				       enum bptype,
+				       const struct breakpoint_ops *);
 
 static struct breakpoint *
   momentary_breakpoint_from_master (struct breakpoint *orig,
 				    enum bptype type,
-				    struct breakpoint_ops *ops);
+				    const struct breakpoint_ops *ops);
 
 static void breakpoint_adjustment_warning (CORE_ADDR, CORE_ADDR, int, int);
 
@@ -2110,7 +2111,7 @@ set_breakpoint_number (int internal, struct breakpoint *b)
 static struct breakpoint *
 create_internal_breakpoint (struct gdbarch *gdbarch,
 			    CORE_ADDR address, enum bptype type,
-			    struct breakpoint_ops *ops)
+			    const struct breakpoint_ops *ops)
 {
   struct symtab_and_line sal;
   struct breakpoint *b;
@@ -5605,7 +5606,7 @@ static void
 init_raw_breakpoint_without_location (struct breakpoint *b,
 				      struct gdbarch *gdbarch,
 				      enum bptype bptype,
-				      struct breakpoint_ops *ops)
+				      const struct breakpoint_ops *ops)
 {
   memset (b, 0, sizeof (*b));
 
@@ -5634,7 +5635,7 @@ init_raw_breakpoint_without_location (struct breakpoint *b,
 static struct breakpoint *
 set_raw_breakpoint_without_location (struct gdbarch *gdbarch,
 				     enum bptype bptype,
-				     struct breakpoint_ops *ops)
+				     const struct breakpoint_ops *ops)
 {
   struct breakpoint *b = XNEW (struct breakpoint);
 
@@ -5711,7 +5712,7 @@ get_sal_arch (struct symtab_and_line sal)
 static void
 init_raw_breakpoint (struct breakpoint *b, struct gdbarch *gdbarch,
 		     struct symtab_and_line sal, enum bptype bptype,
-		     struct breakpoint_ops *ops)
+		     const struct breakpoint_ops *ops)
 {
   CORE_ADDR adjusted_address;
   struct gdbarch *loc_gdbarch;
@@ -5775,7 +5776,7 @@ init_raw_breakpoint (struct breakpoint *b, struct gdbarch *gdbarch,
 struct breakpoint *
 set_raw_breakpoint (struct gdbarch *gdbarch,
 		    struct symtab_and_line sal, enum bptype bptype,
-		    struct breakpoint_ops *ops)
+		    const struct breakpoint_ops *ops)
 {
   struct breakpoint *b = XNEW (struct breakpoint);
 
@@ -6616,7 +6617,7 @@ static void
 init_catchpoint (struct breakpoint *b,
 		 struct gdbarch *gdbarch, int tempflag,
 		 char *cond_string,
-		 struct breakpoint_ops *ops)
+		 const struct breakpoint_ops *ops)
 {
   struct symtab_and_line sal;
 
@@ -6643,7 +6644,7 @@ install_breakpoint (int internal, struct breakpoint *b)
 static void
 create_fork_vfork_event_catchpoint (struct gdbarch *gdbarch,
 				    int tempflag, char *cond_string,
-                                    struct breakpoint_ops *ops)
+                                    const struct breakpoint_ops *ops)
 {
   struct fork_catchpoint *c = XNEW (struct fork_catchpoint);
 
@@ -6762,7 +6763,7 @@ static struct breakpoint_ops catch_exec_breakpoint_ops;
 
 static void
 create_syscall_event_catchpoint (int tempflag, VEC(int) *filter,
-                                 struct breakpoint_ops *ops)
+                                 const struct breakpoint_ops *ops)
 {
   struct syscall_catchpoint *c;
   struct gdbarch *gdbarch = get_current_arch ();
@@ -6942,7 +6943,7 @@ set_momentary_breakpoint (struct gdbarch *gdbarch, struct symtab_and_line sal,
 static struct breakpoint *
 momentary_breakpoint_from_master (struct breakpoint *orig,
 				  enum bptype type,
-				  struct breakpoint_ops *ops)
+				  const struct breakpoint_ops *ops)
 {
   struct breakpoint *copy;
 
@@ -7093,7 +7094,7 @@ init_breakpoint_sal (struct breakpoint *b, struct gdbarch *gdbarch,
 		     char *cond_string,
 		     enum bptype type, enum bpdisp disposition,
 		     int thread, int task, int ignore_count,
-		     struct breakpoint_ops *ops, int from_tty,
+		     const struct breakpoint_ops *ops, int from_tty,
 		     int enabled, int internal, int display_canonical)
 {
   int i;
@@ -7218,7 +7219,7 @@ create_breakpoint_sal (struct gdbarch *gdbarch,
 		       char *cond_string,
 		       enum bptype type, enum bpdisp disposition,
 		       int thread, int task, int ignore_count,
-		       struct breakpoint_ops *ops, int from_tty,
+		       const struct breakpoint_ops *ops, int from_tty,
 		       int enabled, int internal, int display_canonical)
 {
   struct breakpoint *b;
@@ -7402,7 +7403,7 @@ create_breakpoints_sal (struct gdbarch *gdbarch,
 			char *cond_string,
 			enum bptype type, enum bpdisp disposition,
 			int thread, int task, int ignore_count,
-			struct breakpoint_ops *ops, int from_tty,
+			const struct breakpoint_ops *ops, int from_tty,
 			int enabled, int internal)
 {
   int i;
@@ -7682,7 +7683,7 @@ create_breakpoint (struct gdbarch *gdbarch,
 		   int tempflag, enum bptype type_wanted,
 		   int ignore_count,
 		   enum auto_boolean pending_break_support,
-		   struct breakpoint_ops *ops,
+		   const struct breakpoint_ops *ops,
 		   int from_tty, int enabled, int internal)
 {
   volatile struct gdb_exception e;
@@ -9864,7 +9865,7 @@ init_ada_exception_breakpoint (struct breakpoint *b,
 			       struct gdbarch *gdbarch,
 			       struct symtab_and_line sal,
 			       char *addr_string,
-			       struct breakpoint_ops *ops,
+			       const struct breakpoint_ops *ops,
 			       int tempflag,
 			       int from_tty)
 {
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index 58e33d4..9b79e3e 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -533,7 +533,7 @@ extern int target_exact_watchpoints;
 struct breakpoint
   {
     /* Methods associated with this breakpoint.  */
-    struct breakpoint_ops *ops;
+    const struct breakpoint_ops *ops;
 
     struct breakpoint *next;
     /* Type of breakpoint.  */
@@ -1093,7 +1093,7 @@ extern void
 				 struct gdbarch *gdbarch,
 				 struct symtab_and_line sal,
 				 char *addr_string,
-				 struct breakpoint_ops *ops,
+				 const struct breakpoint_ops *ops,
 				 int tempflag,
 				 int from_tty);
 
@@ -1110,7 +1110,7 @@ extern int create_breakpoint (struct gdbarch *gdbarch, char *arg,
 			      int tempflag, enum bptype wanted_type,
 			      int ignore_count,
 			      enum auto_boolean pending_break_support,
-			      struct breakpoint_ops *ops,
+			      const struct breakpoint_ops *ops,
 			      int from_tty,
 			      int enabled,
 			      int internal);