Re: [MI tracepoints 6/9] trace variable commands

Vladimir Prus
2010-03-16T13:17:39Z

[-- Attachment #1: Type: Text/Plain, Size: 1747 bytes --]

On Monday 15 March 2010 21:00:38 Pedro Alves wrote:

> On Sunday 14 March 2010 08:58:36, Vladimir Prus wrote:
> > + name = argv[0];
> > + if (name[0] != '$')
> > + error (_("Name of trace variable should start with '$'"));
> > + ++name;
> > +
> > + expr = parse_expression (argv[0]);
> > + back_to = make_cleanup (xfree, expr);
> > +
> > + if (expr->nelts == 3 && expr->elts[0].opcode == OP_INTERNALVAR)
> > + {
> > + struct internalvar *intvar = expr->elts[1].internalvar;
> > + if (intvar)
> > + name = internalvar_name (intvar);
> > + }
> > +
> > + if (!name || *name == '\0')
> > + error (_("Invalid name of trace variable"));
>
> Waitaminute. Is there a merge error here?
>
> Repeating the snippet:
>
> > + name = argv[0];
> > + if (name[0] != '$')
> > + error (_("Name of trace variable should start with '$'"));
> > + ++name;
> > +
>
> I think this whole bit above shouldn't be here.

Yes, it's a merge error.

> > + expr = parse_expression (argv[0]);
> > + back_to = make_cleanup (xfree, expr);
> > +
> > + if (expr->nelts == 3 && expr->elts[0].opcode == OP_INTERNALVAR)
> > + {
> > + struct internalvar *intvar = expr->elts[1].internalvar;
> > + if (intvar)
> > + name = internalvar_name (intvar); <<<<<<<< (1)
> > + }
> > +
> > + if (!name || *name == '\0')
> > + error (_("Invalid name of trace variable"));
>
> Otherwise, it looks like you can get here with an invalid name, if
> the expression did parse sucessfully, but (1) wasn't reached at all.

At least after removing the unnecessary bit, if (!) is not executed,
name will be NULL -- which seems right behaviour.

Thanks,

--
Vladimir Prus
CodeSourcery
vladimir@codesourcery.com
(650) 331-3385 x722

[-- Attachment #2: 6-trace-variables-2.diff --]
[-- Type: text/x-patch, Size: 7793 bytes --]

commit 1b7506f06e31d6ae934fc50e73eb1d8bde3ccd28
Author: vladimir <vladimir@e7755896-6108-0410-9592-8049d3e74e28>
Date: Thu Aug 6 07:50:47 2009 +0000

-trace-define-variable and -trace-list-variables.

* tracepoint.c (create_trace_state_variable): Make
private copy of name, as opposed to assuming the
pointer lives forever.
(tvariables_info_1): New.
(tvariables_info): Use the above.
* tracepoint.h (create_trace_state_variable, tvariables_info_1):
Declare.
* mi/mi-cmds.c (mi_cmds): Register -trace-define-variable
and -trace-list-variables.
* mi/mi-cmds.h (mi_cmd_trace_define_variable)
(mi_cmd_trace_list_variables): New.
* mi/mi-main.c (mi_cmd_trace_define_variable)
(mi_cmd_trace_list_variables): New.

diff --git a/gdb/mi/mi-cmds.c b/gdb/mi/mi-cmds.c
index 6b260fc..a07ee3b 100644
--- a/gdb/mi/mi-cmds.c
+++ b/gdb/mi/mi-cmds.c
@@ -106,6 +106,8 @@ struct mi_cmd mi_cmds[] =
{ "thread-info", { NULL, 0 }, mi_cmd_thread_info },
{ "thread-list-ids", { NULL, 0 }, mi_cmd_thread_list_ids},
{ "thread-select", { NULL, 0 }, mi_cmd_thread_select},
+ { "trace-define-variable", { NULL, 0 }, mi_cmd_trace_define_variable },
+ { "trace-list-variables", { NULL, 0 }, mi_cmd_trace_list_variables },
{ "trace-start", { NULL, 0 }, mi_cmd_trace_start },
{ "trace-status", { NULL, 0 }, mi_cmd_trace_status },
{ "trace-stop", { NULL, 0 }, mi_cmd_trace_stop },
diff --git a/gdb/mi/mi-cmds.h b/gdb/mi/mi-cmds.h
index b5ff61f..dc2b2c6 100644
--- a/gdb/mi/mi-cmds.h
+++ b/gdb/mi/mi-cmds.h
@@ -89,6 +89,8 @@ extern mi_cmd_argv_ftype mi_cmd_target_file_delete;
extern mi_cmd_argv_ftype mi_cmd_thread_info;
extern mi_cmd_argv_ftype mi_cmd_thread_list_ids;
extern mi_cmd_argv_ftype mi_cmd_thread_select;
+extern mi_cmd_argv_ftype mi_cmd_trace_define_variable;
+extern mi_cmd_argv_ftype mi_cmd_trace_list_variables;
extern mi_cmd_argv_ftype mi_cmd_trace_start;
extern mi_cmd_argv_ftype mi_cmd_trace_status;
extern mi_cmd_argv_ftype mi_cmd_trace_stop;
diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index 5aebc50..1f1c014 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -2082,6 +2082,52 @@ print_diff (struct mi_timestamp *start, struct mi_timestamp *end)
}

void
+mi_cmd_trace_define_variable (char *command, char **argv, int argc)
+{
+ struct expression *expr;
+ struct cleanup *back_to;
+ LONGEST initval = 0;
+ struct trace_state_variable *tsv;
+ char *name = 0;
+
+ if (argc != 1 && argc != 2)
+ error (_("Usage: -trace-define-variable VARIABLE [VALUE]"));
+
+ expr = parse_expression (argv[0]);
+ back_to = make_cleanup (xfree, expr);
+
+ if (expr->nelts == 3 && expr->elts[0].opcode == OP_INTERNALVAR)
+ {
+ struct internalvar *intvar = expr->elts[1].internalvar;
+ if (intvar)
+ name = internalvar_name (intvar);
+ }
+
+ if (!name || *name == '\0')
+ error (_("Invalid name of trace variable"));
+
+ tsv = find_trace_state_variable (name);
+ if (!tsv)
+ tsv = create_trace_state_variable (name);
+
+ if (argc == 2)
+ initval = value_as_long (parse_and_eval (argv[1]));
+
+ tsv->initial_value = initval;
+
+ do_cleanups (back_to);
+}
+
+void
+mi_cmd_trace_list_variables (char *command, char **argv, int argc)
+{
+ if (argc != 0)
+ error (_("-trace-list-variables: no arguments are allowed"));
+
+ tvariables_info_1 ();
+}
+
+void
mi_cmd_trace_start (char *command, char **argv, int argc)
{
start_tracing ();
diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index b7fae78..8e55d82 100644
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -271,7 +271,7 @@ create_trace_state_variable (const char *name)
struct trace_state_variable tsv;

memset (&tsv, 0, sizeof (tsv));
- tsv.name = name;
+ tsv.name = xstrdup (name);
tsv.number = next_tsv_number++;
return VEC_safe_push (tsv_s, tvariables, &tsv);
}
@@ -300,6 +300,7 @@ delete_trace_state_variable (const char *name)
for (ix = 0; VEC_iterate (tsv_s, tvariables, ix, tsv); ++ix)
if (strcmp (name, tsv->name) == 0)
{
+ xfree ((void *)tsv->name);
VEC_unordered_remove (tsv_s, tvariables, ix);
return;
}
@@ -401,17 +402,15 @@ delete_trace_variable_command (char *args, int from_tty)
dont_repeat ();
}

-/* List all the trace state variables. */
-
-static void
-tvariables_info (char *args, int from_tty)
+void
+tvariables_info_1 (void)
{
struct trace_state_variable *tsv;
int ix;
- char *reply;
- ULONGEST tval;
+ int count = 0;
+ struct cleanup *back_to;

- if (VEC_length (tsv_s, tvariables) == 0)
+ if (VEC_length (tsv_s, tvariables) == 0 && !ui_out_is_mi_like_p (uiout))
{
printf_filtered (_("No trace state variables.\n"));
return;
@@ -422,24 +421,56 @@ tvariables_info (char *args, int from_tty)
tsv->value_known = target_get_trace_state_variable_value (tsv->number,
&(tsv->value));

- printf_filtered (_("Name\t\t Initial\tCurrent\n"));
+ back_to = make_cleanup_ui_out_table_begin_end (uiout, 3,
+ count, "trace-variables");
+ ui_out_table_header (uiout, 15, ui_left, "name", "Name");
+ ui_out_table_header (uiout, 11, ui_left, "initial", "Initial");
+ ui_out_table_header (uiout, 11, ui_left, "current", "Current");
+
+ ui_out_table_body (uiout);

for (ix = 0; VEC_iterate (tsv_s, tvariables, ix, tsv); ++ix)
{
- printf_filtered ("$%s", tsv->name);
- print_spaces_filtered (17 - strlen (tsv->name), gdb_stdout);
- printf_filtered ("%s ", plongest (tsv->initial_value));
- print_spaces_filtered (11 - strlen (plongest (tsv->initial_value)), gdb_stdout);
+ struct cleanup *back_to2;
+ char *c;
+ char *name;
+
+ back_to2 = make_cleanup_ui_out_tuple_begin_end (uiout, "variable");
+
+ name = concat ("$", tsv->name, NULL);
+ make_cleanup (xfree, name);
+ ui_out_field_string (uiout, "name", name);
+ ui_out_field_string (uiout, "initial", plongest (tsv->initial_value));
+
if (tsv->value_known)
- printf_filtered (" %s", plongest (tsv->value));
+ c = plongest (tsv->value);
+ else if (ui_out_is_mi_like_p (uiout))
+ /* For MI, we prefer not to use magic string constants, but rather
+ omit the field completely. The difference between unknown and
+ undefined does not seem important enough to represent. */
+ c = NULL;
else if (current_trace_status ()->running || traceframe_number >= 0)
/* The value is/was defined, but we don't have it. */
- printf_filtered (_(" <unknown>"));
+ c = "<unknown>";
else
/* It is not meaningful to ask about the value. */
- printf_filtered (_(" <undefined>"));
- printf_filtered ("\n");
+ c = "<undefined>";
+ if (c)
+ ui_out_field_string (uiout, "current", c);
+ ui_out_text (uiout, "\n");
+
+ do_cleanups (back_to2);
}
+
+ do_cleanups (back_to);
+}
+
+/* List all the trace state variables. */
+
+static void
+tvariables_info (char *args, int from_tty)
+{
+ tvariables_info_1 ();
}

/* ACTIONS functions: */
diff --git a/gdb/tracepoint.h b/gdb/tracepoint.h
index 2e9193c..d03f09a 100644
--- a/gdb/tracepoint.h
+++ b/gdb/tracepoint.h
@@ -145,6 +145,7 @@ extern void end_actions_pseudocommand (char *args, int from_tty);
extern void while_stepping_pseudocommand (char *args, int from_tty);

extern struct trace_state_variable *find_trace_state_variable (const char *name);
+extern struct trace_state_variable *create_trace_state_variable (const char *name);

extern void parse_trace_status (char *line, struct trace_status *ts);

@@ -162,4 +163,6 @@ extern void stop_tracing (void);

extern void trace_status_mi (int on_stop);

+extern void tvariables_info_1 (void);
+
#endif /* TRACEPOINT_H */