This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[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)

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]