Re: [patch 1/8] Download tracepoint on location level

Pedro Alves
2011-11-10T14:54:34Z

On Wednesday 09 November 2011 03:34:01, Yao Qi wrote:

> --- a/gdb/target.c
> +++ b/gdb/target.c
> @@ -674,7 +674,7 @@ update_current_target (void)
> INHERIT (to_supports_enable_disable_tracepoint, t);
> INHERIT (to_supports_string_tracing, t);
> INHERIT (to_trace_init, t);
> - INHERIT (to_download_tracepoint, t);
> + INHERIT (to_download_tracepoint_loc, t);

Please drop the _loc from the function name. We don't have it
in the other methods like to_enable_tracepoint/to_disable_tracepoint
which take a location too, and others like to_insert_breakpoint
aren't called to_insert_breakpoint_gdbarch_target_info either.

> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -9835,9 +9835,9 @@ remote_download_command_source (int num, ULONGEST addr,
> }
>
> static void
> -remote_download_tracepoint (struct breakpoint *b)
> +remote_download_tracepoint_loc (struct bp_location *loc)
> {

Ditto.

> --- a/gdb/target.h
> +++ b/gdb/target.h
> @@ -686,8 +686,10 @@ struct target_ops
> /* Prepare the target for a tracing run. */
> void (*to_trace_init) (void);
>
> - /* Send full details of a tracepoint to the target. */
> - void (*to_download_tracepoint) (struct breakpoint *t);
> + /* Send full details of a tracepoint location to the target. Target may
> + install or not install tracepoint locations to inferior, which is
> + determined by target's factors, such tracing state. */

This comment doesn't belong in this patch. And I think it only
made sense on a previous version of the patch series.

> + void (*to_download_tracepoint_loc) (struct bp_location *location);
>
> /* Send full details of a trace state variable to the target. */
> void (*to_download_trace_state_variable) (struct trace_state_variable *tsv);
> @@ -1474,8 +1476,8 @@ extern int target_search_memory (CORE_ADDR start_addr,
> #define target_trace_init() \
> (*current_target.to_trace_init) ()
>
> -#define target_download_tracepoint(t) \
> - (*current_target.to_download_tracepoint) (t)
> +#define target_download_tracepoint_loc(t) \
> + (*current_target.to_download_tracepoint_loc) (t)

Drop _loc.

Okay with those changes.

--
Pedro Alves