This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
[PATCH] Tracepoint action validation cleanup
- From: Stan Shebs <stan at codesourcery dot com>
- To: gdb-patches at sourceware dot org
- Date: Wed, 31 Mar 2010 16:02:41 -0700
- Subject: [PATCH] Tracepoint action validation cleanup
Nathan started it as a simple error message rationalization, I went a
little overboard from there. :-) The most notable part of the change is
that validate_actionline no longer returns a result. The original
theory was it would analyze tracepoint actions and return a thumbs
up/down, but in practice most kinds of problems result in a call to
error() , and so there was little use for a return value.
There is certainly more that could be done, but this is a well-defined
step in the right direction.
Stan
2010-03-31 Stan Shebs <stan@codesourcery.com>
Nathan Sidwell <nathan@codesourcery.com>
* tracepoint.h (enum actionline_type): Remove.
(validate_actionline): Change return to void.
* tracepoint.c (report_agent_reqs_errors): New function.
(validate_actionline): Call it, change return to void, report errors
more consistently.
(collect_symbol): Call report_agent_reqs_errors.
(encode_actions_1): Ditto.
(encode_actions): Don't expect a result from validate_actionline.
* gdb.trace/actions.exp: Tweak expected output.
* gdb.trace/while-stepping.exp: Tweak expected output.
Index: tracepoint.c
===================================================================
RCS file: /cvs/src/src/gdb/tracepoint.c,v
retrieving revision 1.165
diff -p -r1.165 tracepoint.c
*** tracepoint.c 31 Mar 2010 17:59:48 -0000 1.165
--- tracepoint.c 31 Mar 2010 22:47:59 -0000
*************** trace_actions_command (char *args, int f
*** 553,560 ****
/* else just return */
}
/* worker function */
! enum actionline_type
validate_actionline (char **line, struct breakpoint *t)
{
struct cmd_list_element *c;
--- 553,587 ----
/* else just return */
}
+ /* Report the results of checking the agent expression, as errors or
+ internal errors. */
+
+ static void
+ report_agent_reqs_errors (struct agent_expr *aexpr, struct agent_reqs *areqs)
+ {
+ /* All of the "flaws" are serious bytecode generation issues that
+ should never occur. */
+ if (areqs->flaw != agent_flaw_none)
+ internal_error (__FILE__, __LINE__, _("expression is malformed"));
+
+ /* If analysis shows a stack underflow, GDB must have done something
+ badly wrong in its bytecode generation. */
+ if (areqs->min_height < 0)
+ internal_error (__FILE__, __LINE__,
+ _("expression has min height < 0"));
+
+ /* Issue this error if the stack is predicted to get too deep. The
+ limit is rather arbitrary; a better scheme might be for the
+ target to report how much stack it will have available. The
+ depth roughly corresponds to parenthesization, so a limit of 20
+ amounts to 20 levels of expression nesting, which is actually
+ a pretty big hairy expression. */
+ if (areqs->max_height > 20)
+ error (_("Expression is too complicated."));
+ }
+
/* worker function */
! void
validate_actionline (char **line, struct breakpoint *t)
{
struct cmd_list_element *c;
*************** validate_actionline (char **line, struct
*** 562,595 ****
struct cleanup *old_chain = NULL;
char *p, *tmp_p;
struct bp_location *loc;
/* if EOF is typed, *line is NULL */
if (*line == NULL)
! return END;
for (p = *line; isspace ((int) *p);)
p++;
/* Symbol lookup etc. */
if (*p == '\0') /* empty line: just prompt for another line. */
! return BADLINE;
if (*p == '#') /* comment line */
! return GENERIC;
c = lookup_cmd (&p, cmdlist, "", -1, 1);
if (c == 0)
! {
! warning (_("'%s' is not an action that I know, or is ambiguous."),
! p);
! return BADLINE;
! }
if (cmd_cfunc_eq (c, collect_pseudocommand))
{
- struct agent_expr *aexpr;
- struct agent_reqs areqs;
-
do
{ /* repeat over a comma-separated list */
QUIT; /* allow user to bail out with ^C */
--- 589,617 ----
struct cleanup *old_chain = NULL;
char *p, *tmp_p;
struct bp_location *loc;
+ struct agent_expr *aexpr;
+ struct agent_reqs areqs;
/* if EOF is typed, *line is NULL */
if (*line == NULL)
! return;
for (p = *line; isspace ((int) *p);)
p++;
/* Symbol lookup etc. */
if (*p == '\0') /* empty line: just prompt for another line. */
! return;
if (*p == '#') /* comment line */
! return;
c = lookup_cmd (&p, cmdlist, "", -1, 1);
if (c == 0)
! error (_("`%s' is not a tracepoint action, or is ambiguous."), p);
if (cmd_cfunc_eq (c, collect_pseudocommand))
{
do
{ /* repeat over a comma-separated list */
QUIT; /* allow user to bail out with ^C */
*************** validate_actionline (char **line, struct
*** 618,633 ****
{
if (SYMBOL_CLASS (exp->elts[2].symbol) == LOC_CONST)
{
! warning (_("constant %s (value %ld) will not be collected."),
! SYMBOL_PRINT_NAME (exp->elts[2].symbol),
! SYMBOL_VALUE (exp->elts[2].symbol));
! return BADLINE;
}
else if (SYMBOL_CLASS (exp->elts[2].symbol) == LOC_OPTIMIZED_OUT)
{
! warning (_("%s is optimized away and cannot be collected."),
! SYMBOL_PRINT_NAME (exp->elts[2].symbol));
! return BADLINE;
}
}
--- 640,653 ----
{
if (SYMBOL_CLASS (exp->elts[2].symbol) == LOC_CONST)
{
! error (_("constant `%s' (value %ld) will not be collected."),
! SYMBOL_PRINT_NAME (exp->elts[2].symbol),
! SYMBOL_VALUE (exp->elts[2].symbol));
}
else if (SYMBOL_CLASS (exp->elts[2].symbol) == LOC_OPTIMIZED_OUT)
{
! error (_("`%s' is optimized away and cannot be collected."),
! SYMBOL_PRINT_NAME (exp->elts[2].symbol));
}
}
*************** validate_actionline (char **line, struct
*** 638,667 ****
make_cleanup_free_agent_expr (aexpr);
if (aexpr->len > MAX_AGENT_EXPR_LEN)
! error (_("expression too complicated, try simplifying"));
ax_reqs (aexpr, &areqs);
(void) make_cleanup (xfree, areqs.reg_mask);
! if (areqs.flaw != agent_flaw_none)
! error (_("malformed expression"));
!
! if (areqs.min_height < 0)
! error (_("gdb: Internal error: expression has min height < 0"));
!
! if (areqs.max_height > 20)
! error (_("expression too complicated, try simplifying"));
do_cleanups (old_chain);
}
}
while (p && *p++ == ',');
- return GENERIC;
}
else if (cmd_cfunc_eq (c, teval_pseudocommand))
{
- struct agent_expr *aexpr;
-
do
{ /* repeat over a comma-separated list */
QUIT; /* allow user to bail out with ^C */
--- 658,678 ----
make_cleanup_free_agent_expr (aexpr);
if (aexpr->len > MAX_AGENT_EXPR_LEN)
! error (_("Expression is too complicated."));
ax_reqs (aexpr, &areqs);
(void) make_cleanup (xfree, areqs.reg_mask);
! report_agent_reqs_errors (aexpr, &areqs);
do_cleanups (old_chain);
}
}
while (p && *p++ == ',');
}
+
else if (cmd_cfunc_eq (c, teval_pseudocommand))
{
do
{ /* repeat over a comma-separated list */
QUIT; /* allow user to bail out with ^C */
*************** validate_actionline (char **line, struct
*** 683,696 ****
make_cleanup_free_agent_expr (aexpr);
if (aexpr->len > MAX_AGENT_EXPR_LEN)
! error (_("expression too complicated, try simplifying"));
do_cleanups (old_chain);
}
}
while (p && *p++ == ',');
- return GENERIC;
}
else if (cmd_cfunc_eq (c, while_stepping_pseudocommand))
{
char *steparg; /* in case warning is necessary */
--- 694,712 ----
make_cleanup_free_agent_expr (aexpr);
if (aexpr->len > MAX_AGENT_EXPR_LEN)
! error (_("Expression is too complicated."));
!
! ax_reqs (aexpr, &areqs);
! (void) make_cleanup (xfree, areqs.reg_mask);
!
! report_agent_reqs_errors (aexpr, &areqs);
do_cleanups (old_chain);
}
}
while (p && *p++ == ',');
}
+
else if (cmd_cfunc_eq (c, while_stepping_pseudocommand))
{
char *steparg; /* in case warning is necessary */
*************** validate_actionline (char **line, struct
*** 699,717 ****
p++;
steparg = p;
! if (*p == '\0' ||
! (t->step_count = strtol (p, &p, 0)) == 0)
! {
! error (_("'%s': bad step-count."), *line);
! }
! return STEPPING;
}
else if (cmd_cfunc_eq (c, end_actions_pseudocommand))
! return END;
else
! {
! error (_("'%s' is not a supported tracepoint action."), *line);
! }
}
enum {
--- 715,729 ----
p++;
steparg = p;
! if (*p == '\0' || (t->step_count = strtol (p, &p, 0)) == 0)
! error (_("while-stepping step count `%s' is malformed."), *line);
}
+
else if (cmd_cfunc_eq (c, end_actions_pseudocommand))
! ;
!
else
! error (_("`%s' is not a supported tracepoint action."), *line);
}
enum {
*************** collect_symbol (struct collection_list *
*** 976,988 ****
old_chain1 = make_cleanup_free_agent_expr (aexpr);
ax_reqs (aexpr, &areqs);
! if (areqs.flaw != agent_flaw_none)
! error (_("malformed expression"));
!
! if (areqs.min_height < 0)
! error (_("gdb: Internal error: expression has min height < 0"));
! if (areqs.max_height > 20)
! error (_("expression too complicated, try simplifying"));
discard_cleanups (old_chain1);
add_aexpr (collect, aexpr);
--- 988,995 ----
old_chain1 = make_cleanup_free_agent_expr (aexpr);
ax_reqs (aexpr, &areqs);
!
! report_agent_reqs_errors (aexpr, &areqs);
discard_cleanups (old_chain1);
add_aexpr (collect, aexpr);
*************** encode_actions_1 (struct command_line *a
*** 1287,1299 ****
old_chain1 = make_cleanup_free_agent_expr (aexpr);
ax_reqs (aexpr, &areqs);
- if (areqs.flaw != agent_flaw_none)
- error (_("malformed expression"));
! if (areqs.min_height < 0)
! error (_("gdb: Internal error: expression has min height < 0"));
! if (areqs.max_height > 20)
! error (_("expression too complicated, try simplifying"));
discard_cleanups (old_chain1);
add_aexpr (collect, aexpr);
--- 1294,1301 ----
old_chain1 = make_cleanup_free_agent_expr (aexpr);
ax_reqs (aexpr, &areqs);
! report_agent_reqs_errors (aexpr, &areqs);
discard_cleanups (old_chain1);
add_aexpr (collect, aexpr);
*************** encode_actions_1 (struct command_line *a
*** 1347,1359 ****
old_chain1 = make_cleanup_free_agent_expr (aexpr);
ax_reqs (aexpr, &areqs);
- if (areqs.flaw != agent_flaw_none)
- error (_("malformed expression"));
! if (areqs.min_height < 0)
! error (_("gdb: Internal error: expression has min height < 0"));
! if (areqs.max_height > 20)
! error (_("expression too complicated, try simplifying"));
discard_cleanups (old_chain1);
/* Even though we're not officially collecting, add
--- 1349,1356 ----
old_chain1 = make_cleanup_free_agent_expr (aexpr);
ax_reqs (aexpr, &areqs);
! report_agent_reqs_errors (aexpr, &areqs);
discard_cleanups (old_chain1);
/* Even though we're not officially collecting, add
*************** encode_actions (struct breakpoint *t, st
*** 1414,1434 ****
if (*default_collect)
{
char *line;
- enum actionline_type linetype;
default_collect_line = xstrprintf ("collect %s", default_collect);
make_cleanup (xfree, default_collect_line);
line = default_collect_line;
! linetype = validate_actionline (&line, t);
! if (linetype != BADLINE)
! {
! default_collect_action = xmalloc (sizeof (struct command_line));
! make_cleanup (xfree, default_collect_action);
! default_collect_action->next = t->commands->commands;
! default_collect_action->line = line;
! actions = default_collect_action;
! }
}
encode_actions_1 (actions, t, tloc, frame_reg, frame_offset,
&tracepoint_list, &stepping_list);
--- 1411,1428 ----
if (*default_collect)
{
char *line;
default_collect_line = xstrprintf ("collect %s", default_collect);
make_cleanup (xfree, default_collect_line);
line = default_collect_line;
! validate_actionline (&line, t);
!
! default_collect_action = xmalloc (sizeof (struct command_line));
! make_cleanup (xfree, default_collect_action);
! default_collect_action->next = t->commands->commands;
! default_collect_action->line = line;
! actions = default_collect_action;
}
encode_actions_1 (actions, t, tloc, frame_reg, frame_offset,
&tracepoint_list, &stepping_list);
Index: tracepoint.h
===================================================================
RCS file: /cvs/src/src/gdb/tracepoint.h,v
retrieving revision 1.32
diff -p -r1.32 tracepoint.h
*** tracepoint.h 31 Mar 2010 17:59:48 -0000 1.32
--- tracepoint.h 31 Mar 2010 22:47:59 -0000
***************
*** 23,36 ****
#include "breakpoint.h"
#include "target.h"
- enum actionline_type
- {
- BADLINE = -1,
- GENERIC = 0,
- END = 1,
- STEPPING = 2
- };
-
/* A trace state variable is a value managed by a target being
traced. A trace state variable (or tsv for short) can be accessed
and assigned to by tracepoint actions and conditionals, but is not
--- 23,28 ----
*************** void set_traceframe_number (int);
*** 176,182 ****
struct cleanup *make_cleanup_restore_current_traceframe (void);
void free_actions (struct breakpoint *);
! enum actionline_type validate_actionline (char **, struct breakpoint *);
extern void end_actions_pseudocommand (char *args, int from_tty);
extern void while_stepping_pseudocommand (char *args, int from_tty);
--- 168,174 ----
struct cleanup *make_cleanup_restore_current_traceframe (void);
void free_actions (struct breakpoint *);
! extern void validate_actionline (char **, struct breakpoint *);
extern void end_actions_pseudocommand (char *args, int from_tty);
extern void while_stepping_pseudocommand (char *args, int from_tty);
Index: testsuite/gdb.trace/actions.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.trace/actions.exp,v
retrieving revision 1.16
diff -p -r1.16 actions.exp
*** testsuite/gdb.trace/actions.exp 23 Mar 2010 21:32:28 -0000 1.16
--- testsuite/gdb.trace/actions.exp 31 Mar 2010 22:47:59 -0000
*************** gdb_test "actions [expr $trcpt2 + $trcpt
*** 175,181 ****
gdb_trace_setactions "5.7: invalid action" \
"$trcpt1" \
"print gdb_c_test" \
! "'print gdb_c_test' is not a supported tracepoint action"
# 5.8 help actions (collect, while-stepping, end)
--- 175,181 ----
gdb_trace_setactions "5.7: invalid action" \
"$trcpt1" \
"print gdb_c_test" \
! "`print gdb_c_test' is not a supported tracepoint action"
# 5.8 help actions (collect, while-stepping, end)
Index: testsuite/gdb.trace/while-stepping.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.trace/while-stepping.exp,v
retrieving revision 1.15
diff -p -r1.15 while-stepping.exp
*** testsuite/gdb.trace/while-stepping.exp 23 Mar 2010 21:32:28 -0000 1.15
--- testsuite/gdb.trace/while-stepping.exp 31 Mar 2010 22:47:59 -0000
*************** proc while_stepping_bogus_arg { bogus ms
*** 82,88 ****
gdb_trace_setactions "$msgstring" \
"" \
! "while-stepping $bogus" ".*bad step-count"
}
# 5.14 while-stepping (no argument)
--- 82,88 ----
gdb_trace_setactions "$msgstring" \
"" \
! "while-stepping $bogus" ".*while-stepping step count"
}
# 5.14 while-stepping (no argument)