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

[Pedro Alves <palves@redhat.com>]

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