[RFA] Real C++ operator validation

[Keith Seitz <keiths@redhat.com>]

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

Hi,

C++ operators have been sorely neglected in gdb's C++ code, especially 
the parser and decode_line, the two biggest interfaces to the user.

This patch is an attempt to mitigate the later by validating operator 
names. For example, one cannot break/list on a function named 
"operator_1", since gdb assumes that anything starting with "operator" 
is an operator.

Originally, I set out (in CVS HEAD) to fix an annoying bug concerning an 
error message when the user attempted to call decode_line with an 
undefined operator. Turns out the fix for this is a little more complex 
than I was hoping, but since I am trying to get all my branch patches 
accepted/committed here, I thought, "What the heck?" :-)

As a result, this patch from my archer-keiths-expr-cumulative branch has 
several (good) side-effects:

1) It properly ignores spaces in operator names. "break operator ++" is 
now valid. [NOTE: No single quotes!]
2) It adds the ability to list/break on conversion operators, e.g., 
"break foo::operator char*". [NOTE: No single quotes!]
3) (The bug I was attempting to fix) It now outputs "the class foo does 
not have any method named operator int*" instead of "the class foo does 
not have any method named operator ator".
4) It removes the assumption that functions starting with the word 
"operator" are really operators. If they are not, it treats them as 
"normal" functions.

I have regression tested this on x86 linux, and there are no 
regressions. [I thought I would explicitly mention this. I always do 
this anyway.]

Ok?/Comments?/Concerns?

Keith

PS. Shortly I'm planning to submit my "realcpp" tests which offers 
substantially more thorough testing of this and a lot more. Consequently 
I've intentionally kept the testing in this patch restricted to the bug 
I originally set out to fix.

ChangeLog
2009-09-18  Keith Seitz  <keiths@redhat.com>

	* cp-support.h (cp_validate_operator): Declare new function.
	* cp-support.c (cp_validate_operator): New function.
	* linespec.c (decode_compound): For C++ check for a valid operator.

testsuite/ChangeLog

2009-09-18  Keith Seitz  <keiths@redhat.com>

	* gdb.cp/cplusfuncs.exp (do_tests): Add check for proper error message
	with invalid operator.


[-- Attachment #2: cp_validate_operator.patch --]
[-- Type: text/plain, Size: 7563 bytes --]

Index: cp-support.h
===================================================================
RCS file: /cvs/src/src/gdb/cp-support.h,v
retrieving revision 1.29
diff -u -p -r1.29 cp-support.h
--- cp-support.h	7 Jul 2009 17:25:11 -0000	1.29
+++ cp-support.h	18 Sep 2009 22:39:55 -0000
@@ -72,6 +72,8 @@ extern struct symbol **make_symbol_overl
 extern struct type *cp_lookup_rtti_type (const char *name,
 					 struct block *block);
 
+extern int cp_validate_operator (const char *input);
+
 /* Functions/variables from cp-namespace.c.  */
 
 extern int cp_is_anonymous (const char *namespace);
Index: cp-support.c
===================================================================
RCS file: /cvs/src/src/gdb/cp-support.c,v
retrieving revision 1.33
diff -u -p -r1.33 cp-support.c
--- cp-support.c	7 Jul 2009 17:25:11 -0000	1.33
+++ cp-support.c	18 Sep 2009 22:39:56 -0000
@@ -32,6 +32,9 @@
 #include "block.h"
 #include "complaints.h"
 #include "gdbtypes.h"
+#include "exceptions.h"
+#include "expression.h"
+#include "value.h"
 
 #include "safe-ctype.h"
 
@@ -70,6 +73,18 @@ struct cmd_list_element *maint_cplus_cmd
 static void maint_cplus_command (char *arg, int from_tty);
 static void first_component_command (char *arg, int from_tty);
 
+/* Operator validation.
+   NOTE: Multi-byte operators (usually the assignment variety operator)
+   must appear before the single byte version, i.e., "+=" before "+".  */
+static const char *operator_tokens[] =
+  {
+    "++", "+=", "+", "->*", "->", "--", "-=", "-", "*=", "*", "/=", "/",
+    "%=", "%", "!=", "==", "!", "&&", "<<=", "<<", ">>=", ">>",
+    "<=", "<", ">=", ">", "~", "&=", "&", "|=", "||", "|", "^=", "^",
+    "=", "()", "[]", ",", "new", "delete"
+    /* new[] and delete[] require special whitespace handling */
+  };
+
 /* Return 1 if STRING is clearly already in canonical form.  This
    function is conservative; things which it does not recognize are
    assumed to be non-canonical, and the parser will sort them out
@@ -909,6 +924,109 @@ first_component_command (char *arg, int 
 
 extern initialize_file_ftype _initialize_cp_support; /* -Wmissing-prototypes */
 
+#define SKIP_SPACE(P)				\
+  do						\
+  {						\
+    while (*(P) == ' ' || *(P) == '\t')		\
+      ++(P);					\
+  }						\
+  while (0)
+
+/* Returns the length of the operator name or 0 if INPUT does not
+   point to a valid C++ operator.  INPUT should start with "operator".  */
+int
+cp_validate_operator (const char *input)
+{
+  int i;
+  char *copy;
+  const char *p;
+  struct expression *expr;
+  struct value *val;
+  struct gdb_exception except;
+  struct cleanup *old_chain;
+
+  p = input;
+
+  /* Most callers appear to do this already, but since this is not
+     really time-critical code, it is double-checked here.  */
+  if (strncmp (p, "operator", 8) == 0)
+    {
+      int valid = 0;
+      p += 8;
+
+      SKIP_SPACE (p);
+      for (i = 0; i < sizeof (operator_tokens) / sizeof (operator_tokens[0]);
+	   ++i)
+	{
+	  int length = strlen (operator_tokens[i]);
+	  /* By using strncmp here, we MUST have operator_tokens ordered!
+	     See additional notes where operator_tokens is defined above.  */
+	  if (strncmp (p, operator_tokens[i], length) == 0)
+	    {
+	      const char *op = p;
+	      valid = 1;
+	      p += length;
+
+	      if (strncmp (op, "new", 3) == 0
+		  || strncmp (op, "delete", 6) == 0)
+		{
+
+		  /* Special case: new[] and delete[].  We must be careful
+		     to swallow whitespace before/in "[]".  */
+		  SKIP_SPACE (p);
+
+		  if (*p == '[')
+		    {
+		      ++p;
+		      SKIP_SPACE (p);
+		      if (*p == ']')
+			++p;
+		      else
+			valid = 0;
+		    }
+		}
+
+	      if (valid)
+		return (p - input);
+	    }
+	}
+
+      /* Check input for a conversion operator.  */
+
+      /* Skip past base typename */
+      while (*p != '*' && *p != '&' && *p != 0 && *p != ' ')
+	++p;
+      SKIP_SPACE (p);
+
+      /* Add modifiers '*'/'&' */
+      while (*p == '*' || *p == '&')
+	{
+	  ++p;
+	  SKIP_SPACE (p);
+	}
+
+      /* Check for valid type.  [Remember: input starts with 
+	 "operator".]  */
+      copy = savestring (input + 8, p - input - 8);
+      expr = NULL;
+      val = NULL;
+      TRY_CATCH (except, RETURN_MASK_ALL)
+	{
+	  expr = parse_expression (copy);
+	  val = evaluate_type (expr);
+	}
+
+      xfree (copy);
+      if (expr)
+	xfree (expr);
+
+      if (val != NULL && value_type (val) != NULL)
+	return (p - input);
+    }
+
+  return 0;
+}
+
 void
 _initialize_cp_support (void)
 {
Index: linespec.c
===================================================================
RCS file: /cvs/src/src/gdb/linespec.c,v
retrieving revision 1.89
diff -u -p -r1.89 linespec.c
--- linespec.c	14 Aug 2009 00:32:32 -0000	1.89
+++ linespec.c	18 Sep 2009 22:39:56 -0000
@@ -30,6 +30,7 @@
 #include "value.h"
 #include "completer.h"
 #include "cp-abi.h"
+#include "cp-support.h"
 #include "parser-defs.h"
 #include "block.h"
 #include "objc-lang.h"
@@ -1257,6 +1258,9 @@ decode_compound (char **argptr, int funf
       /* Move pointer ahead to next double-colon.  */
       while (*p && (p[0] != ' ') && (p[0] != '\t') && (p[0] != '\''))
 	{
+	  if (current_language->la_language == language_cplus)
+	    p += cp_validate_operator (p);
+
 	  if (p[0] == '<')
 	    {
 	      temp_end = find_template_name_end (p);
@@ -1334,6 +1338,15 @@ decode_compound (char **argptr, int funf
 	  while (*p && *p != ' ' && *p != '\t' && *p != ',' && *p != ':')
 	    p++;
 	  /* At this point p->"".  String ended.  */
+	  /* Nope, C++ operators could have spaces in them
+	     ("foo::operator <" or "foo::operator delete []").
+	     I apologize, this is a bit hacky...  */
+	  if (current_language->la_language == language_cplus
+	      && *p == ' ' && p - 8 - *argptr + 1 > 0)
+	    {
+	      /* The above loop has already swallowed "operator".  */
+	      p += cp_validate_operator (p - 8) - 8;
+	    }
 	}
 
       /* Allocate our own copy of the substring between argptr and
@@ -1474,26 +1487,16 @@ find_method (int funfirstline, char ***c
     }
   else
     {
-      char *tmp;
-
-      if (is_operator_name (copy))
-	{
-	  tmp = (char *) alloca (strlen (copy + 3) + 9);
-	  strcpy (tmp, "operator ");
-	  strcat (tmp, copy + 3);
-	}
-      else
-	tmp = copy;
       if (not_found_ptr)
         *not_found_ptr = 1;
-      if (tmp[0] == '~')
+      if (copy[0] == '~')
 	cplusplus_error (saved_arg,
 			 "the class `%s' does not have destructor defined\n",
 			 SYMBOL_PRINT_NAME (sym_class));
       else
 	cplusplus_error (saved_arg,
 			 "the class %s does not have any method named %s\n",
-			 SYMBOL_PRINT_NAME (sym_class), tmp);
+			 SYMBOL_PRINT_NAME (sym_class), copy);
     }
 }
 
Index: testsuite/gdb.cp/cplusfuncs.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.cp/cplusfuncs.exp,v
retrieving revision 1.7
diff -u -p -r1.7 cplusfuncs.exp
--- testsuite/gdb.cp/cplusfuncs.exp	3 Jan 2009 05:58:04 -0000	1.7
+++ testsuite/gdb.cp/cplusfuncs.exp	18 Sep 2009 22:39:56 -0000
@@ -534,6 +534,7 @@ proc do_tests {} {
     global srcdir
     global binfile
     global gdb_prompt
+    global dm_type_int_star
 
     set prms_id 0
     set bug_id 0
@@ -557,6 +558,10 @@ proc do_tests {} {
     test_paddr_operator_functions
     test_paddr_hairy_functions
     test_lookup_operator_functions
+
+    # A regression test on errors involving operators
+    gdb_test "list foo::operator $dm_type_int_star" \
+	".*the class foo does not have any method named operator $dm_type_int_star.*"
 }
 
 do_tests