Re: [RFA] Add method overload resolution to expression parser

[Keith Seitz <keiths@redhat.com>]

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

On 10/13/2009 02:24 PM, Tom Tromey wrote:
> First, since this is an extension to C++, I wonder whether it will
> introduce any parsing ambiguities once the parser is complete.  I
> suspect not, but I thought you might have a better view.

If it does, I haven't found them yet. Unfortunately, for a vast majority 
of cases, I must largely rely on the test suite to highlight some 
errors. In this specific case, though, I don't believe the new rules are 
reachable except within a C++ context since they rely on OP_SCOPE. [If 
you try to use this with a C function, the code path is quite different. 
It cannot get to TYPE_INSTANCE.]

> Keith>  +exp     :       exp '(' nonempty_typelist ')' const_or_volatile
>
> This is an interesting production.

Unfortunately, it is the only one that works. There are other rules that 
do a similar, sometimes non-sensical thing. The functional call rule is 
one example, e.g., "p (1+3)(5)" will attempt to call a function.

> I would have expected it to explicitly look for maybe-qualified
> identifiers -- not an arbitrary expression.  Does this let us do
> something we could not otherwise do?  Or, what does this do:
>
>    print (return_a_function ()) (int)

This will not currently do anything -- the "(int)" part is ignored 
during evaluation. That's because all that the "(int)" part does is add 
TYPE_INSTANCE to the expression chain, which causes an expected type to 
be sent down to evaluate_subexp_standard. evaluate_subexp_standard 
largely ignores expect_type, so this acts as a nop.

> Does it work to call an explicitly-specified overload?
>
>    print overloaded(int)(5)
>
> (I assume from reading the patch that this works as expected.)

I never tried that, but it does work (for C++ methods -- again C 
functions do not work at all).

> Keith>   func_mod:	'(' ')'
> Keith>   			{ $$ = 0; }
> Keith>   	|	'(' nonempty_typelist ')'
> Keith>  -			{ free ($2); $$ = 0; }
> Keith>  +			{ do_cleanups (typelist_cleanup); $$ = 0; }
>
> I'm a bit surprised that the cleanup stuff works in the parser.
> Interesting.

Daniel also mentions this in a later message. I'm not sure why I did it 
this way, but the code has changed (or it was a mistake) and it is 
certainly not needed. I've removed this and free the {re,m}alloc'd data 
in the TYPE_INSTANCE-generating rule.

> Keith>  +static void
> Keith>  +free_param_types (void *arg)
>
> Needs a short intro comment.

This function has been removed.

> Keith>  +static struct type *
> Keith>  +make_params (int num_types, struct type **param_types)
> [...]
> Keith>  +  make_cleanup (free_param_types, type);
>
> It is a little unusual to make a cleanup that isn't returned.  Is it
> really safe in this case?  To know that, I think you'd have to examine
> all callers of evaluate_subexp_standard.  It seems somewhat safer to do
> explicit cleanups in the TYPE_INSTANCE case, what do you think?

This cleanup has also now been removed.

I've attached an updated patch which I think addresses most of the 
serious concerns.

 From one of your follow-up messages, re: doing this in the parser:

The reason this is done during evaluation is because that is where the 
actual lookup of symbol is performed (unlike in C, where it is done in 
c_lex).

Keith

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

	* c-exp.y: Add new rule for resolving method overloads.
	* eval.c (make_params): New function.
	(evaluate_subexp_standard): Pass expect_type to value_aggregate_elt.
	Handle case TYPE_INSTANCE.
	(evaluate_subexp_for_address): Pass expect_type to value_aggregate_elt.
	* expression.h (enum exp_opcode): Add TYPE_INSTANCE.
	(compare_parameters): Add declaration.
	* parse.c (operator_length_standard): Add TYPE_INSTANCE.
	* valops.c (value_aggregate_elt): Add new expect_type parameter.
	Pass expect_type to value_struct_elt_for_reference.
	(value_struct_elt_for_reference): Add expect_type parameter and use
	compare_parameters.
	Check for overload matches with and without artificial parameters.
	Skip artificial methods.
	(compare_parameters): New function.
	* value.h (value_aggregate_elt): Add new expect_type parameter.

testsuite/ChangeLog
2009-11-09  Keith Seitz  <keiths@redhat.com>

	* gdb.cp/overload.exp: Add tests for resolving overloaded
	methods in expression parsing/evaluation.

[-- Attachment #2: ovld-expr-2.patch --]
[-- Type: text/plain, Size: 11383 bytes --]

Index: c-exp.y
===================================================================
RCS file: /cvs/src/src/gdb/c-exp.y,v
retrieving revision 1.63
diff -u -p -r1.63 c-exp.y
--- c-exp.y	6 Oct 2009 22:47:19 -0000	1.63
+++ c-exp.y	9 Nov 2009 21:48:48 -0000
@@ -401,6 +401,18 @@ arglist	:	arglist ',' exp   %prec ABOVE_
 			{ arglist_len++; }
 	;
 
+exp     :       exp '(' nonempty_typelist ')' const_or_volatile
+			{ int i;
+			  write_exp_elt_opcode (TYPE_INSTANCE);
+			  write_exp_elt_longcst ((LONGEST) $<ivec>3[0]);
+			  for (i = 0; i < $<ivec>3[0]; ++i)
+			    write_exp_elt_type ($<tvec>3[i + 1]);
+			  write_exp_elt_longcst((LONGEST) $<ivec>3[0]);
+			  write_exp_elt_opcode (TYPE_INSTANCE);
+			  free ($3);
+			}
+	;
+
 rcurly	:	'}'
 			{ $$ = end_arglist () - 1; }
 	;
Index: eval.c
===================================================================
RCS file: /cvs/src/src/gdb/eval.c,v
retrieving revision 1.122
diff -u -p -r1.122 eval.c
--- eval.c	6 Oct 2009 23:27:05 -0000	1.122
+++ eval.c	9 Nov 2009 21:48:48 -0000
@@ -40,6 +40,8 @@
 #include "regcache.h"
 #include "user-regs.h"
 #include "valprint.h"
+#include "gdb_obstack.h"
+#include "objfiles.h"
 #include "python/python.h"
 
 #include "gdb_assert.h"
@@ -651,6 +653,29 @@ ptrmath_type_p (struct type *type)
     }
 }
 
+/* Constructs a fake method with the given parameter types.
+   This function is used by the parser to construct an "expected"
+   type for method overload resolution.  */
+
+static struct type *
+make_params (int num_types, struct type **param_types)
+{
+  struct type *type = XZALLOC (struct type);
+  TYPE_MAIN_TYPE (type) = XZALLOC (struct main_type);
+  TYPE_LENGTH (type) = 1;
+  TYPE_CODE (type) = TYPE_CODE_METHOD;
+  TYPE_VPTR_FIELDNO (type) = -1;
+  TYPE_CHAIN (type) = type;
+  TYPE_NFIELDS (type) = num_types;
+  TYPE_FIELDS (type) = (struct field *)
+    TYPE_ZALLOC (type, sizeof (struct field) * num_types);
+
+  while (num_types-- > 0)
+    TYPE_FIELD_TYPE (type, num_types) = param_types[num_types];
+
+  return type;
+}
+
 struct value *
 evaluate_subexp_standard (struct type *expect_type,
 			  struct expression *exp, int *pos,
@@ -684,7 +709,7 @@ evaluate_subexp_standard (struct type *e
 	goto nosideret;
       arg1 = value_aggregate_elt (exp->elts[pc + 1].type,
 				  &exp->elts[pc + 3].string,
-				  0, noside);
+				  expect_type, 0, noside);
       if (arg1 == NULL)
 	error (_("There is no field named %s"), &exp->elts[pc + 3].string);
       return arg1;
@@ -1730,6 +1755,20 @@ evaluate_subexp_standard (struct type *e
 	  error (_("non-pointer-to-member value used in pointer-to-member construct"));
 	}
 
+    case TYPE_INSTANCE:
+      nargs = longest_to_int (exp->elts[pc + 1].longconst);
+      arg_types = (struct type **) alloca (nargs * sizeof (struct type *));
+      for (ix = 0; ix < nargs; ++ix)
+	arg_types[ix] = exp->elts[pc + 1 + ix + 1].type;
+
+      expect_type = make_params (nargs, arg_types);
+      *(pos) += 3 + nargs;
+      arg1 = evaluate_subexp_standard (expect_type, exp, pos, noside);
+      xfree (TYPE_FIELDS (expect_type));
+      xfree (TYPE_MAIN_TYPE (expect_type));
+      xfree (expect_type);
+      return arg1;
+
     case BINOP_CONCAT:
       arg1 = evaluate_subexp_with_coercion (exp, pos, noside);
       arg2 = evaluate_subexp_with_coercion (exp, pos, noside);
@@ -2612,7 +2651,7 @@ evaluate_subexp_for_address (struct expr
       (*pos) += 5 + BYTES_TO_EXP_ELEM (tem + 1);
       x = value_aggregate_elt (exp->elts[pc + 1].type,
 			       &exp->elts[pc + 3].string,
-			       1, noside);
+			       NULL, 1, noside);
       if (x == NULL)
 	error (_("There is no field named %s"), &exp->elts[pc + 3].string);
       return x;
Index: expression.h
===================================================================
RCS file: /cvs/src/src/gdb/expression.h,v
retrieving revision 1.31
diff -u -p -r1.31 expression.h
--- expression.h	15 Sep 2009 16:09:32 -0000	1.31
+++ expression.h	9 Nov 2009 21:48:48 -0000
@@ -88,6 +88,13 @@ enum exp_opcode
        when X is a pointer instead of an aggregate.  */
     STRUCTOP_MPTR,
 
+    /* TYPE_INSTANCE is used when the user specifies a specific
+       type instantiation for overloaded methods/functions.
+
+       The format is:
+       TYPE_INSTANCE num_types type0 ... typeN num_types TYPE_INSTANCE  */
+    TYPE_INSTANCE,
+
     /* end of C++.  */
 
     /* For Modula-2 integer division DIV */
Index: parse.c
===================================================================
RCS file: /cvs/src/src/gdb/parse.c,v
retrieving revision 1.90
diff -u -p -r1.90 parse.c
--- parse.c	2 Jul 2009 17:02:34 -0000	1.90
+++ parse.c	9 Nov 2009 21:48:49 -0000
@@ -837,6 +837,11 @@ operator_length_standard (struct express
       args = 1 + longest_to_int (expr->elts[endpos - 2].longconst);
       break;
 
+    case TYPE_INSTANCE:
+      oplen = 4 + longest_to_int (expr->elts[endpos - 2].longconst);
+      args = 1;
+      break;
+
     case OP_OBJC_MSGCALL:	/* Objective C message (method) call */
       oplen = 4;
       args = 1 + longest_to_int (expr->elts[endpos - 2].longconst);
Index: valops.c
===================================================================
RCS file: /cvs/src/src/gdb/valops.c,v
retrieving revision 1.226
diff -u -p -r1.226 valops.c
--- valops.c	28 Sep 2009 09:16:15 -0000	1.226
+++ valops.c	9 Nov 2009 21:48:49 -0000
@@ -2536,8 +2536,8 @@ check_field (struct type *type, const ch
    the comment before value_struct_elt_for_reference.  */
 
 struct value *
-value_aggregate_elt (struct type *curtype,
-		     char *name, int want_address,
+value_aggregate_elt (struct type *curtype, char *name,
+		     struct type *expect_type, int want_address,
 		     enum noside noside)
 {
   switch (TYPE_CODE (curtype))
@@ -2545,7 +2545,7 @@ value_aggregate_elt (struct type *curtyp
     case TYPE_CODE_STRUCT:
     case TYPE_CODE_UNION:
       return value_struct_elt_for_reference (curtype, 0, curtype, 
-					     name, NULL,
+					     name, expect_type,
 					     want_address, noside);
     case TYPE_CODE_NAMESPACE:
       return value_namespace_elt (curtype, name, 
@@ -2556,6 +2556,57 @@ value_aggregate_elt (struct type *curtyp
     }
 }
 
+/* Compares the two method/function types T1 and T2 for "equality" 
+   with respect to the the methods' parameters.  If the types of the
+   two parameter lists are the same, returns 1; 0 otherwise.  This
+   comparison may ignore any artificial parameters in T1 if
+   SKIP_ARTIFICIAL is non-zero.  This function will ALWAYS skip
+   the first artificial parameter in T1, assumed to be a 'this' pointer.
+
+   The type T2 is expected to have come from make_params (in eval.c).  */
+
+static int
+compare_parameters (struct type *t1, struct type *t2, int skip_artificial)
+{
+  int start = 0;
+
+  if (TYPE_FIELD_ARTIFICIAL (t1, 0))
+    ++start;
+
+  /* If skipping artificial fields, find the first real field
+     in T1. */
+  if (skip_artificial)
+    {
+      while (start < TYPE_NFIELDS (t1)
+	     && TYPE_FIELD_ARTIFICIAL (t1, start))
+	++start;
+    }
+
+  /* Now compare parameters */
+
+  /* Special case: a method taking void.  T1 will contain no
+     non-artificial fields, and T2 will contain TYPE_CODE_VOID.  */
+  if ((TYPE_NFIELDS (t1) - start) == 0 && TYPE_NFIELDS (t2) == 1
+      && TYPE_CODE (TYPE_FIELD_TYPE (t2, 0)) == TYPE_CODE_VOID)
+    return 1;
+
+  if ((TYPE_NFIELDS (t1) - start) == TYPE_NFIELDS (t2))
+    {
+      int i;
+      for (i = 0; i < TYPE_NFIELDS (t2); ++i)
+	{
+	  if (rank_one_type (TYPE_FIELD_TYPE (t1, start + i),
+			      TYPE_FIELD_TYPE (t2, i))
+	      != 0)
+	    return 0;
+	}
+
+      return 1;
+    }
+
+  return 0;
+}
+
 /* C++: Given an aggregate type CURTYPE, and a member name NAME,
    return the address of this member as a "pointer to member" type.
    If INTYPE is non-null, then it will be the type of the member we
@@ -2633,23 +2684,46 @@ value_struct_elt_for_reference (struct t
 	}
       if (t_field_name && strcmp (t_field_name, name) == 0)
 	{
-	  int j = TYPE_FN_FIELDLIST_LENGTH (t, i);
+	  int j;
+	  int len = TYPE_FN_FIELDLIST_LENGTH (t, i);
 	  struct fn_field *f = TYPE_FN_FIELDLIST1 (t, i);
 
 	  check_stub_method_group (t, i);
 
-	  if (intype == 0 && j > 1)
-	    error (_("non-unique member `%s' requires type instantiation"), name);
 	  if (intype)
 	    {
-	      while (j--)
-		if (TYPE_FN_FIELD_TYPE (f, j) == intype)
-		  break;
-	      if (j < 0)
-		error (_("no member function matches that type instantiation"));
-	    }
+	      for (j = 0; j < len; ++j)
+		{
+		  if (compare_parameters (TYPE_FN_FIELD_TYPE (f, j), intype, 0)
+		      || compare_parameters (TYPE_FN_FIELD_TYPE (f, j), intype, 1))
+		    break;
+		}
+
+	      if (j == len)
+		error (_("no member function matches that type instantiation"));	    }
 	  else
-	    j = 0;
+	    {
+	      int ii;
+	      /* Skip artificial methods.  This is necessary if, for example,
+		 the user wants to "print subclass::subclass" with only
+		 one user-defined constructor.  There is no ambiguity in this
+		 case.  */
+	      for (ii = 0; ii < TYPE_FN_FIELDLIST_LENGTH (t, i);
+		   ++ii)
+		{
+		  if (TYPE_FN_FIELD_ARTIFICIAL (f, ii))
+		    --len;
+		}
+
+	      /* Desired method is ambiguous if more than one method is
+		 defined.  */
+	      if (len > 1)
+		error (_("non-unique member `%s' requires type instantiation"), name);
+
+	      /* This assumes, of course, that all artificial methods appear
+		 BEFORE any concrete methods.  */
+	      j = TYPE_FN_FIELDLIST_LENGTH (t, i) - 1;
+	    }
 
 	  if (TYPE_FN_FIELD_STATIC_P (f, j))
 	    {
Index: value.h
===================================================================
RCS file: /cvs/src/src/gdb/value.h,v
retrieving revision 1.149
diff -u -p -r1.149 value.h
--- value.h	31 Aug 2009 20:18:45 -0000	1.149
+++ value.h	9 Nov 2009 21:48:50 -0000
@@ -436,6 +436,7 @@ extern struct value *value_struct_elt (s
 
 extern struct value *value_aggregate_elt (struct type *curtype,
 					  char *name,
+					  struct type *expect_type,
 					  int want_address,
 					  enum noside noside);
 
Index: testsuite/gdb.cp/overload.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.cp/overload.exp,v
retrieving revision 1.11
diff -u -p -r1.11 overload.exp
--- testsuite/gdb.cp/overload.exp	3 Jan 2009 05:58:04 -0000	1.11
+++ testsuite/gdb.cp/overload.exp	9 Nov 2009 21:48:50 -0000
@@ -312,3 +312,24 @@ gdb_test "print overloadNamespace(dummyI
 # I wonder what this is for?
 
 gdb_test "print intToChar(1)" "\\$\[0-9\]+ = 297"
+
+# Test expression evaluation with overloaded methods
+gdb_test "print foo::overload1arg" \
+    "non-unique member `overload1arg' requires type instantiation" \
+    "print foo::overload1arg"
+
+gdb_test "print foo::overload1arg(char***)" \
+    "no member function matches that type instantiation" \
+    "print foo::overload1arg(char***)"
+
+gdb_test "print foo::overload1arg(void)" \
+    "\\$$decimal = {int \\(foo \\* const\\)} $hex <foo::overload1arg\\(\\)>" \
+    "print foo::overload1arg(void)"
+
+foreach t [list char "signed char" "unsigned char" "short" \
+	       "unsigned short" int "unsigned int" long "unsigned long" \
+	       float double] {
+    gdb_test "print foo::overload1arg($t)" \
+	"\\$$decimal = {int \\(foo \\* const, $t\\)} $hex <foo::overload1arg\\($t\\)>" \
+	"print foo::overload1arg($t)"
+}