[RFA] Add method overload resolution to expression parser

[RFA] Add method overload resolution to expression parser

[Keith Seitz]

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

Hi,

The attached patch is the first of several patches which will attempt 
mitigate the requirement of single-quoting every single C++ expression 
passed to the parser.

This patch specifically adds method overload resolution into parser, so 
now gdb correctly understands "print foo::flubber(int)".

I've added a few basic tests to gdb.cp/overload.exp, but when this 
series of patches is nearing completion, I will submit my archer 
"realcpp" tests, which are far more thorough (and currently causes gdb 
to crash a lot).

Tested regression-free on x86 linux.

Keith

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

	* c-exp.y: Add new rule for resolving method overloads.
	Add cleanups for nonempty_typelist.  Changed all users.
	* eval.c (make_params): New function.
	(free_param_types): 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-09-01  Keith Seitz  <keiths@redhat.com>

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

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

Index: c-exp.y
===================================================================
RCS file: /cvs/src/src/gdb/c-exp.y,v
retrieving revision 1.61
diff -u -p -r1.61 c-exp.y
--- c-exp.y	25 Aug 2009 18:40:45 -0000	1.61
+++ c-exp.y	1 Sep 2009 17:41:25 -0000
@@ -117,6 +117,8 @@ static int yylex (void);
 
 void yyerror (char *);
 
+/* Cleanup for 'nonempty_typelist' */
+static struct cleanup *typelist_cleanup;
 %}
 
 /* Although the yacc "value" of an expression is not used,
@@ -401,6 +403,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);
+			  do_cleanups (typelist_cleanup);
+			}
+	;
+
 rcurly	:	'}'
 			{ $$ = end_arglist () - 1; }
 	;
@@ -883,7 +897,7 @@ array_mod:	'[' ']'
 func_mod:	'(' ')'
 			{ $$ = 0; }
 	|	'(' nonempty_typelist ')'
-			{ free ($2); $$ = 0; }
+			{ do_cleanups (typelist_cleanup); $$ = 0; }
 	;
 
 /* We used to try to recognize pointer to member types here, but
@@ -1088,12 +1102,15 @@ typename:	TYPENAME
 nonempty_typelist
 	:	type
 		{ $$ = (struct type **) malloc (sizeof (struct type *) * 2);
+		  typelist_cleanup = make_cleanup (free, $$);
 		  $<ivec>$[0] = 1;	/* Number of types in vector */
 		  $$[1] = $1;
 		}
 	|	nonempty_typelist ',' type
 		{ int len = sizeof (struct type *) * (++($<ivec>1[0]) + 1);
 		  $$ = (struct type **) realloc ((char *) $1, len);
+		  discard_cleanups (typelist_cleanup);
+		  typelist_cleanup = make_cleanup (free, $$);
 		  $$[$<ivec>$[0]] = $3;
 		}
 	;
Index: eval.c
===================================================================
RCS file: /cvs/src/src/gdb/eval.c,v
retrieving revision 1.119
diff -u -p -r1.119 eval.c
--- eval.c	2 Jul 2009 17:04:22 -0000	1.119
+++ eval.c	1 Sep 2009 17:41:25 -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,39 @@ ptrmath_type_p (struct type *type)
     }
 }
 
+static void
+free_param_types (void *arg)
+{
+  struct type *type = (struct type *) arg;
+  xfree (TYPE_FIELDS (type));
+  xfree (TYPE_MAIN_TYPE (type));
+  xfree (type);
+}
+
+/* Constructs a fake method with the given parameter types.  This is
+   used to do overload resolution by the expression parser.  The
+   logical counterpart is compare_parameters in valops.c.  */
+
+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];
+
+  make_cleanup (free_param_types, type);
+  return type;
+}
+
 struct value *
 evaluate_subexp_standard (struct type *expect_type,
 			  struct expression *exp, int *pos,
@@ -684,7 +719,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;
@@ -1709,6 +1744,16 @@ 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;
+      return evaluate_subexp_standard (expect_type, exp, pos, noside);
+
     case BINOP_CONCAT:
       arg1 = evaluate_subexp_with_coercion (exp, pos, noside);
       arg2 = evaluate_subexp_with_coercion (exp, pos, noside);
@@ -2591,7 +2636,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.30
diff -u -p -r1.30 expression.h
--- expression.h	3 Jan 2009 05:57:51 -0000	1.30
+++ expression.h	1 Sep 2009 17:41:25 -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	1 Sep 2009 17:41:25 -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.225
diff -u -p -r1.225 valops.c
--- valops.c	31 Aug 2009 20:18:45 -0000	1.225
+++ valops.c	1 Sep 2009 17:41:26 -0000
@@ -2534,8 +2534,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))
@@ -2543,7 +2543,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, 
@@ -2554,6 +2554,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
@@ -2631,23 +2682,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	1 Sep 2009 17:41:26 -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	1 Sep 2009 17:41:26 -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)"
+}

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

[Keith Seitz]

Ping

[I also have expanded tests for this in my next patch, which adds proper 
C++ operator handling in the parser.]

On 09/01/2009 11:06 AM, Keith Seitz wrote:
> Hi,
>
> The attached patch is the first of several patches which will attempt
> mitigate the requirement of single-quoting every single C++ expression
> passed to the parser.
>
> This patch specifically adds method overload resolution into parser, so
> now gdb correctly understands "print foo::flubber(int)".
>
> I've added a few basic tests to gdb.cp/overload.exp, but when this
> series of patches is nearing completion, I will submit my archer
> "realcpp" tests, which are far more thorough (and currently causes gdb
> to crash a lot).
>
> Tested regression-free on x86 linux.
>
> Keith
>
> ChangeLog
> 2009-09-01 Keith Seitz <keiths@redhat.com>
>
> * c-exp.y: Add new rule for resolving method overloads.
> Add cleanups for nonempty_typelist. Changed all users.
> * eval.c (make_params): New function.
> (free_param_types): 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-09-01 Keith Seitz <keiths@redhat.com>
>
> * gdb.cp/overload.exp: Add tests for resolving overloaded
> methods in expression parsing/evaluation.

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

[Daniel Jacobowitz]

On Tue, Sep 01, 2009 at 11:06:03AM -0700, Keith Seitz wrote:
> +/* Cleanup for 'nonempty_typelist' */
> +static struct cleanup *typelist_cleanup;

Does this cleanup get more complicated uses?  As it stands after this
patch, it's just a more confusing way to write a call to free.  IMO
having cleanups created inside the parser is confusing.

> +/* Constructs a fake method with the given parameter types.  This is
> +   used to do overload resolution by the expression parser.  The
> +   logical counterpart is compare_parameters in valops.c.  */
> +
> +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];
> +
> +  make_cleanup (free_param_types, type);
> +  return type;
> +}

And this is even more confusing... how does this cleanup eventually
get run?  We shouldn't leak it back to the top level.  And I suspect
that since nothing ever copies types, if this type makes it into the
value history we're going to have a problem.

I also wonder why TYPE_INSTANCE is necesary.  All it seems to do is
construct the temporary type.  Can't we do that in the parser,
instead, and use UNOP_CAST?

-- 
Daniel Jacobowitz
CodeSourcery

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

[Tom Tromey]

>>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes:

Keith> The attached patch is the first of several patches which will attempt
Keith> mitigate the requirement of single-quoting every single C++ expression
Keith> passed to the parser.

I have a couple of questions about this.

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.

Keith> +exp     :       exp '(' nonempty_typelist ')' const_or_volatile

This is an interesting production.

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)

Does it work to call an explicitly-specified overload?

  print overloaded(int)(5)

(I assume from reading the patch that this works as expected.)

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.

Keith> +static void
Keith> +free_param_types (void *arg)

Needs a short intro comment.

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?

At the very least I think the comment for make_params should mention
this cleanup side effect.

Tom

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

[Tom Tromey]

>>>>> "Daniel" == Daniel Jacobowitz <drow@false.org> writes:

Keith> +static struct type *
Keith> +make_params (int num_types, struct type **param_types)
Keith> +{
[...]
Keith> +  make_cleanup (free_param_types, type);
Keith> +  return type;
Keith> +}

Daniel> And this is even more confusing... how does this cleanup eventually
Daniel> get run?  We shouldn't leak it back to the top level.  And I suspect
Daniel> that since nothing ever copies types, if this type makes it into the
Daniel> value history we're going to have a problem.

I think this can be addressed by changing the TYPE_INSTANCE case to deal
with the cleanups.

Daniel> I also wonder why TYPE_INSTANCE is necesary.  All it seems to do is
Daniel> construct the temporary type.  Can't we do that in the parser,
Daniel> instead, and use UNOP_CAST?

My reading is that it has to do with the lifetime of a type.

This implementation defers creation of the temporary function type to
the point where it is used.  IIUC, which I hope I do :-), this temporary
type can't really leak out of the TYPE_INSTANCE code.  That is, it is
constructed and used to look up an actual function or method (which has
its own type).

If we used UNOP_CAST then we would have to make a long-lived temporary
type, not only because the type would have to live at least from parsing
through the end of the life of the parsed expression, but also because
the result of the cast would have that temporary type, and thus it would
be capturable.

This would not be as big a deal if the type GC work were finished, but
that is still pending.

Tom

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

[Daniel Jacobowitz]

On Wed, Oct 14, 2009 at 12:54:15PM -0600, Tom Tromey wrote:
> This implementation defers creation of the temporary function type to
> the point where it is used.  IIUC, which I hope I do :-), this temporary
> type can't really leak out of the TYPE_INSTANCE code.  That is, it is
> constructed and used to look up an actual function or method (which has
> its own type).
> 
> If we used UNOP_CAST then we would have to make a long-lived temporary
> type, not only because the type would have to live at least from parsing
> through the end of the life of the parsed expression, but also because
> the result of the cast would have that temporary type, and thus it would
> be capturable.
> 
> This would not be as big a deal if the type GC work were finished, but
> that is still pending.

Maybe we need to get that merged, then, instead of complicated
lifetime-confusing hacks around it :-)

We already have related bugs (and memory leaks).  Try:

(gdb) ptype (int (*) (int, int)) 0
type = int (*)()

We lose the arguments - but we still call make_function_type, and leak
the type.  So it's clear to me that we want a more general solution.

-- 
Daniel Jacobowitz
CodeSourcery

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

[Tom Tromey]

>>>>> "Daniel" == Daniel Jacobowitz <drow@false.org> writes:

Daniel> Maybe we need to get that merged, then, instead of complicated
Daniel> lifetime-confusing hacks around it :-)

Yeah, this would help in several scenarios.  E.g., it would let us
memoize Python Type objects.

However, in this case, if we made a new type, I think we would still
need a new expression operator.  My reasoning is that we are selecting
an overload at expression evaluation time, so we are using the type in a
fundamentally different way from a cast operator.

I don't think I understand why we do this overload selection at
evaluation time and not during the parse.  (It is often preferable to
defer operations until evaluation time so that we can apply them to a
convenience variable's possibly-mutable type -- but that does not apply
in this instance.)

Tom

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

[Keith Seitz]

[-- 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)"
+}

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

[Tom Tromey]

>>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes:

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

Thanks.

Keith> +exp     :       exp '(' nonempty_typelist ')' const_or_volatile
Keith> +			{ int i;
Keith> +			  write_exp_elt_opcode (TYPE_INSTANCE);
Keith> +			  write_exp_elt_longcst ((LONGEST) $<ivec>3[0]);
Keith> +			  for (i = 0; i < $<ivec>3[0]; ++i)
Keith> +			    write_exp_elt_type ($<tvec>3[i + 1]);
Keith> +			  write_exp_elt_longcst((LONGEST) $<ivec>3[0]);

This is ok, because c-exp.y already does this, but the ivec/tvec
overloading still seems weird to me.

Keith> +    case TYPE_INSTANCE:
Keith> +      nargs = longest_to_int (exp->elts[pc + 1].longconst);
Keith> +      arg_types = (struct type **) alloca (nargs * sizeof (struct type *));
Keith> +      for (ix = 0; ix < nargs; ++ix)
Keith> +	arg_types[ix] = exp->elts[pc + 1 + ix + 1].type;
Keith> +
Keith> +      expect_type = make_params (nargs, arg_types);
Keith> +      *(pos) += 3 + nargs;
Keith> +      arg1 = evaluate_subexp_standard (expect_type, exp, pos, noside);
Keith> +      xfree (TYPE_FIELDS (expect_type));
Keith> +      xfree (TYPE_MAIN_TYPE (expect_type));
Keith> +      xfree (expect_type);

These frees should be done with cleanups, in case
evaluate_subexp_standard throws an exception.
I think free_param_types was ok, it was just that the cleanup was made in
a surprising place (make_params) and not run at the "right point" (just
before the end of the TYPE_INSTANCE case).

This is ok with that change.  Thanks.

Tom

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

[Keith Seitz]

On 11/09/2009 02:22 PM, Tom Tromey wrote:
>>>>>> "Keith" == Keith Seitz<keiths@redhat.com>  writes:
> These frees should be done with cleanups, in case
> evaluate_subexp_standard throws an exception.
> I think free_param_types was ok, it was just that the cleanup was made in
> a surprising place (make_params) and not run at the "right point" (just
> before the end of the TYPE_INSTANCE case).

Okay, I apologize -- I *completely* misunderstood what you were saying. 
I've corrected this again. O:-)

>
> This is ok with that change.  Thanks.

I'll wait a day to see if Daniel (or anyone else) has any other concerns 
before committing this. In the meantime, I'll see about digging up 
another expr-cumulative patch or two.

Thank you for your reviewing the patch.

Keith

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

[Daniel Jacobowitz]

On Mon, Nov 09, 2009 at 03:35:28PM -0800, Keith Seitz wrote:
> I'll wait a day to see if Daniel (or anyone else) has any other
> concerns before committing this. In the meantime, I'll see about
> digging up another expr-cumulative patch or two.

I don't have any other concerns.

-- 
Daniel Jacobowitz
CodeSourcery

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

[Keith Seitz]

On 11/10/2009 11:14 AM, Daniel Jacobowitz wrote:
> On Mon, Nov 09, 2009 at 03:35:28PM -0800, Keith Seitz wrote:
>> I'll wait a day to see if Daniel (or anyone else) has any other
>> concerns before committing this. In the meantime, I'll see about
>> digging up another expr-cumulative patch or two.
>
> I don't have any other concerns.
>

Ok, thank you to everyone for reviewing this.  I've committed this patch.

Keith