[RFC] Make static tracepoint with markers more OO

[RFC] Make static tracepoint with markers more OO

[Sergio Durigan Junior]

Hello there,

I have been working on this patch, and I would like some comments from
you guys.  It basically implements new methods inside breakpoint_ops in
order to make static tracepoint with markers (`strace -m') more OO.

Currently, we use a check (defined in `is_marker_spec') in order to
identify those tracepoints, and act accordingly.  However, it makes the
code (especially the functions `create_breakpoint' and
`addr_string_to_sals') very conditional.

This would be OK if it were to be the only case, but as it turns out our
SystemTap integration patch will have to add more complexity to this
code, which would make things uglier and uglier.  So, as a preparation
for the second round of submissions of the SystemTap patch, I am
submitting this patch first.

I am not sure the names I picked for the new methods are good.  I had
spent a good time thinking about the names, but unfortunately I couldn't
come up with something better than this.  I am specially concerned about
the `create_breakpoints_sal*' set of methods/functions, because there
are too many of them IMO.  I would appreciate some reviews on the
comments of the methods, as well.  I believe the separation is pretty
straightforward, so I think this is just moving bits here and there
without actually implementing anything new.

Comments are welcome, as usual.

Regards,

Sergio.

2012-01-13  Sergio Durigan Junior  <sergiodj@redhat.com>

	* breakpoint.c (create_sals_from_address_default): New function.
	(create_breakpoints_sal_default): Likewise.
	(decode_linespec_default): Likewise.
	(is_marker_spec): Removed.
	(strace_marker_p): New function.
	(init_breakpoint_sal): Using `strace_marker_p' instead of
	`is_marker_spec'.
	(create_breakpoint): Call method `create_sals_from_address' from
	breakpoint_ops, replacing code that created SALs conditionally
	on the type of the breakpoint.  Call method `create_breakpoints_sal',
	replacing code that created breakpoints conditionally on the type
	wanted.
	(base_breakpoint_create_sals_from_address): New function.
	(base_breakpoint_create_breakpoints_sal): Likewise.
	(base_breakpoint_decode_linespec): Likewise.
	(base_breakpoint_ops): Add methods
	`base_breakpoint_create_sals_from_address',
	`base_breakpoint_create_breakpoints_sal' and
	`base_breakpoint_decode_linespec'.
	(bkpt_create_sals_from_address): New function.
	(bkpt_create_breakpoints_sal): Likewise.
	(bkpt_decode_linespec): Likewise.
	(tracepoint_create_sals_from_address): Likewise.
	(tracepoint_create_breakpoints_sal): Likewise.
	(tracepoint_decode_linespec): Likewise.
	(strace_marker_create_sals_from_address): Likewise.
	(strace_marker_create_breakpoints_sal): Likewise.
	(strace_marker_decode_linespec): Likewise.
	(strace_marker_breakpoint_ops): New variable.
	(addr_string_to_sals): Remove `marker_spec'.  Call method
	`decode_linespec' from breakpoint_ops, replacing code that decoded
	an address string into a SAL.  Use `strace_marker_p' instead of
	`marker_spec'.
	(strace_command): Decide whether we are dealing with a static
	tracepoint with marker or not.  Use the appropriate breakpoint_ops.
	(initialize_breakpoint_ops): Initialize new fields of breakpoint_ops.
	* breakpoint.h (linespec_result, linespec_sals): New forward
	declarations.
	(breakpoint_ops) <create_sals_from_address>,
	<create_breakpoints_sal>, <decode_linespec>: New methods.

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 88fc176..263c190 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -96,6 +96,23 @@ static int breakpoint_re_set_one (void *);
 
 static void breakpoint_re_set_default (struct breakpoint *);
 
+static int create_sals_from_address_default (char **,
+					     struct linespec_result *,
+					     enum bptype, int *, char *,
+					     char **);
+
+static void create_breakpoints_sal_default (struct gdbarch *,
+					    struct linespec_result *,
+					    struct linespec_sals *,
+					    char *, enum bptype,
+					    enum bpdisp, int, int,
+					    int,
+					    const struct breakpoint_ops *,
+					    int, int, int);
+
+static void decode_linespec_default (struct breakpoint *, char **,
+				     struct symtabs_and_lines *);
+
 static void clear_command (char *, int);
 
 static void catch_command (char *, int);
@@ -237,10 +254,9 @@ static void trace_pass_command (char *, int);
 
 static int is_masked_watchpoint (const struct breakpoint *b);
 
-/* Assuming we're creating a static tracepoint, does S look like a
-   static tracepoint marker spec ("-m MARKER_ID")?  */
-#define is_marker_spec(s)						\
-  (s != NULL && strncmp (s, "-m", 2) == 0 && ((s)[2] == ' ' || (s)[2] == '\t'))
+/* Return 1 if B refers to a static tracepoint marker, zero otherwise.  */
+
+static int strace_marker_p (struct breakpoint *b);
 
 /* The abstract base class all breakpoint_ops structures inherit
    from.  */
@@ -7298,7 +7314,7 @@ init_breakpoint_sal (struct breakpoint *b, struct gdbarch *gdbarch,
 	      struct tracepoint *t = (struct tracepoint *) b;
 	      struct static_tracepoint_marker marker;
 
-	      if (is_marker_spec (addr_string))
+	      if (strace_marker_p (b))
 		{
 		  /* We already know the marker exists, otherwise, we
 		     wouldn't see a sal for it.  */
@@ -7720,78 +7736,9 @@ create_breakpoint (struct gdbarch *gdbarch,
 
   init_linespec_result (&canonical);
 
-  if (type_wanted == bp_static_tracepoint && is_marker_spec (arg))
-    {
-      int i;
-      struct linespec_sals lsal;
-
-      lsal.sals = decode_static_tracepoint_spec (&arg);
-
-      copy_arg = savestring (addr_start, arg - addr_start);
-
-      canonical.addr_string = xstrdup (copy_arg);
-      lsal.canonical = xstrdup (copy_arg);
-      VEC_safe_push (linespec_sals, canonical.sals, &lsal);
-
-      goto done;
-    }
-
-  TRY_CATCH (e, RETURN_MASK_ALL)
-    {
-      parse_breakpoint_sals (&arg, &canonical);
-    }
-
-  /* If caller is interested in rc value from parse, set value.  */
-  switch (e.reason)
-    {
-    case RETURN_QUIT:
-      throw_exception (e);
-    case RETURN_ERROR:
-      switch (e.error)
-	{
-	case NOT_FOUND_ERROR:
-
-	  /* If pending breakpoint support is turned off, throw
-	     error.  */
-
-	  if (pending_break_support == AUTO_BOOLEAN_FALSE)
-	    throw_exception (e);
-
-	  exception_print (gdb_stderr, e);
-
-          /* If pending breakpoint support is auto query and the user
-	     selects no, then simply return the error code.  */
-	  if (pending_break_support == AUTO_BOOLEAN_AUTO
-	      && !nquery (_("Make %s pending on future shared library load? "),
-			  bptype_string (type_wanted)))
-	    return 0;
-
-	  /* At this point, either the user was queried about setting
-	     a pending breakpoint and selected yes, or pending
-	     breakpoint behavior is on and thus a pending breakpoint
-	     is defaulted on behalf of the user.  */
-	  {
-	    struct linespec_sals lsal;
-
-	    copy_arg = xstrdup (addr_start);
-	    lsal.canonical = xstrdup (copy_arg);
-	    lsal.sals.nelts = 1;
-	    lsal.sals.sals = XNEW (struct symtab_and_line);
-	    init_sal (&lsal.sals.sals[0]);
-	    pending = 1;
-	    VEC_safe_push (linespec_sals, canonical.sals, &lsal);
-	  }
-	  break;
-	default:
-	  throw_exception (e);
-	}
-      break;
-    default:
-      if (VEC_empty (linespec_sals, canonical.sals))
-	return 0;
-    }
-
-  done:
+  if (!ops->create_sals_from_address (&arg, &canonical, type_wanted,
+				      &pending, addr_start, &copy_arg))
+    return 0;
 
   /* Create a chain of things that always need to be cleaned up.  */
   old_chain = make_cleanup_destroy_linespec_result (&canonical);
@@ -7855,57 +7802,11 @@ create_breakpoint (struct gdbarch *gdbarch,
             }
         }
 
-      /* If the user is creating a static tracepoint by marker id
-	 (strace -m MARKER_ID), then store the sals index, so that
-	 breakpoint_re_set can try to match up which of the newly
-	 found markers corresponds to this one, and, don't try to
-	 expand multiple locations for each sal, given than SALS
-	 already should contain all sals for MARKER_ID.  */
-      if (type_wanted == bp_static_tracepoint
-	  && is_marker_spec (copy_arg))
-	{
-	  int i;
-
-	  for (i = 0; i < lsal->sals.nelts; ++i)
-	    {
-	      struct symtabs_and_lines expanded;
-	      struct tracepoint *tp;
-	      struct cleanup *old_chain;
-	      char *addr_string;
-
-	      expanded.nelts = 1;
-	      expanded.sals = &lsal->sals.sals[i];
-
-	      addr_string = xstrdup (canonical.addr_string);
-	      old_chain = make_cleanup (xfree, addr_string);
-
-	      tp = XCNEW (struct tracepoint);
-	      init_breakpoint_sal (&tp->base, gdbarch, expanded,
-				   addr_string, NULL,
+      ops->create_breakpoints_sal (gdbarch, &canonical, lsal,
 				   cond_string, type_wanted,
 				   tempflag ? disp_del : disp_donttouch,
 				   thread, task, ignore_count, ops,
-				   from_tty, enabled, internal,
-				   canonical.special_display);
-	      /* Given that its possible to have multiple markers with
-		 the same string id, if the user is creating a static
-		 tracepoint by marker id ("strace -m MARKER_ID"), then
-		 store the sals index, so that breakpoint_re_set can
-		 try to match up which of the newly found markers
-		 corresponds to this one  */
-	      tp->static_trace_marker_id_idx = i;
-
-	      install_breakpoint (internal, &tp->base, 0);
-
-	      discard_cleanups (old_chain);
-	    }
-	}
-      else
-	create_breakpoints_sal (gdbarch, &canonical, cond_string,
-				type_wanted,
-				tempflag ? disp_del : disp_donttouch,
-				thread, task, ignore_count, ops, from_tty,
-				enabled, internal);
+				   from_tty, enabled, internal);
     }
   else
     {
@@ -10910,6 +10811,39 @@ base_breakpoint_print_recreate (struct breakpoint *b, struct ui_file *fp)
   internal_error_pure_virtual_called ();
 }
 
+static int
+base_breakpoint_create_sals_from_address (char **arg,
+					  struct linespec_result *canonical,
+					  enum bptype type_wanted,
+					  int *pending, char *addr_start,
+					  char **copy_arg)
+{
+  internal_error_pure_virtual_called ();
+}
+
+static void
+base_breakpoint_create_breakpoints_sal (struct gdbarch *gdbarch,
+					struct linespec_result *c,
+					struct linespec_sals *lsal,
+					char *cond_string,
+					enum bptype type_wanted,
+					enum bpdisp disposition,
+					int thread,
+					int task, int ignore_count,
+					const struct breakpoint_ops *o,
+					int from_tty, int enabled,
+					int internal)
+{
+  internal_error_pure_virtual_called ();
+}
+
+static void
+base_breakpoint_decode_linespec (struct breakpoint *b, char **s,
+				 struct symtabs_and_lines *sals)
+{
+  internal_error_pure_virtual_called ();
+}
+
 static struct breakpoint_ops base_breakpoint_ops =
 {
   base_breakpoint_dtor,
@@ -10925,7 +10859,10 @@ static struct breakpoint_ops base_breakpoint_ops =
   NULL,
   base_breakpoint_print_one_detail,
   base_breakpoint_print_mention,
-  base_breakpoint_print_recreate
+  base_breakpoint_print_recreate,
+  base_breakpoint_create_sals_from_address,
+  base_breakpoint_create_breakpoints_sal,
+  base_breakpoint_decode_linespec,
 };
 
 /* Default breakpoint_ops methods.  */
@@ -11071,6 +11008,43 @@ bkpt_print_recreate (struct breakpoint *tp, struct ui_file *fp)
   print_recreate_thread (tp, fp);
 }
 
+static int
+bkpt_create_sals_from_address (char **arg,
+			       struct linespec_result *canonical,
+			       enum bptype type_wanted, int *pending,
+			       char *addr_start, char **copy_arg)
+{
+  return create_sals_from_address_default (arg, canonical, type_wanted,
+					   pending, addr_start, copy_arg);
+}
+
+static void
+bkpt_create_breakpoints_sal (struct gdbarch *gdbarch,
+			     struct linespec_result *canonical,
+			     struct linespec_sals *lsal,
+			     char *cond_string,
+			     enum bptype type_wanted,
+			     enum bpdisp disposition,
+			     int thread,
+			     int task, int ignore_count,
+			     const struct breakpoint_ops *ops,
+			     int from_tty, int enabled,
+			     int internal)
+{
+  create_breakpoints_sal_default (gdbarch, canonical, lsal,
+				  cond_string, type_wanted,
+				  disposition, thread, task,
+				  ignore_count, ops, from_tty,
+				  enabled, internal);
+}
+
+static void
+bkpt_decode_linespec (struct breakpoint *b, char **s,
+		      struct symtabs_and_lines *sals)
+{
+  decode_linespec_default (b, s, sals);
+}
+
 /* Virtual table for internal breakpoints.  */
 
 static void
@@ -11297,8 +11271,147 @@ tracepoint_print_recreate (struct breakpoint *self, struct ui_file *fp)
     fprintf_unfiltered (fp, "  passcount %d\n", tp->pass_count);
 }
 
+static int
+tracepoint_create_sals_from_address (char **arg,
+				     struct linespec_result *canonical,
+				     enum bptype type_wanted, int *pending,
+				     char *addr_start, char **copy_arg)
+{
+  return create_sals_from_address_default (arg, canonical, type_wanted,
+					   pending, addr_start, copy_arg);
+}
+
+static void
+tracepoint_create_breakpoints_sal (struct gdbarch *gdbarch,
+				   struct linespec_result *canonical,
+				   struct linespec_sals *lsal,
+				   char *cond_string,
+				   enum bptype type_wanted,
+				   enum bpdisp disposition,
+				   int thread,
+				   int task, int ignore_count,
+				   const struct breakpoint_ops *ops,
+				   int from_tty, int enabled,
+				   int internal)
+{
+  create_breakpoints_sal_default (gdbarch, canonical, lsal,
+				  cond_string, type_wanted,
+				  disposition, thread, task,
+				  ignore_count, ops, from_tty,
+				  enabled, internal);
+}
+
+static void
+tracepoint_decode_linespec (struct breakpoint *b, char **s,
+			    struct symtabs_and_lines *sals)
+{
+  decode_linespec_default (b, s, sals);
+}
+
 struct breakpoint_ops tracepoint_breakpoint_ops;
 
+/* The breakpoint_ops structure to be used on static tracepoints with
+   markers (`-m').  */
+
+static int
+strace_marker_create_sals_from_address (char **arg,
+					struct linespec_result *canonical,
+					enum bptype type_wanted, int *pending,
+					char *addr_start, char **copy_arg)
+{
+  struct linespec_sals lsal;
+
+  lsal.sals = decode_static_tracepoint_spec (arg);
+
+  *copy_arg = savestring (addr_start, *arg - addr_start);
+
+  canonical->addr_string = xstrdup (*copy_arg);
+  lsal.canonical = xstrdup (*copy_arg);
+  VEC_safe_push (linespec_sals, canonical->sals, &lsal);
+
+  return 1;
+}
+
+static void
+strace_marker_create_breakpoints_sal (struct gdbarch *gdbarch,
+				      struct linespec_result *canonical,
+				      struct linespec_sals *lsal,
+				      char *cond_string,
+				      enum bptype type_wanted,
+				      enum bpdisp disposition,
+				      int thread,
+				      int task, int ignore_count,
+				      const struct breakpoint_ops *ops,
+				      int from_tty, int enabled,
+				      int internal)
+{
+  int i;
+
+  /* If the user is creating a static tracepoint by marker id
+     (strace -m MARKER_ID), then store the sals index, so that
+     breakpoint_re_set can try to match up which of the newly
+     found markers corresponds to this one, and, don't try to
+     expand multiple locations for each sal, given than SALS
+     already should contain all sals for MARKER_ID.  */
+
+  for (i = 0; i < lsal->sals.nelts; ++i)
+    {
+      struct symtabs_and_lines expanded;
+      struct tracepoint *tp;
+      struct cleanup *old_chain;
+      char *addr_string;
+
+      expanded.nelts = 1;
+      expanded.sals = &lsal->sals.sals[i];
+
+      addr_string = xstrdup (canonical->addr_string);
+      old_chain = make_cleanup (xfree, addr_string);
+
+      tp = XCNEW (struct tracepoint);
+      init_breakpoint_sal (&tp->base, gdbarch, expanded,
+			   addr_string, NULL,
+			   cond_string, type_wanted, disposition,
+			   thread, task, ignore_count, ops,
+			   from_tty, enabled, internal,
+			   canonical->special_display);
+      /* Given that its possible to have multiple markers with
+	 the same string id, if the user is creating a static
+	 tracepoint by marker id ("strace -m MARKER_ID"), then
+	 store the sals index, so that breakpoint_re_set can
+	 try to match up which of the newly found markers
+	 corresponds to this one  */
+      tp->static_trace_marker_id_idx = i;
+
+      install_breakpoint (internal, &tp->base, 0);
+
+      discard_cleanups (old_chain);
+    }
+}
+
+static void
+strace_marker_decode_linespec (struct breakpoint *b, char **s,
+			       struct symtabs_and_lines *sals)
+{
+  struct tracepoint *tp = (struct tracepoint *) b;
+
+  *sals = decode_static_tracepoint_spec (s);
+  if (sals->nelts > tp->static_trace_marker_id_idx)
+    {
+      sals->sals[0] = sals->sals[tp->static_trace_marker_id_idx];
+      sals->nelts = 1;
+    }
+  else
+    error (_("marker %s not found"), tp->static_trace_marker_id);
+}
+
+static struct breakpoint_ops strace_marker_breakpoint_ops;
+
+static int
+strace_marker_p (struct breakpoint *b)
+{
+  return b->ops == &strace_marker_breakpoint_ops;
+}
+
 /* Delete a breakpoint and clean up all traces of it in the data
    structures.  */
 
@@ -11833,54 +11946,15 @@ static struct symtabs_and_lines
 addr_string_to_sals (struct breakpoint *b, char *addr_string, int *found)
 {
   char *s;
-  int marker_spec;
   struct symtabs_and_lines sals = {0};
   volatile struct gdb_exception e;
 
+  gdb_assert (b->ops != NULL);
   s = addr_string;
-  marker_spec = b->type == bp_static_tracepoint && is_marker_spec (s);
 
   TRY_CATCH (e, RETURN_MASK_ERROR)
     {
-      if (marker_spec)
-	{
-	  struct tracepoint *tp = (struct tracepoint *) b;
-
-	  sals = decode_static_tracepoint_spec (&s);
-	  if (sals.nelts > tp->static_trace_marker_id_idx)
-	    {
-	      sals.sals[0] = sals.sals[tp->static_trace_marker_id_idx];
-	      sals.nelts = 1;
-	    }
-	  else
-	    error (_("marker %s not found"), tp->static_trace_marker_id);
-	}
-      else
-	{
-	  struct linespec_result canonical;
-
-	  init_linespec_result (&canonical);
-	  decode_line_full (&s, DECODE_LINE_FUNFIRSTLINE,
-			    (struct symtab *) NULL, 0,
-			    &canonical, multiple_symbols_all,
-			    b->filter);
-
-	  /* We should get 0 or 1 resulting SALs.  */
-	  gdb_assert (VEC_length (linespec_sals, canonical.sals) < 2);
-
-	  if (VEC_length (linespec_sals, canonical.sals) > 0)
-	    {
-	      struct linespec_sals *lsal;
-
-	      lsal = VEC_index (linespec_sals, canonical.sals, 0);
-	      sals = lsal->sals;
-	      /* Arrange it so the destructor does not free the
-		 contents.  */
-	      lsal->sals.sals = NULL;
-	    }
-
-	  destroy_linespec_result (&canonical);
-	}
+      b->ops->decode_linespec (b, &s, &sals);
     }
   if (e.reason < 0)
     {
@@ -11933,7 +12007,7 @@ addr_string_to_sals (struct breakpoint *b, char *addr_string, int *found)
 	  b->condition_not_parsed = 0;
 	}
 
-      if (b->type == bp_static_tracepoint && !marker_spec)
+      if (strace_marker_p (b))
 	sals.sals[0] = update_static_tracepoint (b, sals.sals[0]);
 
       *found = 1;
@@ -11976,6 +12050,130 @@ breakpoint_re_set_default (struct breakpoint *b)
   update_breakpoint_locations (b, expanded, expanded_end);
 }
 
+/* Default method for creating SALs from an address string.  It basically
+   calls parse_breakpoint_sals.  Return 1 for success, zero for failure.  */
+
+static int
+create_sals_from_address_default (char **arg,
+				  struct linespec_result *canonical,
+				  enum bptype type_wanted, int *pending,
+				  char *addr_start, char **copy_arg)
+{
+  volatile struct gdb_exception e;
+
+  TRY_CATCH (e, RETURN_MASK_ALL)
+    {
+      parse_breakpoint_sals (arg, canonical);
+    }
+
+  /* If caller is interested in rc value from parse, set value.  */
+  switch (e.reason)
+    {
+    case RETURN_QUIT:
+      throw_exception (e);
+    case RETURN_ERROR:
+      switch (e.error)
+	{
+	case NOT_FOUND_ERROR:
+
+	  /* If pending breakpoint support is turned off, throw
+	     error.  */
+
+	  if (pending_break_support == AUTO_BOOLEAN_FALSE)
+	    throw_exception (e);
+
+	  exception_print (gdb_stderr, e);
+
+          /* If pending breakpoint support is auto query and the user
+	     selects no, then simply return the error code.  */
+	  if (pending_break_support == AUTO_BOOLEAN_AUTO
+	      && !nquery (_("Make %s pending on future shared library load? "),
+			  bptype_string (type_wanted)))
+	    return 0;
+
+	  /* At this point, either the user was queried about setting
+	     a pending breakpoint and selected yes, or pending
+	     breakpoint behavior is on and thus a pending breakpoint
+	     is defaulted on behalf of the user.  */
+	  {
+	    struct linespec_sals lsal;
+
+	    *copy_arg = xstrdup (addr_start);
+	    lsal.canonical = xstrdup (*copy_arg);
+	    lsal.sals.nelts = 1;
+	    lsal.sals.sals = XNEW (struct symtab_and_line);
+	    init_sal (&lsal.sals.sals[0]);
+	    *pending = 1;
+	    VEC_safe_push (linespec_sals, canonical->sals, &lsal);
+	  }
+	  break;
+	default:
+	  throw_exception (e);
+	}
+      break;
+    default:
+      if (VEC_empty (linespec_sals, canonical->sals))
+	return 0;
+    }
+
+  return 1;
+}
+
+/* Call create_breakpoints_sal for the given arguments.  This is the default
+   function for the `create_breakpoints_sal' method of
+   breakpoint_ops.  */
+
+static void
+create_breakpoints_sal_default (struct gdbarch *gdbarch,
+				struct linespec_result *canonical,
+				struct linespec_sals *lsal,
+				char *cond_string,
+				enum bptype type_wanted,
+				enum bpdisp disposition,
+				int thread,
+				int task, int ignore_count,
+				const struct breakpoint_ops *ops,
+				int from_tty, int enabled,
+				int internal)
+{
+  create_breakpoints_sal (gdbarch, canonical, cond_string,
+			  type_wanted, disposition,
+			  thread, task, ignore_count, ops, from_tty,
+			  enabled, internal);
+}
+
+/* Decode the line represented by S by calling decode_line_full.  This is the
+   default function for the `decode_linespec' method of breakpoint_ops.  */
+
+static void
+decode_linespec_default (struct breakpoint *b, char **s,
+			 struct symtabs_and_lines *sals)
+{
+  struct linespec_result canonical;
+
+  init_linespec_result (&canonical);
+  decode_line_full (s, DECODE_LINE_FUNFIRSTLINE,
+		    (struct symtab *) NULL, 0,
+		    &canonical, multiple_symbols_all,
+		    b->filter);
+
+  /* We should get 0 or 1 resulting SALs.  */
+  gdb_assert (VEC_length (linespec_sals, canonical.sals) < 2);
+
+  if (VEC_length (linespec_sals, canonical.sals) > 0)
+    {
+      struct linespec_sals *lsal;
+
+      lsal = VEC_index (linespec_sals, canonical.sals, 0);
+      *sals = lsal->sals;
+      /* Arrange it so the destructor does not free the
+	 contents.  */
+      lsal->sals.sals = NULL;
+    }
+
+  destroy_linespec_result (&canonical);
+}
+
 /* Prepare the global context for a re-set of breakpoint B.  */
 
 static struct cleanup *
@@ -12794,6 +12992,15 @@ ftrace_command (char *arg, int from_tty)
 void
 strace_command (char *arg, int from_tty)
 {
+  struct breakpoint_ops *ops;
+
+  /* Decide if we are dealing with a static tracepoint marker (`-m'),
+     or with a normal static tracepoint.  */
+  if (arg && strncmp (arg, "-m", 2) == 0 && isblank (arg[2]))
+    ops = &strace_marker_breakpoint_ops;
+  else
+    ops = &tracepoint_breakpoint_ops;
+
   if (create_breakpoint (get_current_arch (),
 			 arg,
 			 NULL, 0, 1 /* parse arg */,
@@ -12801,7 +13008,7 @@ strace_command (char *arg, int from_tty)
 			 bp_static_tracepoint /* type_wanted */,
 			 0 /* Ignore count */,
 			 pending_break_support,
-			 &tracepoint_breakpoint_ops,
+			 ops,
 			 from_tty,
 			 1 /* enabled */,
 			 0 /* internal */))
@@ -13446,6 +13653,9 @@ initialize_breakpoint_ops (void)
   ops->insert_location = bkpt_insert_location;
   ops->remove_location = bkpt_remove_location;
   ops->breakpoint_hit = bkpt_breakpoint_hit;
+  ops->create_sals_from_address = bkpt_create_sals_from_address;
+  ops->create_breakpoints_sal = bkpt_create_breakpoints_sal;
+  ops->decode_linespec = bkpt_decode_linespec;
 
   /* The breakpoint_ops structure to be used in regular breakpoints.  */
   ops = &bkpt_breakpoint_ops;
@@ -13526,6 +13736,16 @@ initialize_breakpoint_ops (void)
   ops->print_one_detail = tracepoint_print_one_detail;
   ops->print_mention = tracepoint_print_mention;
   ops->print_recreate = tracepoint_print_recreate;
+  ops->create_sals_from_address = tracepoint_create_sals_from_address;
+  ops->create_breakpoints_sal = tracepoint_create_breakpoints_sal;
+  ops->decode_linespec = tracepoint_decode_linespec;
+
+  /* Static tracepoints with marker (`-m').  */
+  ops = &strace_marker_breakpoint_ops;
+  *ops = tracepoint_breakpoint_ops;
+  ops->create_sals_from_address = strace_marker_create_sals_from_address;
+  ops->create_breakpoints_sal = strace_marker_create_breakpoints_sal;
+  ops->decode_linespec = strace_marker_decode_linespec;
 
   /* Fork catchpoints.  */
   ops = &catch_fork_breakpoint_ops;
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index c1d3be9..b318ca9 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -30,6 +30,8 @@ struct get_number_or_range_state;
 struct thread_info;
 struct bpstats;
 struct bp_location;
+struct linespec_result;
+struct linespec_sals;
 
 /* This is the maximum number of bytes a breakpoint instruction can
    take.  Feel free to increase it.  It's just used in a few places to
@@ -482,6 +484,38 @@ struct breakpoint_ops
 
   /* Print to FP the CLI command that recreates this breakpoint.  */
   void (*print_recreate) (struct breakpoint *, struct ui_file *fp);
+
+  /* Create SALs from address string, storing the result in linespec_result.
+     Return 1 on success, or zero otherwise.
+
+     For an explanation about the arguments, see the function
+     `create_sals_from_address_default'.
+
+     This function is called inside `create_breakpoint'.  */
+  int (*create_sals_from_address) (char **, struct linespec_result *,
+				   enum bptype, int *, char *, char **);
+
+  /* This method will be responsible for creating a breakpoint given its SALs.
+     Usually, it just calls `create_breakpoints_sal' (for ordinary
+     breakpoints).  However, there may be some special cases where we might
+     need to do some tweaks, e.g., see
+     `strace_marker_init_or_create_breakpoint_sal'.
+
+     This function is called inside `create_breakpoint'.  */
+  void (*create_breakpoints_sal) (struct gdbarch *,
+				  struct linespec_result *,
+				  struct linespec_sals *, char *,
+				  enum bptype, enum bpdisp, int, int,
+				  int, const struct breakpoint_ops *,
+				  int, int, int);
+
+  /* Given the address string (second parameter), this method decodes it
+     and provides the SAL locations related to it.  For ordinary breakpoints,
+     it calls `decode_line_full'.
+
+     This function is called inside `addr_string_to_sals'.  */
+  void (*decode_linespec) (struct breakpoint *, char **,
+			   struct symtabs_and_lines *);
 };
 
 /* Helper for breakpoint_ops->print_recreate implementations.  Prints

Re: [RFC] Make static tracepoint with markers more OO

[Pedro Alves]

On 01/13/2012 04:01 AM, Sergio Durigan Junior wrote:
> Hello there,
> 
> I have been working on this patch, and I would like some comments from
> you guys.  It basically implements new methods inside breakpoint_ops in
> order to make static tracepoint with markers (`strace -m') more OO.

Thanks.

> Currently, we use a check (defined in `is_marker_spec') in order to
> identify those tracepoints, and act accordingly.  However, it makes the
> code (especially the functions `create_breakpoint' and
> `addr_string_to_sals') very conditional.
> 
> This would be OK if it were to be the only case, but as it turns out our
> SystemTap integration patch will have to add more complexity to this
> code, which would make things uglier and uglier.  So, as a preparation
> for the second round of submissions of the SystemTap patch, I am
> submitting this patch first.
> 
> I am not sure the names I picked for the new methods are good.  I had
> spent a good time thinking about the names, but unfortunately I couldn't
> come up with something better than this.  I am specially concerned about
> the `create_breakpoints_sal*' set of methods/functions, because there
> are too many of them IMO.  I would appreciate some reviews on the
> comments of the methods, as well.  I believe the separation is pretty
> straightforward, so I think this is just moving bits here and there
> without actually implementing anything new.
> 
> Comments are welcome, as usual.

This is fine with me.

> -/* Assuming we're creating a static tracepoint, does S look like a
> -   static tracepoint marker spec ("-m MARKER_ID")?  */
> -#define is_marker_spec(s)						\
> -  (s != NULL && strncmp (s, "-m", 2) == 0 && ((s)[2] == ' ' || (s)[2] == '\t'))
> +/* Return 1 if B refers to a static tracepoint marker, zero otherwise.  */

I think

 /* Return 1 if B refers to a static tracepoint set by marker ("-m"), zero otherwise.  */

would be clearer.

> +
> +static int strace_marker_p (struct breakpoint *b);


> -      if (b->type == bp_static_tracepoint && !marker_spec)
> +      if (strace_marker_p (b))

This one looks wrong.  The old condition had a `!', so this was for
static tracepoints _not_ set through a marker.

> +
> +  /* Create SALs from address string, storing the result in linespec_result.
> +     Return 1 on success, or zero otherwise.
> +
> +     For an explanation about the arguments, see the function
> +     `create_sals_from_address_default'.
> +
> +     This function is called inside `create_breakpoint'.  */
> +  int (*create_sals_from_address) (char **, struct linespec_result *,
> +				   enum bptype, int *, char *, char **);
> +
> +  /* This method will be responsible for creating a breakpoint given its SALs.
> +     Usually, it just calls `create_breakpoints_sal' (for ordinary
> +     breakpoints).  However, there may be some special cases where we might
> +     need to do some tweaks, e.g., see
> +     `strace_marker_init_or_create_breakpoint_sal'.
> +
> +     This function is called inside `create_breakpoint'.  */
> +  void (*create_breakpoints_sal) (struct gdbarch *,
> +				  struct linespec_result *,
> +				  struct linespec_sals *, char *,
> +				  enum bptype, enum bpdisp, int, int,
> +				  int, const struct breakpoint_ops *,
> +				  int, int, int);

It's unfortunate to be calling the breakpoint's virtual methods
before the object itself is created, which will require some redesign
and refactoring if we ever switch to C++ (and is dangerous, as you may
end up touching parts of the object which are not constructed yet by
mistake), but, this is no worse than what we have now, so I'm fine with it.

> +
> +  /* Given the address string (second parameter), this method decodes it
> +     and provides the SAL locations related to it.  For ordinary breakpoints,
> +     it calls `decode_line_full'.
> +
> +     This function is called inside `addr_string_to_sals'.  */
> +  void (*decode_linespec) (struct breakpoint *, char **,
> +			   struct symtabs_and_lines *);
>  };
>  
>  /* Helper for breakpoint_ops->print_recreate implementations.  Prints

Re: [RFC] Make static tracepoint with markers more OO

[Tom Tromey]

>>>>> "Sergio" == Sergio Durigan Junior <sergiodj@redhat.com> writes:

Sergio> I have been working on this patch, and I would like some comments from
Sergio> you guys.  It basically implements new methods inside breakpoint_ops in
Sergio> order to make static tracepoint with markers (`strace -m') more OO.

I think it is very nice.  Thanks.

At first I thought that maybe the overlap between the
create_sals_from_address and decode_linespec methods could be eliminated.
But, I think the first one has to compute the text that is used
when re-evaluating, but the latter just has to re-evaluate it, and so
the savings would be minimal.

My only comment on the design is that I wonder if the handling of
pending breakpoints is done at the right place.  I assume we don't
support pending static tracepoint (not even sure if that can mean
anything) -- but I think we probably do want to support pending
SystemTap probe breakpoints.

Sergio> +  if (arg && strncmp (arg, "-m", 2) == 0 && isblank (arg[2]))

I am not sure that isblank is ok to use.  gdb doesn't use it anywhere
else.  I suggest just using the original check.

Sergio> +  /* This method will be responsible for creating a breakpoint given its SALs.
Sergio> +     Usually, it just calls `create_breakpoints_sal' (for ordinary
Sergio> +     breakpoints).  However, there may be some special cases where we might
Sergio> +     need to do some tweaks, e.g., see
Sergio> +     `strace_marker_init_or_create_breakpoint_sal'.

This function doesn't exist :)

Tom

Re: [RFC] Make static tracepoint with markers more OO

[Sergio Durigan Junior]

On Friday, January 13 2012, Pedro Alves wrote:

> On 01/13/2012 04:01 AM, Sergio Durigan Junior wrote:
>> -/* Assuming we're creating a static tracepoint, does S look like a
>> -   static tracepoint marker spec ("-m MARKER_ID")?  */
>> -#define is_marker_spec(s)						\
>> -  (s != NULL && strncmp (s, "-m", 2) == 0 && ((s)[2] == ' ' || (s)[2] == '\t'))
>> +/* Return 1 if B refers to a static tracepoint marker, zero otherwise.  */
>
> I think
>
>  /* Return 1 if B refers to a static tracepoint set by marker ("-m"), zero otherwise.  */
>
> would be clearer.

Thanks, will fix it.

>> +static int strace_marker_p (struct breakpoint *b);
>
>
>> -      if (b->type == bp_static_tracepoint && !marker_spec)
>> +      if (strace_marker_p (b))
>
> This one looks wrong.  The old condition had a `!', so this was for
> static tracepoints _not_ set through a marker.

Ops, you're right, I missed that.

>> +
>> +  /* Create SALs from address string, storing the result in linespec_result.
>> +     Return 1 on success, or zero otherwise.
>> +
>> +     For an explanation about the arguments, see the function
>> +     `create_sals_from_address_default'.
>> +
>> +     This function is called inside `create_breakpoint'.  */
>> +  int (*create_sals_from_address) (char **, struct linespec_result *,
>> +				   enum bptype, int *, char *, char **);
>> +
>> +  /* This method will be responsible for creating a breakpoint given its SALs.
>> +     Usually, it just calls `create_breakpoints_sal' (for ordinary
>> +     breakpoints).  However, there may be some special cases where we might
>> +     need to do some tweaks, e.g., see
>> +     `strace_marker_init_or_create_breakpoint_sal'.
>> +
>> +     This function is called inside `create_breakpoint'.  */
>> +  void (*create_breakpoints_sal) (struct gdbarch *,
>> +				  struct linespec_result *,
>> +				  struct linespec_sals *, char *,
>> +				  enum bptype, enum bpdisp, int, int,
>> +				  int, const struct breakpoint_ops *,
>> +				  int, int, int);
>
> It's unfortunate to be calling the breakpoint's virtual methods
> before the object itself is created, which will require some redesign
> and refactoring if we ever switch to C++ (and is dangerous, as you may
> end up touching parts of the object which are not constructed yet by
> mistake), but, this is no worse than what we have now, so I'm fine with it.

Yes, I understand what you're saying.  I couldn't figure out a better
way of handling this (except creating a "pre_breakpoint_ops"?).  Anyway,
thanks for the review, I will submit a fixed version of the patch in
Tromey's reply.

Re: [RFC] Make static tracepoint with markers more OO

[Sergio Durigan Junior]

On Friday, January 13 2012, Tom Tromey wrote:

>>>>>> "Sergio" == Sergio Durigan Junior <sergiodj@redhat.com> writes:
>
> Sergio> I have been working on this patch, and I would like some comments from
> Sergio> you guys.  It basically implements new methods inside breakpoint_ops in
> Sergio> order to make static tracepoint with markers (`strace -m') more OO.
>
> My only comment on the design is that I wonder if the handling of
> pending breakpoints is done at the right place.  I assume we don't
> support pending static tracepoint (not even sure if that can mean
> anything) -- but I think we probably do want to support pending
> SystemTap probe breakpoints.

Thanks for the review, Tom.  As we have discussed, I reworked the code
so that the pending breakpoint "handler" stays in `create_breakpoint'.
This way, I believe that if we ever want to add pending breakpoint
support on SystemTap probe breakpoints, it's going to be trivial.

I have also confirmed that the static tracepoint with markers (-m) won't
be able to reach this pending breakpoint code, just the way it was before.

> Sergio> +  if (arg && strncmp (arg, "-m", 2) == 0 && isblank (arg[2]))
>
> I am not sure that isblank is ok to use.  gdb doesn't use it anywhere
> else.  I suggest just using the original check.

Yeah, well, the original check is for space and tab, and isblank does
exactly that.  I am not sure what is the deal about using it, but I
removed as suggested.

> Sergio> +  /* This method will be responsible for creating a breakpoint given its SALs.
> Sergio> +     Usually, it just calls `create_breakpoints_sal' (for ordinary
> Sergio> +     breakpoints).  However, there may be some special cases where we might
> Sergio> +     need to do some tweaks, e.g., see
> Sergio> +     `strace_marker_init_or_create_breakpoint_sal'.
>
> This function doesn't exist :)

Ops, thanks for noticing.  I changed the names so many times that I
forgot to change this one.

The attached patch fixes all the issues pointed out by you and Pedro.  I
have also tested this on my machine using UST probes, and on testfarm.
No regressions were found.  Is it OK to apply?

Thanks.

2012-01-13  Sergio Durigan Junior  <sergiodj@redhat.com>

	* breakpoint.c (create_sals_from_address_default): New function.
	(create_breakpoints_sal_default): Likewise.
	(decode_linespec_default): Likewise.
	(is_marker_spec): Removed.
	(strace_marker_p): New function.
	(init_breakpoint_sal): Using `strace_marker_p' instead of
	`is_marker_spec'.
	(create_breakpoint): Call method `create_sals_from_address' from
	breakpoint_ops, replacing code that created SALs conditionally
	on the type of the breakpoint.  Call method `create_breakpoints_sal',
	replacing code that created breakpoints conditionally on the type
	wanted.
	(base_breakpoint_create_sals_from_address): New function.
	(base_breakpoint_create_breakpoints_sal): Likewise.
	(base_breakpoint_decode_linespec): Likewise.
	(base_breakpoint_ops): Add methods
	`base_breakpoint_create_sals_from_address',
	`base_breakpoint_create_breakpoints_sal' and
	`base_breakpoint_decode_linespec'.
	(bkpt_create_sals_from_address): New function.
	(bkpt_create_breakpoints_sal): Likewise.
	(bkpt_decode_linespec): Likewise.
	(tracepoint_create_sals_from_address): Likewise.
	(tracepoint_create_breakpoints_sal): Likewise.
	(tracepoint_decode_linespec): Likewise.
	(strace_marker_create_sals_from_address): Likewise.
	(strace_marker_create_breakpoints_sal): Likewise.
	(strace_marker_decode_linespec): Likewise.
	(strace_marker_breakpoint_ops): New variable.
	(addr_string_to_sals): Remove `marker_spec'.  Call method
	`decode_linespec' from breakpoint_ops, replacing code that decoded
	an address string into a SAL.  Use `strace_marker_p' instead of
	`marker_spec'.
	(strace_command): Decide whether we are dealing with a static
	tracepoint with marker or not.  Use the appropriate breakpoint_ops.
	(initialize_breakpoint_ops): Initialize new fields of breakpoint_ops.
	* breakpoint.h (linespec_result, linespec_sals): New forward
	declarations.
	(breakpoint_ops) <create_sals_from_address>,
	<create_breakpoints_sal>, <decode_linespec>: New methods.

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 88fc176..902c8aa 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -96,6 +96,23 @@ static int breakpoint_re_set_one (void *);
 
 static void breakpoint_re_set_default (struct breakpoint *);
 
+static void create_sals_from_address_default (char **,
+					      struct linespec_result *,
+					      enum bptype, char *,
+					      char **);
+
+static void create_breakpoints_sal_default (struct gdbarch *,
+					    struct linespec_result *,
+					    struct linespec_sals *,
+					    char *, enum bptype,
+					    enum bpdisp, int, int,
+					    int,
+					    const struct breakpoint_ops *,
+					    int, int, int);
+
+static void decode_linespec_default (struct breakpoint *, char **,
+				     struct symtabs_and_lines *);
+
 static void clear_command (char *, int);
 
 static void catch_command (char *, int);
@@ -237,10 +254,10 @@ static void trace_pass_command (char *, int);
 
 static int is_masked_watchpoint (const struct breakpoint *b);
 
-/* Assuming we're creating a static tracepoint, does S look like a
-   static tracepoint marker spec ("-m MARKER_ID")?  */
-#define is_marker_spec(s)						\
-  (s != NULL && strncmp (s, "-m", 2) == 0 && ((s)[2] == ' ' || (s)[2] == '\t'))
+/* Return 1 if B refers to a static tracepoint set by marker ("-m"), zero
+   otherwise.  */
+
+static int strace_marker_p (struct breakpoint *b);
 
 /* The abstract base class all breakpoint_ops structures inherit
    from.  */
@@ -7298,7 +7315,7 @@ init_breakpoint_sal (struct breakpoint *b, struct gdbarch *gdbarch,
 	      struct tracepoint *t = (struct tracepoint *) b;
 	      struct static_tracepoint_marker marker;
 
-	      if (is_marker_spec (addr_string))
+	      if (strace_marker_p (b))
 		{
 		  /* We already know the marker exists, otherwise, we
 		     wouldn't see a sal for it.  */
@@ -7720,32 +7737,19 @@ create_breakpoint (struct gdbarch *gdbarch,
 
   init_linespec_result (&canonical);
 
-  if (type_wanted == bp_static_tracepoint && is_marker_spec (arg))
-    {
-      int i;
-      struct linespec_sals lsal;
-
-      lsal.sals = decode_static_tracepoint_spec (&arg);
-
-      copy_arg = savestring (addr_start, arg - addr_start);
-
-      canonical.addr_string = xstrdup (copy_arg);
-      lsal.canonical = xstrdup (copy_arg);
-      VEC_safe_push (linespec_sals, canonical.sals, &lsal);
-
-      goto done;
-    }
-
   TRY_CATCH (e, RETURN_MASK_ALL)
     {
-      parse_breakpoint_sals (&arg, &canonical);
+      ops->create_sals_from_address (&arg, &canonical, type_wanted,
+				     addr_start, &copy_arg);
     }
 
   /* If caller is interested in rc value from parse, set value.  */
   switch (e.reason)
     {
-    case RETURN_QUIT:
-      throw_exception (e);
+    case GDB_NO_ERROR:
+      if (VEC_empty (linespec_sals, canonical.sals))
+	return 0;
+      break;
     case RETURN_ERROR:
       switch (e.error)
 	{
@@ -7787,12 +7791,9 @@ create_breakpoint (struct gdbarch *gdbarch,
 	}
       break;
     default:
-      if (VEC_empty (linespec_sals, canonical.sals))
-	return 0;
+      throw_exception (e);
     }
 
-  done:
-
   /* Create a chain of things that always need to be cleaned up.  */
   old_chain = make_cleanup_destroy_linespec_result (&canonical);
 
@@ -7855,57 +7856,11 @@ create_breakpoint (struct gdbarch *gdbarch,
             }
         }
 
-      /* If the user is creating a static tracepoint by marker id
-	 (strace -m MARKER_ID), then store the sals index, so that
-	 breakpoint_re_set can try to match up which of the newly
-	 found markers corresponds to this one, and, don't try to
-	 expand multiple locations for each sal, given than SALS
-	 already should contain all sals for MARKER_ID.  */
-      if (type_wanted == bp_static_tracepoint
-	  && is_marker_spec (copy_arg))
-	{
-	  int i;
-
-	  for (i = 0; i < lsal->sals.nelts; ++i)
-	    {
-	      struct symtabs_and_lines expanded;
-	      struct tracepoint *tp;
-	      struct cleanup *old_chain;
-	      char *addr_string;
-
-	      expanded.nelts = 1;
-	      expanded.sals = &lsal->sals.sals[i];
-
-	      addr_string = xstrdup (canonical.addr_string);
-	      old_chain = make_cleanup (xfree, addr_string);
-
-	      tp = XCNEW (struct tracepoint);
-	      init_breakpoint_sal (&tp->base, gdbarch, expanded,
-				   addr_string, NULL,
+      ops->create_breakpoints_sal (gdbarch, &canonical, lsal,
 				   cond_string, type_wanted,
 				   tempflag ? disp_del : disp_donttouch,
 				   thread, task, ignore_count, ops,
-				   from_tty, enabled, internal,
-				   canonical.special_display);
-	      /* Given that its possible to have multiple markers with
-		 the same string id, if the user is creating a static
-		 tracepoint by marker id ("strace -m MARKER_ID"), then
-		 store the sals index, so that breakpoint_re_set can
-		 try to match up which of the newly found markers
-		 corresponds to this one  */
-	      tp->static_trace_marker_id_idx = i;
-
-	      install_breakpoint (internal, &tp->base, 0);
-
-	      discard_cleanups (old_chain);
-	    }
-	}
-      else
-	create_breakpoints_sal (gdbarch, &canonical, cond_string,
-				type_wanted,
-				tempflag ? disp_del : disp_donttouch,
-				thread, task, ignore_count, ops, from_tty,
-				enabled, internal);
+				   from_tty, enabled, internal);
     }
   else
     {
@@ -10910,6 +10865,39 @@ base_breakpoint_print_recreate (struct breakpoint *b, struct ui_file *fp)
   internal_error_pure_virtual_called ();
 }
 
+static void
+base_breakpoint_create_sals_from_address (char **arg,
+					  struct linespec_result *canonical,
+					  enum bptype type_wanted,
+					  char *addr_start,
+					  char **copy_arg)
+{
+  internal_error_pure_virtual_called ();
+}
+
+static void
+base_breakpoint_create_breakpoints_sal (struct gdbarch *gdbarch,
+					struct linespec_result *c,
+					struct linespec_sals *lsal,
+					char *cond_string,
+					enum bptype type_wanted,
+					enum bpdisp disposition,
+					int thread,
+					int task, int ignore_count,
+					const struct breakpoint_ops *o,
+					int from_tty, int enabled,
+					int internal)
+{
+  internal_error_pure_virtual_called ();
+}
+
+static void
+base_breakpoint_decode_linespec (struct breakpoint *b, char **s,
+				 struct symtabs_and_lines *sals)
+{
+  internal_error_pure_virtual_called ();
+}
+
 static struct breakpoint_ops base_breakpoint_ops =
 {
   base_breakpoint_dtor,
@@ -10925,7 +10913,10 @@ static struct breakpoint_ops base_breakpoint_ops =
   NULL,
   base_breakpoint_print_one_detail,
   base_breakpoint_print_mention,
-  base_breakpoint_print_recreate
+  base_breakpoint_print_recreate,
+  base_breakpoint_create_sals_from_address,
+  base_breakpoint_create_breakpoints_sal,
+  base_breakpoint_decode_linespec,
 };
 
 /* Default breakpoint_ops methods.  */
@@ -11071,6 +11062,43 @@ bkpt_print_recreate (struct breakpoint *tp, struct ui_file *fp)
   print_recreate_thread (tp, fp);
 }
 
+static void
+bkpt_create_sals_from_address (char **arg,
+			       struct linespec_result *canonical,
+			       enum bptype type_wanted,
+			       char *addr_start, char **copy_arg)
+{
+  create_sals_from_address_default (arg, canonical, type_wanted,
+				    addr_start, copy_arg);
+}
+
+static void
+bkpt_create_breakpoints_sal (struct gdbarch *gdbarch,
+			     struct linespec_result *canonical,
+			     struct linespec_sals *lsal,
+			     char *cond_string,
+			     enum bptype type_wanted,
+			     enum bpdisp disposition,
+			     int thread,
+			     int task, int ignore_count,
+			     const struct breakpoint_ops *ops,
+			     int from_tty, int enabled,
+			     int internal)
+{
+  create_breakpoints_sal_default (gdbarch, canonical, lsal,
+				  cond_string, type_wanted,
+				  disposition, thread, task,
+				  ignore_count, ops, from_tty,
+				  enabled, internal);
+}
+
+static void
+bkpt_decode_linespec (struct breakpoint *b, char **s,
+		      struct symtabs_and_lines *sals)
+{
+  decode_linespec_default (b, s, sals);
+}
+
 /* Virtual table for internal breakpoints.  */
 
 static void
@@ -11297,8 +11325,145 @@ tracepoint_print_recreate (struct breakpoint *self, struct ui_file *fp)
     fprintf_unfiltered (fp, "  passcount %d\n", tp->pass_count);
 }
 
+static void
+tracepoint_create_sals_from_address (char **arg,
+				     struct linespec_result *canonical,
+				     enum bptype type_wanted,
+				     char *addr_start, char **copy_arg)
+{
+  create_sals_from_address_default (arg, canonical, type_wanted,
+				    addr_start, copy_arg);
+}
+
+static void
+tracepoint_create_breakpoints_sal (struct gdbarch *gdbarch,
+				   struct linespec_result *canonical,
+				   struct linespec_sals *lsal,
+				   char *cond_string,
+				   enum bptype type_wanted,
+				   enum bpdisp disposition,
+				   int thread,
+				   int task, int ignore_count,
+				   const struct breakpoint_ops *ops,
+				   int from_tty, int enabled,
+				   int internal)
+{
+  create_breakpoints_sal_default (gdbarch, canonical, lsal,
+				  cond_string, type_wanted,
+				  disposition, thread, task,
+				  ignore_count, ops, from_tty,
+				  enabled, internal);
+}
+
+static void
+tracepoint_decode_linespec (struct breakpoint *b, char **s,
+			    struct symtabs_and_lines *sals)
+{
+  decode_linespec_default (b, s, sals);
+}
+
 struct breakpoint_ops tracepoint_breakpoint_ops;
 
+/* The breakpoint_ops structure to be used on static tracepoints with
+   markers (`-m').  */
+
+static void
+strace_marker_create_sals_from_address (char **arg,
+					struct linespec_result *canonical,
+					enum bptype type_wanted,
+					char *addr_start, char **copy_arg)
+{
+  struct linespec_sals lsal;
+
+  lsal.sals = decode_static_tracepoint_spec (arg);
+
+  *copy_arg = savestring (addr_start, *arg - addr_start);
+
+  canonical->addr_string = xstrdup (*copy_arg);
+  lsal.canonical = xstrdup (*copy_arg);
+  VEC_safe_push (linespec_sals, canonical->sals, &lsal);
+}
+
+static void
+strace_marker_create_breakpoints_sal (struct gdbarch *gdbarch,
+				      struct linespec_result *canonical,
+				      struct linespec_sals *lsal,
+				      char *cond_string,
+				      enum bptype type_wanted,
+				      enum bpdisp disposition,
+				      int thread,
+				      int task, int ignore_count,
+				      const struct breakpoint_ops *ops,
+				      int from_tty, int enabled,
+				      int internal)
+{
+  int i;
+
+  /* If the user is creating a static tracepoint by marker id
+     (strace -m MARKER_ID), then store the sals index, so that
+     breakpoint_re_set can try to match up which of the newly
+     found markers corresponds to this one, and, don't try to
+     expand multiple locations for each sal, given than SALS
+     already should contain all sals for MARKER_ID.  */
+
+  for (i = 0; i < lsal->sals.nelts; ++i)
+    {
+      struct symtabs_and_lines expanded;
+      struct tracepoint *tp;
+      struct cleanup *old_chain;
+      char *addr_string;
+
+      expanded.nelts = 1;
+      expanded.sals = &lsal->sals.sals[i];
+
+      addr_string = xstrdup (canonical->addr_string);
+      old_chain = make_cleanup (xfree, addr_string);
+
+      tp = XCNEW (struct tracepoint);
+      init_breakpoint_sal (&tp->base, gdbarch, expanded,
+			   addr_string, NULL,
+			   cond_string, type_wanted, disposition,
+			   thread, task, ignore_count, ops,
+			   from_tty, enabled, internal,
+			   canonical->special_display);
+      /* Given that its possible to have multiple markers with
+	 the same string id, if the user is creating a static
+	 tracepoint by marker id ("strace -m MARKER_ID"), then
+	 store the sals index, so that breakpoint_re_set can
+	 try to match up which of the newly found markers
+	 corresponds to this one  */
+      tp->static_trace_marker_id_idx = i;
+
+      install_breakpoint (internal, &tp->base, 0);
+
+      discard_cleanups (old_chain);
+    }
+}
+
+static void
+strace_marker_decode_linespec (struct breakpoint *b, char **s,
+			       struct symtabs_and_lines *sals)
+{
+  struct tracepoint *tp = (struct tracepoint *) b;
+
+  *sals = decode_static_tracepoint_spec (s);
+  if (sals->nelts > tp->static_trace_marker_id_idx)
+    {
+      sals->sals[0] = sals->sals[tp->static_trace_marker_id_idx];
+      sals->nelts = 1;
+    }
+  else
+    error (_("marker %s not found"), tp->static_trace_marker_id);
+}
+
+static struct breakpoint_ops strace_marker_breakpoint_ops;
+
+static int
+strace_marker_p (struct breakpoint *b)
+{
+  return b->ops == &strace_marker_breakpoint_ops;
+}
+
 /* Delete a breakpoint and clean up all traces of it in the data
    structures.  */
 
@@ -11833,54 +11998,15 @@ static struct symtabs_and_lines
 addr_string_to_sals (struct breakpoint *b, char *addr_string, int *found)
 {
   char *s;
-  int marker_spec;
   struct symtabs_and_lines sals = {0};
   volatile struct gdb_exception e;
 
+  gdb_assert (b->ops != NULL);
   s = addr_string;
-  marker_spec = b->type == bp_static_tracepoint && is_marker_spec (s);
 
   TRY_CATCH (e, RETURN_MASK_ERROR)
     {
-      if (marker_spec)
-	{
-	  struct tracepoint *tp = (struct tracepoint *) b;
-
-	  sals = decode_static_tracepoint_spec (&s);
-	  if (sals.nelts > tp->static_trace_marker_id_idx)
-	    {
-	      sals.sals[0] = sals.sals[tp->static_trace_marker_id_idx];
-	      sals.nelts = 1;
-	    }
-	  else
-	    error (_("marker %s not found"), tp->static_trace_marker_id);
-	}
-      else
-	{
-	  struct linespec_result canonical;
-
-	  init_linespec_result (&canonical);
-	  decode_line_full (&s, DECODE_LINE_FUNFIRSTLINE,
-			    (struct symtab *) NULL, 0,
-			    &canonical, multiple_symbols_all,
-			    b->filter);
-
-	  /* We should get 0 or 1 resulting SALs.  */
-	  gdb_assert (VEC_length (linespec_sals, canonical.sals) < 2);
-
-	  if (VEC_length (linespec_sals, canonical.sals) > 0)
-	    {
-	      struct linespec_sals *lsal;
-
-	      lsal = VEC_index (linespec_sals, canonical.sals, 0);
-	      sals = lsal->sals;
-	      /* Arrange it so the destructor does not free the
-		 contents.  */
-	      lsal->sals.sals = NULL;
-	    }
-
-	  destroy_linespec_result (&canonical);
-	}
+      b->ops->decode_linespec (b, &s, &sals);
     }
   if (e.reason < 0)
     {
@@ -11933,7 +12059,7 @@ addr_string_to_sals (struct breakpoint *b, char *addr_string, int *found)
 	  b->condition_not_parsed = 0;
 	}
 
-      if (b->type == bp_static_tracepoint && !marker_spec)
+      if (b->type == bp_static_tracepoint && !strace_marker_p (b))
 	sals.sals[0] = update_static_tracepoint (b, sals.sals[0]);
 
       *found = 1;
@@ -11976,6 +12102,73 @@ breakpoint_re_set_default (struct breakpoint *b)
   update_breakpoint_locations (b, expanded, expanded_end);
 }
 
+/* Default method for creating SALs from an address string.  It basically
+   calls parse_breakpoint_sals.  Return 1 for success, zero for failure.  */
+
+static void
+create_sals_from_address_default (char **arg,
+				  struct linespec_result *canonical,
+				  enum bptype type_wanted,
+				  char *addr_start, char **copy_arg)
+{
+  parse_breakpoint_sals (arg, canonical);
+}
+
+/* Call create_breakpoints_sal for the given arguments.  This is the default
+   function for the `create_breakpoints_sal' method of
+   breakpoint_ops.  */
+
+static void
+create_breakpoints_sal_default (struct gdbarch *gdbarch,
+				struct linespec_result *canonical,
+				struct linespec_sals *lsal,
+				char *cond_string,
+				enum bptype type_wanted,
+				enum bpdisp disposition,
+				int thread,
+				int task, int ignore_count,
+				const struct breakpoint_ops *ops,
+				int from_tty, int enabled,
+				int internal)
+{
+  create_breakpoints_sal (gdbarch, canonical, cond_string,
+			  type_wanted, disposition,
+			  thread, task, ignore_count, ops, from_tty,
+			  enabled, internal);
+}
+
+/* Decode the line represented by S by calling decode_line_full.  This is the
+   default function for the `decode_linespec' method of breakpoint_ops.  */
+
+static void
+decode_linespec_default (struct breakpoint *b, char **s,
+			 struct symtabs_and_lines *sals)
+{
+  struct linespec_result canonical;
+
+  init_linespec_result (&canonical);
+  decode_line_full (s, DECODE_LINE_FUNFIRSTLINE,
+		    (struct symtab *) NULL, 0,
+		    &canonical, multiple_symbols_all,
+		    b->filter);
+
+  /* We should get 0 or 1 resulting SALs.  */
+  gdb_assert (VEC_length (linespec_sals, canonical.sals) < 2);
+
+  if (VEC_length (linespec_sals, canonical.sals) > 0)
+    {
+      struct linespec_sals *lsal;
+
+      lsal = VEC_index (linespec_sals, canonical.sals, 0);
+      *sals = lsal->sals;
+      /* Arrange it so the destructor does not free the
+	 contents.  */
+      lsal->sals.sals = NULL;
+    }
+
+  destroy_linespec_result (&canonical);
+}
+
 /* Prepare the global context for a re-set of breakpoint B.  */
 
 static struct cleanup *
@@ -12794,6 +12987,16 @@ ftrace_command (char *arg, int from_tty)
 void
 strace_command (char *arg, int from_tty)
 {
+  struct breakpoint_ops *ops;
+
+  /* Decide if we are dealing with a static tracepoint marker (`-m'),
+     or with a normal static tracepoint.  */
+  if (arg && strncmp (arg, "-m", 2) == 0 && (isspace (arg[2])
+					     || arg[2] == '\t'))
+    ops = &strace_marker_breakpoint_ops;
+  else
+    ops = &tracepoint_breakpoint_ops;
+
   if (create_breakpoint (get_current_arch (),
 			 arg,
 			 NULL, 0, 1 /* parse arg */,
@@ -12801,7 +13004,7 @@ strace_command (char *arg, int from_tty)
 			 bp_static_tracepoint /* type_wanted */,
 			 0 /* Ignore count */,
 			 pending_break_support,
-			 &tracepoint_breakpoint_ops,
+			 ops,
 			 from_tty,
 			 1 /* enabled */,
 			 0 /* internal */))
@@ -13446,6 +13649,9 @@ initialize_breakpoint_ops (void)
   ops->insert_location = bkpt_insert_location;
   ops->remove_location = bkpt_remove_location;
   ops->breakpoint_hit = bkpt_breakpoint_hit;
+  ops->create_sals_from_address = bkpt_create_sals_from_address;
+  ops->create_breakpoints_sal = bkpt_create_breakpoints_sal;
+  ops->decode_linespec = bkpt_decode_linespec;
 
   /* The breakpoint_ops structure to be used in regular breakpoints.  */
   ops = &bkpt_breakpoint_ops;
@@ -13526,6 +13732,16 @@ initialize_breakpoint_ops (void)
   ops->print_one_detail = tracepoint_print_one_detail;
   ops->print_mention = tracepoint_print_mention;
   ops->print_recreate = tracepoint_print_recreate;
+  ops->create_sals_from_address = tracepoint_create_sals_from_address;
+  ops->create_breakpoints_sal = tracepoint_create_breakpoints_sal;
+  ops->decode_linespec = tracepoint_decode_linespec;
+
+  /* Static tracepoints with marker (`-m').  */
+  ops = &strace_marker_breakpoint_ops;
+  *ops = tracepoint_breakpoint_ops;
+  ops->create_sals_from_address = strace_marker_create_sals_from_address;
+  ops->create_breakpoints_sal = strace_marker_create_breakpoints_sal;
+  ops->decode_linespec = strace_marker_decode_linespec;
 
   /* Fork catchpoints.  */
   ops = &catch_fork_breakpoint_ops;
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index c1d3be9..8a8d5f2 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -30,6 +30,8 @@ struct get_number_or_range_state;
 struct thread_info;
 struct bpstats;
 struct bp_location;
+struct linespec_result;
+struct linespec_sals;
 
 /* This is the maximum number of bytes a breakpoint instruction can
    take.  Feel free to increase it.  It's just used in a few places to
@@ -482,6 +484,37 @@ struct breakpoint_ops
 
   /* Print to FP the CLI command that recreates this breakpoint.  */
   void (*print_recreate) (struct breakpoint *, struct ui_file *fp);
+
+  /* Create SALs from address string, storing the result in linespec_result.
+
+     For an explanation about the arguments, see the function
+     `create_sals_from_address_default'.
+
+     This function is called inside `create_breakpoint'.  */
+  void (*create_sals_from_address) (char **, struct linespec_result *,
+				    enum bptype, char *, char **);
+
+  /* This method will be responsible for creating a breakpoint given its SALs.
+     Usually, it just calls `create_breakpoints_sal' (for ordinary
+     breakpoints).  However, there may be some special cases where we might
+     need to do some tweaks, e.g., see
+     `strace_marker_create_breakpoints_sal'.
+
+     This function is called inside `create_breakpoint'.  */
+  void (*create_breakpoints_sal) (struct gdbarch *,
+				  struct linespec_result *,
+				  struct linespec_sals *, char *,
+				  enum bptype, enum bpdisp, int, int,
+				  int, const struct breakpoint_ops *,
+				  int, int, int);
+
+  /* Given the address string (second parameter), this method decodes it
+     and provides the SAL locations related to it.  For ordinary breakpoints,
+     it calls `decode_line_full'.
+
+     This function is called inside `addr_string_to_sals'.  */
+  void (*decode_linespec) (struct breakpoint *, char **,
+			   struct symtabs_and_lines *);
 };
 
 /* Helper for breakpoint_ops->print_recreate implementations.  Prints

Re: [RFC] Make static tracepoint with markers more OO

[Pedro Alves]

On 01/13/2012 05:09 PM, Sergio Durigan Junior wrote:
>> > It's unfortunate to be calling the breakpoint's virtual methods
>> > before the object itself is created, which will require some redesign
>> > and refactoring if we ever switch to C++ (and is dangerous, as you may
>> > end up touching parts of the object which are not constructed yet by
>> > mistake), but, this is no worse than what we have now, so I'm fine with it.
> Yes, I understand what you're saying.  I couldn't figure out a better
> way of handling this (except creating a "pre_breakpoint_ops"?).

Yeah, something like a factory object, along with trying to push out of
init_breakpoint_sal, etc. breakpoint-type specific bits, up into the callers, so
to get rid of layer inversion.  Anyway, don't worry about it now.  Your patch
is already a good cleanup.

> Anyway, thanks for the review, I will submit a fixed version of the patch in
> Tromey's reply.

-- 
Pedro Alves

Re: [RFC] Make static tracepoint with markers more OO

[Tom Tromey]

>>>>> "Sergio" == Sergio Durigan Junior <sergiodj@redhat.com> writes:

Sergio> +  if (arg && strncmp (arg, "-m", 2) == 0 && isblank (arg[2]))

Tom> I am not sure that isblank is ok to use.  gdb doesn't use it anywhere
Tom> else.  I suggest just using the original check.

Sergio> Yeah, well, the original check is for space and tab, and isblank does
Sergio> exactly that.  I am not sure what is the deal about using it, but I
Sergio> removed as suggested.

I believe isblank was added in C99, but gdb generally uses C90.

These days, you can pretty much search for "gnulib $FUNCTION" and find
all the information you need:

    http://www.gnu.org/software/gnulib/manual/html_node/isblank.html

I wouldn't mind us upgrading, or using more of gnulib, or both.

Sergio> The attached patch fixes all the issues pointed out by you and Pedro.  I
Sergio> have also tested this on my machine using UST probes, and on testfarm.
Sergio> No regressions were found.  Is it OK to apply?

Sergio> +  if (arg && strncmp (arg, "-m", 2) == 0 && (isspace (arg[2])
Sergio> +					     || arg[2] == '\t'))

isspace also looks for \t, so please remove the || clause.

I have no further comments, but I would rather Pedro give the final OK
on this.

Tom

Re: [RFC] Make static tracepoint with markers more OO

[Sergio Durigan Junior]

On Monday, January 16 2012, Tom Tromey wrote:

>>>>>> "Sergio" == Sergio Durigan Junior <sergiodj@redhat.com> writes:
>
> Sergio> +  if (arg && strncmp (arg, "-m", 2) == 0 && isblank (arg[2]))
>
> Tom> I am not sure that isblank is ok to use.  gdb doesn't use it anywhere
> Tom> else.  I suggest just using the original check.
>
> Sergio> Yeah, well, the original check is for space and tab, and isblank does
> Sergio> exactly that.  I am not sure what is the deal about using it, but I
> Sergio> removed as suggested.
>
> I believe isblank was added in C99, but gdb generally uses C90.

Oh, I see the problem.  Thanks for the info!

> Sergio> The attached patch fixes all the issues pointed out by you and Pedro.  I
> Sergio> have also tested this on my machine using UST probes, and on testfarm.
> Sergio> No regressions were found.  Is it OK to apply?
>
> Sergio> +  if (arg && strncmp (arg, "-m", 2) == 0 && (isspace (arg[2])
> Sergio> +					     || arg[2] == '\t'))
>
> isspace also looks for \t, so please remove the || clause.

Heh, that was the reason for using `isblank'.  Thanks for the review.

> I have no further comments, but I would rather Pedro give the final OK
> on this.

Ok.  Here is the final version of the patch, then.

Thanks.

2012-01-16  Sergio Durigan Junior  <sergiodj@redhat.com>

	* breakpoint.c (create_sals_from_address_default): New function.
	(create_breakpoints_sal_default): Likewise.
	(decode_linespec_default): Likewise.
	(is_marker_spec): Removed.
	(strace_marker_p): New function.
	(init_breakpoint_sal): Using `strace_marker_p' instead of
	`is_marker_spec'.
	(create_breakpoint): Call method `create_sals_from_address' from
	breakpoint_ops, replacing code that created SALs conditionally
	on the type of the breakpoint.  Call method `create_breakpoints_sal',
	replacing code that created breakpoints conditionally on the type
	wanted.
	(base_breakpoint_create_sals_from_address): New function.
	(base_breakpoint_create_breakpoints_sal): Likewise.
	(base_breakpoint_decode_linespec): Likewise.
	(base_breakpoint_ops): Add methods
	`base_breakpoint_create_sals_from_address',
	`base_breakpoint_create_breakpoints_sal' and
	`base_breakpoint_decode_linespec'.
	(bkpt_create_sals_from_address): New function.
	(bkpt_create_breakpoints_sal): Likewise.
	(bkpt_decode_linespec): Likewise.
	(tracepoint_create_sals_from_address): Likewise.
	(tracepoint_create_breakpoints_sal): Likewise.
	(tracepoint_decode_linespec): Likewise.
	(strace_marker_create_sals_from_address): Likewise.
	(strace_marker_create_breakpoints_sal): Likewise.
	(strace_marker_decode_linespec): Likewise.
	(strace_marker_breakpoint_ops): New variable.
	(addr_string_to_sals): Remove `marker_spec'.  Call method
	`decode_linespec' from breakpoint_ops, replacing code that decoded
	an address string into a SAL.  Use `strace_marker_p' instead of
	`marker_spec'.
	(strace_command): Decide whether we are dealing with a static
	tracepoint with marker or not.  Use the appropriate breakpoint_ops.
	(initialize_breakpoint_ops): Initialize new fields of breakpoint_ops.
	* breakpoint.h (linespec_result, linespec_sals): New forward
	declarations.
	(breakpoint_ops) <create_sals_from_address>,
	<create_breakpoints_sal>, <decode_linespec>: New methods.

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 88fc176..e9d29ff 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -96,6 +96,23 @@ static int breakpoint_re_set_one (void *);
 
 static void breakpoint_re_set_default (struct breakpoint *);
 
+static void create_sals_from_address_default (char **,
+					      struct linespec_result *,
+					      enum bptype, char *,
+					      char **);
+
+static void create_breakpoints_sal_default (struct gdbarch *,
+					    struct linespec_result *,
+					    struct linespec_sals *,
+					    char *, enum bptype,
+					    enum bpdisp, int, int,
+					    int,
+					    const struct breakpoint_ops *,
+					    int, int, int);
+
+static void decode_linespec_default (struct breakpoint *, char **,
+				     struct symtabs_and_lines *);
+
 static void clear_command (char *, int);
 
 static void catch_command (char *, int);
@@ -237,10 +254,10 @@ static void trace_pass_command (char *, int);
 
 static int is_masked_watchpoint (const struct breakpoint *b);
 
-/* Assuming we're creating a static tracepoint, does S look like a
-   static tracepoint marker spec ("-m MARKER_ID")?  */
-#define is_marker_spec(s)						\
-  (s != NULL && strncmp (s, "-m", 2) == 0 && ((s)[2] == ' ' || (s)[2] == '\t'))
+/* Return 1 if B refers to a static tracepoint set by marker ("-m"), zero
+   otherwise.  */
+
+static int strace_marker_p (struct breakpoint *b);
 
 /* The abstract base class all breakpoint_ops structures inherit
    from.  */
@@ -7298,7 +7315,7 @@ init_breakpoint_sal (struct breakpoint *b, struct gdbarch *gdbarch,
 	      struct tracepoint *t = (struct tracepoint *) b;
 	      struct static_tracepoint_marker marker;
 
-	      if (is_marker_spec (addr_string))
+	      if (strace_marker_p (b))
 		{
 		  /* We already know the marker exists, otherwise, we
 		     wouldn't see a sal for it.  */
@@ -7720,32 +7737,19 @@ create_breakpoint (struct gdbarch *gdbarch,
 
   init_linespec_result (&canonical);
 
-  if (type_wanted == bp_static_tracepoint && is_marker_spec (arg))
-    {
-      int i;
-      struct linespec_sals lsal;
-
-      lsal.sals = decode_static_tracepoint_spec (&arg);
-
-      copy_arg = savestring (addr_start, arg - addr_start);
-
-      canonical.addr_string = xstrdup (copy_arg);
-      lsal.canonical = xstrdup (copy_arg);
-      VEC_safe_push (linespec_sals, canonical.sals, &lsal);
-
-      goto done;
-    }
-
   TRY_CATCH (e, RETURN_MASK_ALL)
     {
-      parse_breakpoint_sals (&arg, &canonical);
+      ops->create_sals_from_address (&arg, &canonical, type_wanted,
+				     addr_start, &copy_arg);
     }
 
   /* If caller is interested in rc value from parse, set value.  */
   switch (e.reason)
     {
-    case RETURN_QUIT:
-      throw_exception (e);
+    case GDB_NO_ERROR:
+      if (VEC_empty (linespec_sals, canonical.sals))
+	return 0;
+      break;
     case RETURN_ERROR:
       switch (e.error)
 	{
@@ -7787,12 +7791,9 @@ create_breakpoint (struct gdbarch *gdbarch,
 	}
       break;
     default:
-      if (VEC_empty (linespec_sals, canonical.sals))
-	return 0;
+      throw_exception (e);
     }
 
-  done:
-
   /* Create a chain of things that always need to be cleaned up.  */
   old_chain = make_cleanup_destroy_linespec_result (&canonical);
 
@@ -7855,57 +7856,11 @@ create_breakpoint (struct gdbarch *gdbarch,
             }
         }
 
-      /* If the user is creating a static tracepoint by marker id
-	 (strace -m MARKER_ID), then store the sals index, so that
-	 breakpoint_re_set can try to match up which of the newly
-	 found markers corresponds to this one, and, don't try to
-	 expand multiple locations for each sal, given than SALS
-	 already should contain all sals for MARKER_ID.  */
-      if (type_wanted == bp_static_tracepoint
-	  && is_marker_spec (copy_arg))
-	{
-	  int i;
-
-	  for (i = 0; i < lsal->sals.nelts; ++i)
-	    {
-	      struct symtabs_and_lines expanded;
-	      struct tracepoint *tp;
-	      struct cleanup *old_chain;
-	      char *addr_string;
-
-	      expanded.nelts = 1;
-	      expanded.sals = &lsal->sals.sals[i];
-
-	      addr_string = xstrdup (canonical.addr_string);
-	      old_chain = make_cleanup (xfree, addr_string);
-
-	      tp = XCNEW (struct tracepoint);
-	      init_breakpoint_sal (&tp->base, gdbarch, expanded,
-				   addr_string, NULL,
+      ops->create_breakpoints_sal (gdbarch, &canonical, lsal,
 				   cond_string, type_wanted,
 				   tempflag ? disp_del : disp_donttouch,
 				   thread, task, ignore_count, ops,
-				   from_tty, enabled, internal,
-				   canonical.special_display);
-	      /* Given that its possible to have multiple markers with
-		 the same string id, if the user is creating a static
-		 tracepoint by marker id ("strace -m MARKER_ID"), then
-		 store the sals index, so that breakpoint_re_set can
-		 try to match up which of the newly found markers
-		 corresponds to this one  */
-	      tp->static_trace_marker_id_idx = i;
-
-	      install_breakpoint (internal, &tp->base, 0);
-
-	      discard_cleanups (old_chain);
-	    }
-	}
-      else
-	create_breakpoints_sal (gdbarch, &canonical, cond_string,
-				type_wanted,
-				tempflag ? disp_del : disp_donttouch,
-				thread, task, ignore_count, ops, from_tty,
-				enabled, internal);
+				   from_tty, enabled, internal);
     }
   else
     {
@@ -10910,6 +10865,39 @@ base_breakpoint_print_recreate (struct breakpoint *b, struct ui_file *fp)
   internal_error_pure_virtual_called ();
 }
 
+static void
+base_breakpoint_create_sals_from_address (char **arg,
+					  struct linespec_result *canonical,
+					  enum bptype type_wanted,
+					  char *addr_start,
+					  char **copy_arg)
+{
+  internal_error_pure_virtual_called ();
+}
+
+static void
+base_breakpoint_create_breakpoints_sal (struct gdbarch *gdbarch,
+					struct linespec_result *c,
+					struct linespec_sals *lsal,
+					char *cond_string,
+					enum bptype type_wanted,
+					enum bpdisp disposition,
+					int thread,
+					int task, int ignore_count,
+					const struct breakpoint_ops *o,
+					int from_tty, int enabled,
+					int internal)
+{
+  internal_error_pure_virtual_called ();
+}
+
+static void
+base_breakpoint_decode_linespec (struct breakpoint *b, char **s,
+				 struct symtabs_and_lines *sals)
+{
+  internal_error_pure_virtual_called ();
+}
+
 static struct breakpoint_ops base_breakpoint_ops =
 {
   base_breakpoint_dtor,
@@ -10925,7 +10913,10 @@ static struct breakpoint_ops base_breakpoint_ops =
   NULL,
   base_breakpoint_print_one_detail,
   base_breakpoint_print_mention,
-  base_breakpoint_print_recreate
+  base_breakpoint_print_recreate,
+  base_breakpoint_create_sals_from_address,
+  base_breakpoint_create_breakpoints_sal,
+  base_breakpoint_decode_linespec,
 };
 
 /* Default breakpoint_ops methods.  */
@@ -11071,6 +11062,43 @@ bkpt_print_recreate (struct breakpoint *tp, struct ui_file *fp)
   print_recreate_thread (tp, fp);
 }
 
+static void
+bkpt_create_sals_from_address (char **arg,
+			       struct linespec_result *canonical,
+			       enum bptype type_wanted,
+			       char *addr_start, char **copy_arg)
+{
+  create_sals_from_address_default (arg, canonical, type_wanted,
+				    addr_start, copy_arg);
+}
+
+static void
+bkpt_create_breakpoints_sal (struct gdbarch *gdbarch,
+			     struct linespec_result *canonical,
+			     struct linespec_sals *lsal,
+			     char *cond_string,
+			     enum bptype type_wanted,
+			     enum bpdisp disposition,
+			     int thread,
+			     int task, int ignore_count,
+			     const struct breakpoint_ops *ops,
+			     int from_tty, int enabled,
+			     int internal)
+{
+  create_breakpoints_sal_default (gdbarch, canonical, lsal,
+				  cond_string, type_wanted,
+				  disposition, thread, task,
+				  ignore_count, ops, from_tty,
+				  enabled, internal);
+}
+
+static void
+bkpt_decode_linespec (struct breakpoint *b, char **s,
+		      struct symtabs_and_lines *sals)
+{
+  decode_linespec_default (b, s, sals);
+}
+
 /* Virtual table for internal breakpoints.  */
 
 static void
@@ -11297,8 +11325,145 @@ tracepoint_print_recreate (struct breakpoint *self, struct ui_file *fp)
     fprintf_unfiltered (fp, "  passcount %d\n", tp->pass_count);
 }
 
+static void
+tracepoint_create_sals_from_address (char **arg,
+				     struct linespec_result *canonical,
+				     enum bptype type_wanted,
+				     char *addr_start, char **copy_arg)
+{
+  create_sals_from_address_default (arg, canonical, type_wanted,
+				    addr_start, copy_arg);
+}
+
+static void
+tracepoint_create_breakpoints_sal (struct gdbarch *gdbarch,
+				   struct linespec_result *canonical,
+				   struct linespec_sals *lsal,
+				   char *cond_string,
+				   enum bptype type_wanted,
+				   enum bpdisp disposition,
+				   int thread,
+				   int task, int ignore_count,
+				   const struct breakpoint_ops *ops,
+				   int from_tty, int enabled,
+				   int internal)
+{
+  create_breakpoints_sal_default (gdbarch, canonical, lsal,
+				  cond_string, type_wanted,
+				  disposition, thread, task,
+				  ignore_count, ops, from_tty,
+				  enabled, internal);
+}
+
+static void
+tracepoint_decode_linespec (struct breakpoint *b, char **s,
+			    struct symtabs_and_lines *sals)
+{
+  decode_linespec_default (b, s, sals);
+}
+
 struct breakpoint_ops tracepoint_breakpoint_ops;
 
+/* The breakpoint_ops structure to be used on static tracepoints with
+   markers (`-m').  */
+
+static void
+strace_marker_create_sals_from_address (char **arg,
+					struct linespec_result *canonical,
+					enum bptype type_wanted,
+					char *addr_start, char **copy_arg)
+{
+  struct linespec_sals lsal;
+
+  lsal.sals = decode_static_tracepoint_spec (arg);
+
+  *copy_arg = savestring (addr_start, *arg - addr_start);
+
+  canonical->addr_string = xstrdup (*copy_arg);
+  lsal.canonical = xstrdup (*copy_arg);
+  VEC_safe_push (linespec_sals, canonical->sals, &lsal);
+}
+
+static void
+strace_marker_create_breakpoints_sal (struct gdbarch *gdbarch,
+				      struct linespec_result *canonical,
+				      struct linespec_sals *lsal,
+				      char *cond_string,
+				      enum bptype type_wanted,
+				      enum bpdisp disposition,
+				      int thread,
+				      int task, int ignore_count,
+				      const struct breakpoint_ops *ops,
+				      int from_tty, int enabled,
+				      int internal)
+{
+  int i;
+
+  /* If the user is creating a static tracepoint by marker id
+     (strace -m MARKER_ID), then store the sals index, so that
+     breakpoint_re_set can try to match up which of the newly
+     found markers corresponds to this one, and, don't try to
+     expand multiple locations for each sal, given than SALS
+     already should contain all sals for MARKER_ID.  */
+
+  for (i = 0; i < lsal->sals.nelts; ++i)
+    {
+      struct symtabs_and_lines expanded;
+      struct tracepoint *tp;
+      struct cleanup *old_chain;
+      char *addr_string;
+
+      expanded.nelts = 1;
+      expanded.sals = &lsal->sals.sals[i];
+
+      addr_string = xstrdup (canonical->addr_string);
+      old_chain = make_cleanup (xfree, addr_string);
+
+      tp = XCNEW (struct tracepoint);
+      init_breakpoint_sal (&tp->base, gdbarch, expanded,
+			   addr_string, NULL,
+			   cond_string, type_wanted, disposition,
+			   thread, task, ignore_count, ops,
+			   from_tty, enabled, internal,
+			   canonical->special_display);
+      /* Given that its possible to have multiple markers with
+	 the same string id, if the user is creating a static
+	 tracepoint by marker id ("strace -m MARKER_ID"), then
+	 store the sals index, so that breakpoint_re_set can
+	 try to match up which of the newly found markers
+	 corresponds to this one  */
+      tp->static_trace_marker_id_idx = i;
+
+      install_breakpoint (internal, &tp->base, 0);
+
+      discard_cleanups (old_chain);
+    }
+}
+
+static void
+strace_marker_decode_linespec (struct breakpoint *b, char **s,
+			       struct symtabs_and_lines *sals)
+{
+  struct tracepoint *tp = (struct tracepoint *) b;
+
+  *sals = decode_static_tracepoint_spec (s);
+  if (sals->nelts > tp->static_trace_marker_id_idx)
+    {
+      sals->sals[0] = sals->sals[tp->static_trace_marker_id_idx];
+      sals->nelts = 1;
+    }
+  else
+    error (_("marker %s not found"), tp->static_trace_marker_id);
+}
+
+static struct breakpoint_ops strace_marker_breakpoint_ops;
+
+static int
+strace_marker_p (struct breakpoint *b)
+{
+  return b->ops == &strace_marker_breakpoint_ops;
+}
+
 /* Delete a breakpoint and clean up all traces of it in the data
    structures.  */
 
@@ -11833,54 +11998,15 @@ static struct symtabs_and_lines
 addr_string_to_sals (struct breakpoint *b, char *addr_string, int *found)
 {
   char *s;
-  int marker_spec;
   struct symtabs_and_lines sals = {0};
   volatile struct gdb_exception e;
 
+  gdb_assert (b->ops != NULL);
   s = addr_string;
-  marker_spec = b->type == bp_static_tracepoint && is_marker_spec (s);
 
   TRY_CATCH (e, RETURN_MASK_ERROR)
     {
-      if (marker_spec)
-	{
-	  struct tracepoint *tp = (struct tracepoint *) b;
-
-	  sals = decode_static_tracepoint_spec (&s);
-	  if (sals.nelts > tp->static_trace_marker_id_idx)
-	    {
-	      sals.sals[0] = sals.sals[tp->static_trace_marker_id_idx];
-	      sals.nelts = 1;
-	    }
-	  else
-	    error (_("marker %s not found"), tp->static_trace_marker_id);
-	}
-      else
-	{
-	  struct linespec_result canonical;
-
-	  init_linespec_result (&canonical);
-	  decode_line_full (&s, DECODE_LINE_FUNFIRSTLINE,
-			    (struct symtab *) NULL, 0,
-			    &canonical, multiple_symbols_all,
-			    b->filter);
-
-	  /* We should get 0 or 1 resulting SALs.  */
-	  gdb_assert (VEC_length (linespec_sals, canonical.sals) < 2);
-
-	  if (VEC_length (linespec_sals, canonical.sals) > 0)
-	    {
-	      struct linespec_sals *lsal;
-
-	      lsal = VEC_index (linespec_sals, canonical.sals, 0);
-	      sals = lsal->sals;
-	      /* Arrange it so the destructor does not free the
-		 contents.  */
-	      lsal->sals.sals = NULL;
-	    }
-
-	  destroy_linespec_result (&canonical);
-	}
+      b->ops->decode_linespec (b, &s, &sals);
     }
   if (e.reason < 0)
     {
@@ -11933,7 +12059,7 @@ addr_string_to_sals (struct breakpoint *b, char *addr_string, int *found)
 	  b->condition_not_parsed = 0;
 	}
 
-      if (b->type == bp_static_tracepoint && !marker_spec)
+      if (b->type == bp_static_tracepoint && !strace_marker_p (b))
 	sals.sals[0] = update_static_tracepoint (b, sals.sals[0]);
 
       *found = 1;
@@ -11976,6 +12102,73 @@ breakpoint_re_set_default (struct breakpoint *b)
   update_breakpoint_locations (b, expanded, expanded_end);
 }
 
+/* Default method for creating SALs from an address string.  It basically
+   calls parse_breakpoint_sals.  Return 1 for success, zero for failure.  */
+
+static void
+create_sals_from_address_default (char **arg,
+				  struct linespec_result *canonical,
+				  enum bptype type_wanted,
+				  char *addr_start, char **copy_arg)
+{
+  parse_breakpoint_sals (arg, canonical);
+}
+
+/* Call create_breakpoints_sal for the given arguments.  This is the default
+   function for the `create_breakpoints_sal' method of
+   breakpoint_ops.  */
+
+static void
+create_breakpoints_sal_default (struct gdbarch *gdbarch,
+				struct linespec_result *canonical,
+				struct linespec_sals *lsal,
+				char *cond_string,
+				enum bptype type_wanted,
+				enum bpdisp disposition,
+				int thread,
+				int task, int ignore_count,
+				const struct breakpoint_ops *ops,
+				int from_tty, int enabled,
+				int internal)
+{
+  create_breakpoints_sal (gdbarch, canonical, cond_string,
+			  type_wanted, disposition,
+			  thread, task, ignore_count, ops, from_tty,
+			  enabled, internal);
+}
+
+/* Decode the line represented by S by calling decode_line_full.  This is the
+   default function for the `decode_linespec' method of breakpoint_ops.  */
+
+static void
+decode_linespec_default (struct breakpoint *b, char **s,
+			 struct symtabs_and_lines *sals)
+{
+  struct linespec_result canonical;
+
+  init_linespec_result (&canonical);
+  decode_line_full (s, DECODE_LINE_FUNFIRSTLINE,
+		    (struct symtab *) NULL, 0,
+		    &canonical, multiple_symbols_all,
+		    b->filter);
+
+  /* We should get 0 or 1 resulting SALs.  */
+  gdb_assert (VEC_length (linespec_sals, canonical.sals) < 2);
+
+  if (VEC_length (linespec_sals, canonical.sals) > 0)
+    {
+      struct linespec_sals *lsal;
+
+      lsal = VEC_index (linespec_sals, canonical.sals, 0);
+      *sals = lsal->sals;
+      /* Arrange it so the destructor does not free the
+	 contents.  */
+      lsal->sals.sals = NULL;
+    }
+
+  destroy_linespec_result (&canonical);
+}
+
 /* Prepare the global context for a re-set of breakpoint B.  */
 
 static struct cleanup *
@@ -12794,6 +12987,15 @@ ftrace_command (char *arg, int from_tty)
 void
 strace_command (char *arg, int from_tty)
 {
+  struct breakpoint_ops *ops;
+
+  /* Decide if we are dealing with a static tracepoint marker (`-m'),
+     or with a normal static tracepoint.  */
+  if (arg && strncmp (arg, "-m", 2) == 0 && isspace (arg[2]))
+    ops = &strace_marker_breakpoint_ops;
+  else
+    ops = &tracepoint_breakpoint_ops;
+
   if (create_breakpoint (get_current_arch (),
 			 arg,
 			 NULL, 0, 1 /* parse arg */,
@@ -12801,7 +13003,7 @@ strace_command (char *arg, int from_tty)
 			 bp_static_tracepoint /* type_wanted */,
 			 0 /* Ignore count */,
 			 pending_break_support,
-			 &tracepoint_breakpoint_ops,
+			 ops,
 			 from_tty,
 			 1 /* enabled */,
 			 0 /* internal */))
@@ -13446,6 +13648,9 @@ initialize_breakpoint_ops (void)
   ops->insert_location = bkpt_insert_location;
   ops->remove_location = bkpt_remove_location;
   ops->breakpoint_hit = bkpt_breakpoint_hit;
+  ops->create_sals_from_address = bkpt_create_sals_from_address;
+  ops->create_breakpoints_sal = bkpt_create_breakpoints_sal;
+  ops->decode_linespec = bkpt_decode_linespec;
 
   /* The breakpoint_ops structure to be used in regular breakpoints.  */
   ops = &bkpt_breakpoint_ops;
@@ -13526,6 +13731,16 @@ initialize_breakpoint_ops (void)
   ops->print_one_detail = tracepoint_print_one_detail;
   ops->print_mention = tracepoint_print_mention;
   ops->print_recreate = tracepoint_print_recreate;
+  ops->create_sals_from_address = tracepoint_create_sals_from_address;
+  ops->create_breakpoints_sal = tracepoint_create_breakpoints_sal;
+  ops->decode_linespec = tracepoint_decode_linespec;
+
+  /* Static tracepoints with marker (`-m').  */
+  ops = &strace_marker_breakpoint_ops;
+  *ops = tracepoint_breakpoint_ops;
+  ops->create_sals_from_address = strace_marker_create_sals_from_address;
+  ops->create_breakpoints_sal = strace_marker_create_breakpoints_sal;
+  ops->decode_linespec = strace_marker_decode_linespec;
 
   /* Fork catchpoints.  */
   ops = &catch_fork_breakpoint_ops;
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index c1d3be9..8a8d5f2 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -30,6 +30,8 @@ struct get_number_or_range_state;
 struct thread_info;
 struct bpstats;
 struct bp_location;
+struct linespec_result;
+struct linespec_sals;
 
 /* This is the maximum number of bytes a breakpoint instruction can
    take.  Feel free to increase it.  It's just used in a few places to
@@ -482,6 +484,37 @@ struct breakpoint_ops
 
   /* Print to FP the CLI command that recreates this breakpoint.  */
   void (*print_recreate) (struct breakpoint *, struct ui_file *fp);
+
+  /* Create SALs from address string, storing the result in linespec_result.
+
+     For an explanation about the arguments, see the function
+     `create_sals_from_address_default'.
+
+     This function is called inside `create_breakpoint'.  */
+  void (*create_sals_from_address) (char **, struct linespec_result *,
+				    enum bptype, char *, char **);
+
+  /* This method will be responsible for creating a breakpoint given its SALs.
+     Usually, it just calls `create_breakpoints_sal' (for ordinary
+     breakpoints).  However, there may be some special cases where we might
+     need to do some tweaks, e.g., see
+     `strace_marker_create_breakpoints_sal'.
+
+     This function is called inside `create_breakpoint'.  */
+  void (*create_breakpoints_sal) (struct gdbarch *,
+				  struct linespec_result *,
+				  struct linespec_sals *, char *,
+				  enum bptype, enum bpdisp, int, int,
+				  int, const struct breakpoint_ops *,
+				  int, int, int);
+
+  /* Given the address string (second parameter), this method decodes it
+     and provides the SAL locations related to it.  For ordinary breakpoints,
+     it calls `decode_line_full'.
+
+     This function is called inside `addr_string_to_sals'.  */
+  void (*decode_linespec) (struct breakpoint *, char **,
+			   struct symtabs_and_lines *);
 };
 
 /* Helper for breakpoint_ops->print_recreate implementations.  Prints

Re: [RFC] Make static tracepoint with markers more OO

[Pedro Alves]

On 01/16/2012 05:03 PM, Sergio Durigan Junior wrote:
> On Monday, January 16 2012, Tom Tromey wrote:

>> I have no further comments, but I would rather Pedro give the final OK
>> on this.
> 
> Ok.  Here is the final version of the patch, then.

Looks good to me.  Thanks.

-- 
Pedro Alves

Re: [RFC] Make static tracepoint with markers more OO

[Sergio Durigan Junior]

On Monday, January 16 2012, Pedro Alves wrote:

> On 01/16/2012 05:03 PM, Sergio Durigan Junior wrote:
>> On Monday, January 16 2012, Tom Tromey wrote:
>
>>> I have no further comments, but I would rather Pedro give the final OK
>>> on this.
>> 
>> Ok.  Here is the final version of the patch, then.
>
> Looks good to me.  Thanks.

Thanks.  Committed:
         http://sourceware.org/ml/gdb-cvs/2012-01/msg00140.html