[patch[ Improve the error messages of tvariable command

[patch[ Improve the error messages of tvariable command

[Hafiz Abid Qadeer]

[-- Attachment #1: Type: text/plain, Size: 1863 bytes --]

Hi All,
This patch improves the error message of tvariable command. The  
difference in error messages in various cases is shown below. This  
patch was originally written by Pedro. I have updated it to latest code  
and regtested. Is it ok?

Regards,
Abid

Before the patch:

(gdb) start
Temporary breakpoint 1 at 0x80483ba: file test.c, line 3.
Starting program: /home/abidh/test

Temporary breakpoint 1, main () at test.c:3
3	  int x = 1;

(gdb) list
1	int main()
2	{
3	  int x = 1;
4	  return 0;
5	}
(gdb) tvar x
Syntax must be $NAME [ = EXPR ]
(gdb) tvar y
No symbol "y" in current context.
(gdb) tvar $
Syntax must be $NAME [ = EXPR ]
(gdb) tvar $1234
Syntax must be $NAME [ = EXPR ]


After the patch:

(gdb) start
Temporary breakpoint 1 at 0x80483ba: file test.c, line 3.
Starting program: /home/abidh/test

Temporary breakpoint 1, main () at test.c:3
3	  int x = 1;

(gdb) list
1	int main()
2	{
3	  int x = 1;
4	  return 0;
5	}
(gdb) tvar x
Name of trace variable should start with '$'
(gdb) tvar y
Name of trace variable should start with '$'
(gdb) tvar $
Must supply a non-empty variable name
(gdb) tvar $1234
$1234 is not a valid trace state variable name
(gdb) tvar $asdf=1=1
Left operand of assignment is not an lvalue.



2013-02-12  Pedro Alves  <palves@redhat.com>
	    Hafiz Abid Qadeer  <abidh@codesourcery.com>

	gdb/
	* tracepoint.h (validate_trace_state_variable_name): Declare.
	* tracepoint.c (validate_trace_state_variable_name): New.
	(trace_variable_command): Parse the trace state variable's name
	without using parse_expression.  Do several validations.
	* mi/mi-main.c (mi_cmd_trace_define_variable): Don't parse the
	trace state variable's name with parse_expression.  Validate it.

	gdb/testsuite/
	* gdb.trace/tsv.exp: Adjust tests, and add a few more.


[-- Attachment #2: tvar.patch --]
[-- Type: text/x-patch, Size: 6694 bytes --]

diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index 28de126aa..37294e0 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -2357,7 +2357,6 @@ 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;
@@ -2365,19 +2364,11 @@ mi_cmd_trace_define_variable (char *command, char **argv, int argc)
   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);
-    }
+  name = argv[0];
+  if (*name++ != '$')
+    error (_("Name of trace variable should start with '$'"));
 
-  if (!name || *name == '\0')
-    error (_("Invalid name of trace variable"));
+  validate_trace_state_variable_name (name);
 
   tsv = find_trace_state_variable (name);
   if (!tsv)
@@ -2387,8 +2378,6 @@ mi_cmd_trace_define_variable (char *command, char **argv, int argc)
     initval = value_as_long (parse_and_eval (argv[1]));
 
   tsv->initial_value = initval;
-
-  do_cleanups (back_to);
 }
 
 void
diff --git a/gdb/testsuite/gdb.trace/tsv.exp b/gdb/testsuite/gdb.trace/tsv.exp
index 4ea930f..47d66ad 100644
--- a/gdb/testsuite/gdb.trace/tsv.exp
+++ b/gdb/testsuite/gdb.trace/tsv.exp
@@ -46,14 +46,30 @@ gdb_test "tvariable \$tvar3 = 1234567000000" \
   "Trace state variable \\\$tvar3 now has initial value 1234567000000." \
   "Init trace state variable to a 64-bit value"
 
+gdb_test "tvariable $" \
+  "Must supply a non-empty variable name" \
+  "tvariable syntax error, not empty variable name"
+
 gdb_test "tvariable main" \
-  "Syntax must be \\\$NAME \\\[ = EXPR \\\]" \
+  "Name of trace variable should start with '\\\$'" \
   "tvariable syntax error, bad name"
 
+gdb_test "tvariable \$\$" \
+  "Syntax must be \\\$NAME \\\[ = EXPR \\\]" \
+  "tvariable syntax error, bad name 2"
+
+gdb_test "tvariable \$123" \
+  "\\\$123 is not a valid trace state variable name" \
+  "tvariable syntax error, bad name 3"
+
 gdb_test "tvariable \$tvar1 - 93" \
   "Syntax must be \\\$NAME \\\[ = EXPR \\\]" \
   "tvariable syntax error, not an assignment"
 
+gdb_test "tvariable \$tvar0 = 1 = 1" \
+  "Left operand of assignment is not an lvalue\." \
+  "tvariable creation fails with invalid expression"
+
 gdb_test "info tvariables" \
     "Name\[\t \]+Initial\[\t \]+Current.*
 \\\$tvar1\[\t \]+0\[\t \]+<undefined>.*
diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index b45863e..540665d 100644
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -362,50 +362,67 @@ delete_trace_state_variable (const char *name)
   warning (_("No trace variable named \"$%s\", not deleting"), name);
 }
 
+/* Throws an error if NAME is not valid syntax for a trace state
+   variable's name.  */
+
+void
+validate_trace_state_variable_name (const char *name)
+{
+  const char *p;
+
+  if (*name == '\0')
+    error (_("Must supply a non-empty variable name"));
+
+  /* All digits in the name is reserved for value history
+     references.  */
+  for (p = name; isdigit (*p); p++)
+    ;
+  if (*p == '\0')
+    error (_("$%s is not a valid trace state variable name"), name);
+
+  for (p = name; isalnum (*p) || *p == '_'; p++)
+    ;
+  if (*p != '\0')
+    error (_("$%s is not a valid trace state variable name"), name);
+}
+
 /* The 'tvariable' command collects a name and optional expression to
    evaluate into an initial value.  */
 
 static void
 trace_variable_command (char *args, int from_tty)
 {
-  struct expression *expr;
   struct cleanup *old_chain;
-  struct internalvar *intvar = NULL;
   LONGEST initval = 0;
   struct trace_state_variable *tsv;
+  char *name, *p;
 
   if (!args || !*args)
-    error_no_arg (_("trace state variable name"));
+    error_no_arg (_("Syntax is $NAME [ = EXPR ]"));
 
-  /* All the possible valid arguments are expressions.  */
-  expr = parse_expression (args);
-  old_chain = make_cleanup (free_current_contents, &expr);
+  /* Only allow two syntaxes; "$name" and "$name=value".  */
+  p = skip_spaces (args);
 
-  if (expr->nelts == 0)
-    error (_("No expression?"));
+  if (*p++ != '$')
+    error (_("Name of trace variable should start with '$'"));
 
-  /* Only allow two syntaxes; "$name" and "$name=value".  */
-  if (expr->elts[0].opcode == OP_INTERNALVAR)
-    {
-      intvar = expr->elts[1].internalvar;
-    }
-  else if (expr->elts[0].opcode == BINOP_ASSIGN
-	   && expr->elts[1].opcode == OP_INTERNALVAR)
-    {
-      intvar = expr->elts[2].internalvar;
-      initval = value_as_long (evaluate_subexpression_type (expr, 4));
-    }
-  else
+  name = p;
+  while (isalnum (*p) || *p == '_')
+    p++;
+  name = savestring (name, p - name);
+  old_chain = make_cleanup (xfree, name);
+
+  p = skip_spaces (p);
+  if (*p != '=' && *p != '\0')
     error (_("Syntax must be $NAME [ = EXPR ]"));
 
-  if (!intvar)
-    error (_("No name given"));
+  validate_trace_state_variable_name (name);
 
-  if (strlen (internalvar_name (intvar)) <= 0)
-    error (_("Must supply a non-empty variable name"));
+  if (*p == '=')
+    initval = value_as_long (parse_and_eval (++p));
 
   /* If the variable already exists, just change its initial value.  */
-  tsv = find_trace_state_variable (internalvar_name (intvar));
+  tsv = find_trace_state_variable (name);
   if (tsv)
     {
       if (tsv->initial_value != initval)
@@ -421,7 +438,7 @@ trace_variable_command (char *args, int from_tty)
     }
 
   /* Create a new variable.  */
-  tsv = create_trace_state_variable (internalvar_name (intvar));
+  tsv = create_trace_state_variable (name);
   tsv->initial_value = initval;
 
   observer_notify_tsv_created (tsv);
diff --git a/gdb/tracepoint.h b/gdb/tracepoint.h
index 2e1d83a..61c7212 100644
--- a/gdb/tracepoint.h
+++ b/gdb/tracepoint.h
@@ -246,6 +246,7 @@ 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);
+extern void validate_trace_state_variable_name (const char *name);
 
 extern struct trace_state_variable *find_trace_state_variable (const char *name);
 extern struct trace_state_variable *create_trace_state_variable (const char *name);

Re: [patch[ Improve the error messages of tvariable command

[Pedro Alves]

I somewhat remember investigating this, and writing a longer
rationale for the change -- the gist of it being not
handling tsv name as expressions.   Can you dig it out from
the internal archives and repost including that info?  It'd
be good for posterity.

-- 
Pedro Alves

Re: [patch[ Improve the error messages of tvariable command

[Abid, Hafiz]

On 13/02/13 16:47:38, Pedro Alves wrote:
> I somewhat remember investigating this, and writing a longer
> rationale for the change -- the gist of it being not
> handling tsv name as expressions.   Can you dig it out from
> the internal archives and repost including that info?  It'd
> be good for posterity.
> 
> --
> Pedro Alves
> 
> 

Here is what I found. Is it what you were expecting?

--

Here's a fix for issue #8161, which is about the "tvariable" returning
errors on invalid and tsv names.  E.g.,

  Temporary breakpoint 1, main () at a.cc:2
  2           int x = 1;
  (gdb) tvar x
  Syntax must be $NAME [ = EXPR ]
  (gdb) tvar y
  No symbol "y" in current context.

The problem is the use of parse_expression to parse the tvariable's
name.  We could merely always catch errors thrown from within
parse_expression, and issue a "usage" error, but it wouldn't cover
all cases correctly.  E.g.,

  (gdb) tvar x = nonexistingvariable

is an invalid expression, not invalid syntax.

I also noticed that

  (gdb) tvar x = 1 = 1

would be incorrectly accepted.

Given that the comand is documented to only take an expression
on the right side of the '=', it seems that whatever is on
the left side of the '=' should not be treated as an expression, but
literaly as "$NAME".  That's what the patch does.

MI now shares the same name validations as the CLI variant.

--

Regards,
Abid

Re: [patch[ Improve the error messages of tvariable command

[Pedro Alves]

On 02/13/2013 04:58 PM, Abid, Hafiz wrote:
> On 13/02/13 16:47:38, Pedro Alves wrote:
>> I somewhat remember investigating this, and writing a longer
>> rationale for the change -- the gist of it being not
>> handling tsv name as expressions.   Can you dig it out from
>> the internal archives and repost including that info?  It'd
>> be good for posterity.
>>
>> -- 
>> Pedro Alves
>>
>>
> 
> Here is what I found. Is it what you were expecting?
> 
> -- 
> 
> Here's a fix for issue #8161, which is about the "tvariable" returning
> errors on invalid and tsv names.  E.g.,
> 
>  Temporary breakpoint 1, main () at a.cc:2
>  2           int x = 1;
>  (gdb) tvar x
>  Syntax must be $NAME [ = EXPR ]
>  (gdb) tvar y
>  No symbol "y" in current context.
> 
> The problem is the use of parse_expression to parse the tvariable's
> name.  We could merely always catch errors thrown from within
> parse_expression, and issue a "usage" error, but it wouldn't cover
> all cases correctly.  E.g.,
> 
>  (gdb) tvar x = nonexistingvariable
> 
> is an invalid expression, not invalid syntax.
> 
> I also noticed that
> 
>  (gdb) tvar x = 1 = 1
> 
> would be incorrectly accepted.
> 
> Given that the comand is documented to only take an expression
> on the right side of the '=', it seems that whatever is on
> the left side of the '=' should not be treated as an expression, but
> literaly as "$NAME".  That's what the patch does.

Ah, that's it.  Thanks for digging it out.

> MI now shares the same name validations as the CLI variant.

-- 
Pedro Alves

Re: [patch[ Improve the error messages of tvariable command

[Pedro Alves]

On 02/12/2013 04:59 PM, Hafiz Abid Qadeer wrote:
> Hi All,
> This patch improves the error message of tvariable command. The difference in error messages in various cases is shown below.
> This patch was originally written by Pedro.
> I have updated it to latest code and regtested.
> Is it ok?

OK, but ...

> 2013-02-12  Pedro Alves  <palves@redhat.com>

Write pedro@codesourcery.com.

>         Hafiz Abid Qadeer  <abidh@codesourcery.com>

-- 
Pedro Alves