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

[Pedro Alves <palves at redhat dot com>]

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