Re: [PATCH 5/6] New MI command -trace-frame-collected

Pedro Alves
2013-06-25T17:15:34Z

On 06/18/2013 03:06 PM, Yao Qi wrote:

> +
> +This command returns the set of collected objects, register names,
> +trace state variable names, memory ranges and computed expressions
> +that have been collected at a particular trace frame. The optional
> +parameters to the command affect the output format in different ways.
> +See the output description table below for more details.
> +
> +The reported names can be used in the normal manner to create
> +varobjs and inspect the objects themselves. The items returned by
> +this command are categorized so that it is clear which is a variable,
> +which is a register, which is a trace state variable, which is a
> +memory range and which is a computed expression.
> +
> +For instance, if the actions were
> +@smallexample
> +collect myVar, myArray[myIndex], myObj.field, myPtr->field, myCount + 2
> +collect *(int*)0xaf02bef0@@40
> +@end smallexample
> +
> +@noindent
> +the object collected in its entirety would be @code{myVar}. Object
> +@code{myArray} is partially collected, because only element on index
> +@code{myIndex} is collected. The rest of objects are computed
> +expressions.

#1 "If the actions were ... then foo would be." sounds right.
#2 "If the actions are ... then foo is." would sound right.
#3 "If the actions were ... then foo is." sounds odd.

s/Object/The object/

"only the element at index"

"rest of the objects", or "the remaining objects"

That is, I suggest:

For instance, if the actions were
@smallexample
collect myVar, myArray[myIndex], myObj.field, myPtr->field, myCount + 2
collect *(int*)0xaf02bef0@@40
@end smallexample

@noindent
the object collected in its entirety would be @code{myVar}. The
object @code{myArray} would be partially collected, because only the
element at index @code{myIndex} would be collected. The remaining
objects would be computed expressions.

> diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
> index 430d530..0b8740da 100644
> --- a/gdb/mi/mi-main.c
> +++ b/gdb/mi/mi-main.c

> +
> +/* Implement the "-trace-frame-collected" command. */
> +
> +void
> +mi_cmd_trace_frame_collected (char *command, char **argv, int argc)
> +{

...

> + /* This command only makes sense for the current frame, not the
> + selected frame. */
> + old_chain = make_cleanup_restore_current_thread ();
> + select_frame (get_current_frame ());

ISTR a fix for tdump in the same vein as this. Is it in the queue?
Maybe it's already in...

> +
> + {
> + struct collection_list tracepoint_list, stepping_list;
> +
> + encode_actions (tloc, &tracepoint_list, &stepping_list);
> +
> + if (stepping_frame)
> + clist = &stepping_list;
> + else
> + clist = &tracepoint_list;
> + }

This use of scoping is wrong. As soon as the scope ends,
the life time of tracepoint_list and stepping_list's ends, so
uses of clist's pointee beyond this scope are undefined.

> diff --git a/gdb/remote.c b/gdb/remote.c
> index 080d048..d06b1ed 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -10524,7 +10524,7 @@ remote_download_tracepoint (struct bp_location *loc)
> struct breakpoint *b = loc->owner;
> struct tracepoint *t = (struct tracepoint *) b;
>
> - encode_actions (loc, &tdp_actions, &stepping_actions);
> + encode_actions_rsp (loc, &tdp_actions, &stepping_actions);
> old_chain = make_cleanup (free_actions_list_cleanup_wrapper,
> tdp_actions);
> (void) make_cleanup (free_actions_list_cleanup_wrapper,
> diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
> index d7d0e88..4f10c93 100644
> --- a/gdb/tracepoint.c
> +++ b/gdb/tracepoint.c
> @@ -340,6 +340,22 @@ find_trace_state_variable (const char *name)
> return NULL;
> }
>
> +/* Return the tsv if its number is NUMBER. Return NULL if not
> + found. */

"if its number is" sounds a little odd.

/* Return the tsv with number NUMBER. Return NULL if not
found. */

Or based on find_trace_state_variable's comment:

/* Look for a trace state variable with a given number. Return NULL
if not found. */

> +
> +struct trace_state_variable *
> +find_trace_state_variable_by_number (int number)
> +{

> -/* Render all actions into gdb protocol. */
> +/* Encode actions of tracepoint TLOC->owner and fill TRACEPOINT_LIST
> + and STEPPING_LIST. Return a cleanup pointer to clean up both
> + TRACEPOINT_LIST and STEPPING_LIST. */

I think it'd be good to rename this as:

encode_actions_and_make_cleanup

Functions that return a cleanup aren't that frequent, and from
reading the user code, one gets the impression a cleanup is missing,
if there's no such hint.

>
> -void
> -encode_actions (struct bp_location *tloc, char ***tdp_actions,
> - char ***stepping_actions)
> +struct cleanup *
> +encode_actions (struct bp_location *tloc,
> + struct collection_list *tracepoint_list,
> + struct collection_list *stepping_list)
> {

This looks good to me with those changes.

Thanks!

--
Pedro Alves