[PATCH v14 3/4] Add support for lookup, overload resolution and invocation of C++ debug methods

[PATCH v14 3/4] Add support for lookup, overload resolution and invocation of C++ debug methods

[Siva Chandra]

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

Attached is the v14 of this part of the patch series. This version
addresses a concern raised by Doug during his v12/v13 review.

v13 posting: https://sourceware.org/ml/gdb-patches/2014-04/msg00147.html
Doug's comment: https://sourceware.org/ml/gdb-patches/2014-04/msg00229.html

Doug's example:

class base
{
 public:
  virtual int foo (char x) { return x + 1; }
};

class derived : public base
{
 public:
  int foo (char x) { return x + 2; }
  int foo (int x) { return x + 3; }
};

base* ptr = new derived;

Doug's concern:
Suppose we have a debug method for derived::foo (int) and none for
base::foo(char) or derived::foo(char). If I do "p ptr->foo (3)" in
gdb, which method (c++ or debug) should be invoked?

I think, since the invocation happens via the vtable, the foo (char)
version of the derived class should be invoked. My v12/v13 would have
got it wrong. When we do "p ptr->foo(3)", the number 3 is interpreted
to be an int. Consequently, when we perform an overload resolution on
the derived type, GDB would have picked the foo (int) version. To
avoid this, in the attached v14, I have modified to do this:

When (recursively) calling find_overload_match on the dynamic type
object, cast the matching virtual function arguments to the function's
param types explicitly. This way, when overload resolution is
performed on the derived type, the correct function is selected.

Doug's example also made me think of method hiding. If in the same
example above, the derived class did not have foo (char) overridden,
then its foo (int) version hides the base class foo (char) method. In
such cases, we should not look for matching debug methods on the
dynamic type. The attached v14 addresses this also.

2014-04-15  Siva Chandra Reddy  <sivachandra@google.com>

        * eval.c: #include "extension.h".
        (evaluate_subexp_standard): Lookup and invoke C++ methods defined
        in extension languages.
        * valarith.c: #include "extension.h".
        (value_user_defined_cpp_op): Add "src_fn" and "dm_worker"
        arguments.  Return void.  A source match is returned in "src_fn",
        and a debug method match is returned in "dm_worker".
        (value_user_defined_op): Likewise.
        (value_x_binop, value_x_unop): Lookup and invoke
        overloaded operator methods defined in extension languages.
        * valops.c: #include "extension.h".
        (find_method_list): Add "fn_list" and "dm_worker_vec" arguments.
        Return void.  The list of matching source methods is returned in
        "fn_list" and a vector of matching debug method workers is
        returned in "dm_worker_vec".
        (value_find_oload_method_list): Likewise.
        (find_overload_match): Add "dm_worker" parameter.  If the best
        method match is a debug method, then it is returned in
        "dm_worker".  All callers updated.
        (find_oload_champ): Add "dm_worker_vec" argument.  If a caller
        wants to find the champion among a set of debug methods, then it
        has to pass the debug methods in "dm_worker_vec".  All callers
        updated.
        (value_has_indirect_dynamic_type, cast_args_to_param_types,
        equal_param_types_p, derived_hides_base_method): New functions.
        * value.h (find_overload_match): Update signature.

[-- Attachment #2: dm_cpp_v14.txt --]
[-- Type: text/plain, Size: 38014 bytes --]

diff --git a/gdb/eval.c b/gdb/eval.c
index 3e62ead..dad1101 100644
--- a/gdb/eval.c
+++ b/gdb/eval.c
@@ -22,6 +22,7 @@
 #include "symtab.h"
 #include "gdbtypes.h"
 #include "value.h"
+#include "extension.h"
 #include "expression.h"
 #include "target.h"
 #include "frame.h"
@@ -1592,11 +1593,11 @@ evaluate_subexp_standard (struct type *expect_type,
           func_name = (char *) alloca (name_len + 1);
           strcpy (func_name, &exp->elts[string_pc + 1].string);
 
-          find_overload_match (&argvec[1], nargs, func_name,
-                               NON_METHOD, /* not method */
-                               NULL, NULL, /* pass NULL symbol since
+	  find_overload_match (&argvec[1], nargs, func_name,
+			       NON_METHOD, /* not method */
+			       NULL, NULL, /* pass NULL symbol since
 					      symbol is unknown */
-                               NULL, &symp, NULL, 0);
+			       NULL, &symp, NULL, NULL, 0);
 
           /* Now fix the expression being evaluated.  */
           exp->elts[save_pos1 + 2].symbol = symp;
@@ -1626,11 +1627,12 @@ evaluate_subexp_standard (struct type *expect_type,
 	      /* Language is C++, do some overload resolution before
 		 evaluation.  */
 	      struct value *valp = NULL;
+	      struct debug_method_worker *dm_worker = NULL;
 
 	      (void) find_overload_match (&argvec[1], nargs, tstr,
-	                                  METHOD, /* method */
+					  METHOD, /* method */
 					  &arg2,  /* the object */
-					  NULL, &valp, NULL,
+					  NULL, &valp, NULL, &dm_worker,
 					  &static_memfuncp, 0);
 
 	      if (op == OP_SCOPE && !static_memfuncp)
@@ -1640,8 +1642,24 @@ evaluate_subexp_standard (struct type *expect_type,
 			   "`this' pointer"),
 			 function_name);
 		}
-	      argvec[1] = arg2;	/* the ``this'' pointer */
-	      argvec[0] = valp;	/* Use the method found after overload
+
+	      /* If a method implemented in an extension language is the best
+		 match, then invoke it.  */
+	      if (dm_worker != NULL)
+		{
+		  struct value *ret_val;
+		  struct cleanup *dm_worker_cleanup;
+
+		  dm_worker_cleanup = make_cleanup (xfree, dm_worker);
+		  ret_val = invoke_debug_method (dm_worker, arg2, argvec + 2,
+						 nargs - 1);
+		  do_cleanups (dm_worker_cleanup);
+
+		  return ret_val;
+		}
+
+	      argvec[1] = arg2; /* the ``this'' pointer */
+	      argvec[0] = valp; /* Use the method found after overload
 				   resolution.  */
 	    }
 	  else
@@ -1699,9 +1717,9 @@ evaluate_subexp_standard (struct type *expect_type,
 
 	      (void) find_overload_match (&argvec[1], nargs,
 					  NULL,        /* no need for name */
-	                                  NON_METHOD,  /* not method */
-	                                  NULL, function, /* the function */
-					  NULL, &symp, NULL, no_adl);
+					  NON_METHOD,  /* not method */
+					  NULL, function, /* the function */
+					  NULL, &symp, NULL, NULL, no_adl);
 
 	      if (op == OP_VAR_VALUE)
 		{
diff --git a/gdb/valarith.c b/gdb/valarith.c
index 8e863e3..67d90fc 100644
--- a/gdb/valarith.c
+++ b/gdb/valarith.c
@@ -30,6 +30,7 @@
 #include <math.h>
 #include "infcall.h"
 #include "exceptions.h"
+#include "extension.h"
 
 /* Define whether or not the C operator '/' truncates towards zero for
    differently signed operands (truncation direction is undefined in C).  */
@@ -282,24 +283,37 @@ unop_user_defined_p (enum exp_opcode op, struct value *arg1)
    *STATIC_MEMFUNP will be set to 1, and otherwise 0.
    The search if performed through find_overload_match which will handle
    member operators, non member operators, operators imported implicitly or
-   explicitly, and perform correct overload resolution in all of the above
-   situations or combinations thereof.  */
+   explicitly, operators implemented as debug methods, and perform correct
+   overload resolution in all of the above situations or combinations
+   thereof.  If a matching method/function is found in the binary, its
+   corresponding value is returned in SRC_FN.  If a debug method is the best
+   match, then the corresponding debug method worker is returned in
+   DM_WORKER.  On return, exactly one of SRC_FN, DM_WORKER is non-NULL and
+   the other is NULL.  */
 
-static struct value *
+static void
 value_user_defined_cpp_op (struct value **args, int nargs, char *operator,
-                           int *static_memfuncp)
+			   struct value **src_fn,
+			   struct debug_method_worker **dm_worker,
+			   int *static_memfuncp)
 {
 
   struct symbol *symp = NULL;
   struct value *valp = NULL;
+  struct debug_method_worker *worker = NULL;
 
   find_overload_match (args, nargs, operator, BOTH /* could be method */,
-                       &args[0] /* objp */,
-                       NULL /* pass NULL symbol since symbol is unknown */,
-                       &valp, &symp, static_memfuncp, 0);
+		       &args[0] /* objp */,
+		       NULL /* pass NULL symbol since symbol is unknown */,
+		       &valp, &symp, &worker, static_memfuncp, 0);
 
   if (valp)
-    return valp;
+    {
+      *src_fn = valp;
+      *dm_worker = NULL;
+
+      return;
+    }
 
   if (symp)
     {
@@ -307,28 +321,49 @@ value_user_defined_cpp_op (struct value **args, int nargs, char *operator,
          expect a reference as its first argument
          rather the explicit structure.  */
       args[0] = value_ind (args[0]);
-      return value_of_variable (symp, 0);
+      *src_fn = value_of_variable (symp, 0);
+      *dm_worker = NULL;
+
+      return;
+    }
+
+  if (worker)
+    {
+      *dm_worker = worker;
+      *src_fn = NULL;
+
+      return;
     }
 
   error (_("Could not find %s."), operator);
 }
 
-/* Lookup user defined operator NAME.  Return a value representing the
-   function, otherwise return NULL.  */
+/* Lookup user defined operator NAME.  If a matching operator is found in the
+   binary, its corresponding value is returned in SRC_FN.  If a debug method
+   is the best matching operator, then the corresponding debug method worker
+   is returned in DM_WORKER.  On return, exactly one of SRC_FN, DM_WORKER is
+   set to non-NULL and the other is set to NULL.  */
 
-static struct value *
+static void
 value_user_defined_op (struct value **argp, struct value **args, char *name,
-                       int *static_memfuncp, int nargs)
+		       int *static_memfuncp, int nargs,
+		       struct value **src_fn,
+		       struct debug_method_worker **dm_worker)
 {
   struct value *result = NULL;
 
   if (current_language->la_language == language_cplus)
-    result = value_user_defined_cpp_op (args, nargs, name, static_memfuncp);
+    {
+      value_user_defined_cpp_op (args, nargs, name, src_fn, dm_worker,
+				 static_memfuncp);
+    }
   else
-    result = value_struct_elt (argp, args, name, static_memfuncp,
-                               "structure");
-
-  return result;
+    {
+      result = value_struct_elt (argp, args, name, static_memfuncp,
+				 "structure");
+      *src_fn = result;
+      *dm_worker = NULL;
+    }
 }
 
 /* We know either arg1 or arg2 is a structure, so try to find the right
@@ -345,6 +380,7 @@ value_x_binop (struct value *arg1, struct value *arg2, enum exp_opcode op,
 	       enum exp_opcode otherop, enum noside noside)
 {
   struct value **argvec;
+  struct debug_method_worker *dm_worker = NULL;
   char *ptr;
   char tstr[13];
   int static_memfuncp;
@@ -359,6 +395,7 @@ value_x_binop (struct value *arg1, struct value *arg2, enum exp_opcode op,
     error (_("Can't do that binary op on that type"));	/* FIXME be explicit */
 
   argvec = (struct value **) alloca (sizeof (struct value *) * 4);
+  argvec[0] = NULL;
   argvec[1] = value_addr (arg1);
   argvec[2] = arg2;
   argvec[3] = 0;
@@ -471,8 +508,8 @@ value_x_binop (struct value *arg1, struct value *arg2, enum exp_opcode op,
       error (_("Invalid binary operation specified."));
     }
 
-  argvec[0] = value_user_defined_op (&arg1, argvec + 1, tstr,
-                                     &static_memfuncp, 2);
+  value_user_defined_op (&arg1, argvec + 1, tstr, &static_memfuncp, 2,
+			 &argvec[0], &dm_worker);
 
   if (argvec[0])
     {
@@ -492,6 +529,14 @@ value_x_binop (struct value *arg1, struct value *arg2, enum exp_opcode op,
       return call_function_by_hand (argvec[0], 2 - static_memfuncp,
 				    argvec + 1);
     }
+  if (dm_worker != NULL)
+    {
+      struct cleanup *dm_worker_cleanup = make_cleanup (xfree, dm_worker);
+      struct value *ret_val = invoke_debug_method (dm_worker, arg1, &arg2, 1);
+
+      do_cleanups (dm_worker_cleanup);
+      return ret_val;
+    }
   throw_error (NOT_FOUND_ERROR,
                _("member function %s not found"), tstr);
 #ifdef lint
@@ -510,6 +555,7 @@ value_x_unop (struct value *arg1, enum exp_opcode op, enum noside noside)
 {
   struct gdbarch *gdbarch = get_type_arch (value_type (arg1));
   struct value **argvec;
+  struct debug_method_worker *dm_worker;
   char *ptr;
   char tstr[13], mangle_tstr[13];
   int static_memfuncp, nargs;
@@ -523,6 +569,7 @@ value_x_unop (struct value *arg1, enum exp_opcode op, enum noside noside)
     error (_("Can't do that unary op on that type"));	/* FIXME be explicit */
 
   argvec = (struct value **) alloca (sizeof (struct value *) * 4);
+  argvec[0] = NULL;
   argvec[1] = value_addr (arg1);
   argvec[2] = 0;
 
@@ -574,8 +621,8 @@ value_x_unop (struct value *arg1, enum exp_opcode op, enum noside noside)
       error (_("Invalid unary operation specified."));
     }
 
-  argvec[0] = value_user_defined_op (&arg1, argvec + 1, tstr,
-                                     &static_memfuncp, nargs);
+  value_user_defined_op (&arg1, argvec + 1, tstr, &static_memfuncp, nargs,
+			 &argvec[0], &dm_worker);
 
   if (argvec[0])
     {
@@ -595,6 +642,14 @@ value_x_unop (struct value *arg1, enum exp_opcode op, enum noside noside)
 	}
       return call_function_by_hand (argvec[0], nargs, argvec + 1);
     }
+  if (dm_worker != NULL)
+    {
+      struct cleanup *dm_worker_cleanup = make_cleanup (xfree, dm_worker);
+      struct value *ret_val = invoke_debug_method (dm_worker, arg1, NULL, 0);
+
+      do_cleanups (dm_worker_cleanup);
+      return ret_val;
+    }
   throw_error (NOT_FOUND_ERROR,
                _("member function %s not found"), tstr);
 
diff --git a/gdb/valops.c b/gdb/valops.c
index 7f2d5f0..a810f25 100644
--- a/gdb/valops.c
+++ b/gdb/valops.c
@@ -42,6 +42,7 @@
 #include "observer.h"
 #include "objfiles.h"
 #include "exceptions.h"
+#include "extension.h"
 
 extern unsigned int overload_debug;
 /* Local functions.  */
@@ -70,8 +71,8 @@ int find_oload_champ_namespace_loop (struct value **, int,
 				     const int no_adl);
 
 static int find_oload_champ (struct value **, int, int,
-			     struct fn_field *, struct symbol **,
-			     struct badness_vector **);
+			     struct fn_field *, VEC (debug_method_worker_ptr) *,
+			     struct symbol **, struct badness_vector **);
 
 static int oload_method_static (int, struct fn_field *, int);
 
@@ -98,9 +99,10 @@ static CORE_ADDR allocate_space_in_inferior (int);
 
 static struct value *cast_into_complex (struct type *, struct value *);
 
-static struct fn_field *find_method_list (struct value **, const char *,
-					  int, struct type *, int *,
-					  struct type **, int *);
+static void find_method_list (struct value **, const char *,
+			      int, struct type *, struct fn_field **, int *,
+			      VEC (debug_method_worker_ptr) **,
+			      struct type **, int *);
 
 void _initialize_valops (void);
 
@@ -2255,53 +2257,83 @@ value_struct_elt_bitpos (struct value **argp, int bitpos, struct type *ftype,
 }
 
 /* Search through the methods of an object (and its bases) to find a
-   specified method.  Return the pointer to the fn_field list of
-   overloaded instances.
+   specified method.  Return the pointer to the fn_field list FN_LIST of
+   overloaded instances defined in the source language.  If available
+   and matching, a vector of matching debug methods defined in
+   extension languages are also returned in DM_WORKER_VEC
 
    Helper function for value_find_oload_list.
    ARGP is a pointer to a pointer to a value (the object).
    METHOD is a string containing the method name.
    OFFSET is the offset within the value.
    TYPE is the assumed type of the object.
+   FN_LIST The pointer to matching overloaded instances defined in
+      source language.
    NUM_FNS is the number of overloaded instances.
+   DM_WORKER_VEC The vector of matching debug method workers.
    BASETYPE is set to the actual type of the subobject where the
       method is found.
    BOFFSET is the offset of the base subobject where the method is found.  */
 
-static struct fn_field *
+static void
 find_method_list (struct value **argp, const char *method,
-		  int offset, struct type *type, int *num_fns,
+		  int offset, struct type *type,
+		  struct fn_field **fn_list, int *num_fns,
+		  VEC (debug_method_worker_ptr) **dm_worker_vec,
 		  struct type **basetype, int *boffset)
 {
   int i;
-  struct fn_field *f;
-  CHECK_TYPEDEF (type);
+  struct fn_field *f = NULL;
 
-  *num_fns = 0;
+  CHECK_TYPEDEF (type);
 
-  /* First check in object itself.  */
-  for (i = TYPE_NFN_FIELDS (type) - 1; i >= 0; i--)
+  /* First check in object itself.
+     This function is called recursively to search through base classes.
+     If there is a source method match found at some stage, then we need not
+     look for source methods in consequent recursive calls.  */
+  if (fn_list != NULL && (*fn_list) == NULL)
     {
-      /* pai: FIXME What about operators and type conversions?  */
-      const char *fn_field_name = TYPE_FN_FIELDLIST_NAME (type, i);
-
-      if (fn_field_name && (strcmp_iw (fn_field_name, method) == 0))
+      for (i = TYPE_NFN_FIELDS (type) - 1; i >= 0; i--)
 	{
-	  int len = TYPE_FN_FIELDLIST_LENGTH (type, i);
-	  struct fn_field *f = TYPE_FN_FIELDLIST1 (type, i);
+	  /* pai: FIXME What about operators and type conversions?  */
+	  const char *fn_field_name = TYPE_FN_FIELDLIST_NAME (type, i);
+
+	  if (fn_field_name && (strcmp_iw (fn_field_name, method) == 0))
+	    {
+	      int len = TYPE_FN_FIELDLIST_LENGTH (type, i);
+	      f = TYPE_FN_FIELDLIST1 (type, i);
+	      *fn_list = f;
 
-	  *num_fns = len;
-	  *basetype = type;
-	  *boffset = offset;
+	      *num_fns = len;
+	      *basetype = type;
+	      *boffset = offset;
 
-	  /* Resolve any stub methods.  */
-	  check_stub_method_group (type, i);
+	      /* Resolve any stub methods.  */
+	      check_stub_method_group (type, i);
 
-	  return f;
+	      break;
+	    }
 	}
     }
 
-  /* Not found in object, check in base subobjects.  */
+  /* Unlike source methods, debug methods can be accumulated over successive
+     recursive calls.  In other words, a debug method named 'm' in a class
+     will not hide a debug method named 'm' in its base class(es).  */
+  if (dm_worker_vec)
+    {
+      VEC (debug_method_worker_ptr) *worker_vec = NULL, *new_vec = NULL;
+
+      worker_vec = get_matching_debug_method_workers (type, method);
+      new_vec = VEC_merge (debug_method_worker_ptr, *dm_worker_vec, worker_vec);
+
+      VEC_free (debug_method_worker_ptr, *dm_worker_vec);
+      VEC_free (debug_method_worker_ptr, worker_vec);
+      *dm_worker_vec = new_vec;
+    }
+
+  /* If source methods are not found in current class, look for them in the
+     base classes.  We have to go through the base classes to gather extension
+     methods anyway.  */
   for (i = TYPE_N_BASECLASSES (type) - 1; i >= 0; i--)
     {
       int base_offset;
@@ -2318,28 +2350,35 @@ find_method_list (struct value **argp, const char *method,
 	{
 	  base_offset = TYPE_BASECLASS_BITPOS (type, i) / 8;
 	}
-      f = find_method_list (argp, method, base_offset + offset,
-			    TYPE_BASECLASS (type, i), num_fns, 
-			    basetype, boffset);
-      if (f)
-	return f;
+
+      find_method_list (argp, method, base_offset + offset,
+			TYPE_BASECLASS (type, i), fn_list, num_fns,
+			dm_worker_vec, basetype, boffset);
     }
-  return NULL;
 }
 
-/* Return the list of overloaded methods of a specified name.
+/* Return the list of overloaded methods of a specified name.  The methods
+   could be those GDB finds in the binary, or debug methods.  Methods found
+   in the binary are returned in FN_LIST, and debug methods are returned in
+   DM_WORKER_VEC.
 
    ARGP is a pointer to a pointer to a value (the object).
    METHOD is the method name.
    OFFSET is the offset within the value contents.
+   FN_LIST The pointer to matching overloaded instances defined in
+      source language.
    NUM_FNS is the number of overloaded instances.
+   DM_WORKER_VEC The vector of matching debug method workers defined in
+      extension languages.
    BASETYPE is set to the type of the base subobject that defines the
       method.
    BOFFSET is the offset of the base subobject which defines the method.  */
 
-static struct fn_field *
+static void
 value_find_oload_method_list (struct value **argp, const char *method,
-			      int offset, int *num_fns, 
+                              int offset, struct fn_field **fn_list,
+                              int *num_fns,
+                              VEC (debug_method_worker_ptr) **dm_worker_vec,
 			      struct type **basetype, int *boffset)
 {
   struct type *t;
@@ -2361,8 +2400,124 @@ value_find_oload_method_list (struct value **argp, const char *method,
     error (_("Attempt to extract a component of a "
 	     "value that is not a struct or union"));
 
-  return find_method_list (argp, method, 0, t, num_fns, 
-			   basetype, boffset);
+  /* Clear the lists.  */
+  if (fn_list != NULL)
+    {
+      *fn_list = NULL;
+      *num_fns = 0;
+    }
+  if (dm_worker_vec != NULL)
+    *dm_worker_vec = NULL;
+
+  find_method_list (argp, method, 0, t, fn_list, num_fns, dm_worker_vec,
+		    basetype, boffset);
+}
+
+/* Return the dynamic type of OBJ.  NULL is returned if OBJ does not have any
+   dynamic type.  */
+
+static struct type *
+value_has_indirect_dynamic_type (struct value *obj)
+{
+  struct type *stype, *dtype, *dtype_ind;
+
+  stype = check_typedef (TYPE_TARGET_TYPE (value_type (obj)));
+  dtype_ind = value_rtti_indirect_type (obj, NULL, NULL, NULL);
+  dtype = dtype_ind ? check_typedef (TYPE_TARGET_TYPE (dtype_ind)) : stype;
+
+  if (class_types_same_p (stype, dtype))
+    return NULL;
+  else
+    return dtype_ind;
+}
+
+/* Casts the arguments in the array ARGS to the types of the parameters of
+   the M-th method in FNS_PTR.  The length of the array ARGS is given by
+   NARGS.  This is a helper function for find_overload_match.  */
+
+static void
+cast_args_to_param_types (struct value **args, int nargs,
+			  struct fn_field *fns_ptr, int m)
+{
+  int i;
+
+  gdb_assert (TYPE_NFIELDS (TYPE_FN_FIELD_TYPE (fns_ptr, m)) == nargs);
+
+  for (i = 1; i < nargs; i++)
+    {
+      struct type *param_type = TYPE_FN_FIELD_ARGS (fns_ptr, m)[i].type;
+      struct type *arg_type = check_typedef (value_type (args[i]));
+
+      CHECK_TYPEDEF (param_type);
+      if (TYPE_CODE (param_type) == TYPE_CODE_REF
+	  && TYPE_CODE (arg_type) != TYPE_CODE_REF)
+	param_type = TYPE_TARGET_TYPE (param_type);
+
+      args[i] = value_cast (param_type, args[i]);
+    }
+}
+
+/* Checks if the N1-th method in FNS_PTR1 has exactly the same parameters
+   as that of the N2-th method in FNS_PTR2.
+   Returns 1 is equal, zero otherwise.  */
+
+static int
+equal_param_types_p (struct fn_field *fns_ptr1, int n1,
+		     struct fn_field *fns_ptr2, int n2)
+{
+  int i;
+
+  if (TYPE_NFIELDS (TYPE_FN_FIELD_TYPE (fns_ptr1, n1))
+      != TYPE_NFIELDS (TYPE_FN_FIELD_TYPE (fns_ptr2, n2)))
+    return 0;
+
+  for (i = 1; i < TYPE_NFIELDS (TYPE_FN_FIELD_TYPE (fns_ptr1, n1)); i++)
+    {
+      struct type *type1 = TYPE_FN_FIELD_ARGS (fns_ptr1, n1)[i].type;
+      struct type *type2 = TYPE_FN_FIELD_ARGS (fns_ptr2, n2)[i].type;
+
+      if (!types_deeply_equal (type1, type2))
+	return 0;
+    }
+
+  return 1;
+}
+
+/* Checks if the derived type DTYPE hides a method NAME of its base class.
+   The base class method is the M-th method in FNS_PTR.  */
+
+static int
+derived_hides_base_method (struct type *dtype, const char *name,
+			   struct fn_field *fns_ptr, int m)
+{
+  int i, ret_val = 0;
+
+  dtype = check_typedef (dtype);
+  gdb_assert (TYPE_CODE (dtype) == TYPE_CODE_STRUCT);
+
+  for (i = 0; i < TYPE_NFN_FIELDS (dtype); i++)
+    {
+      const char *fn_field_name = TYPE_FN_FIELDLIST_NAME (dtype, i);
+
+      if (fn_field_name && (strcmp_iw (fn_field_name, name) == 0))
+	{
+	  int j;
+
+	  /* If a method with the same name is found in the dynamic type,
+	     then assume that it hides the base method until a method with
+	     matching param types is found.  */
+	  ret_val = 1;
+
+	  for (j = 0; j < TYPE_FN_FIELDLIST_LENGTH (dtype, i); j++)
+	    {
+	      if (equal_param_types_p (TYPE_FN_FIELDLIST1 (dtype, i), j,
+				       fns_ptr, m))
+		return 0;
+	    }
+	}
+    }
+
+  return ret_val;
 }
 
 /* Given an array of arguments (ARGS) (which includes an
@@ -2390,9 +2545,10 @@ value_find_oload_method_list (struct value **argp, const char *method,
    Return value is an integer: 0 -> good match, 10 -> debugger applied
    non-standard coercions, 100 -> incompatible.
 
-   If a method is being searched for, VALP will hold the value.
-   If a non-method is being searched for, SYMP will hold the symbol 
-   for it.
+   If the best match is a debug method then DM_WORKER will hold the matching
+   debug method worker.  Otherwise, VALP will hold the value if a method is
+   being searched for, or SYMP will hold the symbol for the matching function
+   if a non-method is being searched for.
 
    If a method is being searched for, and it is a static method,
    then STATICP will point to a non-zero value.
@@ -2410,6 +2566,7 @@ find_overload_match (struct value **args, int nargs,
 		     const char *name, enum oload_search_type method,
 		     struct value **objp, struct symbol *fsym,
 		     struct value **valp, struct symbol **symp, 
+		     struct debug_method_worker **dm_worker,
 		     int *staticp, const int no_adl)
 {
   struct value *obj = (objp ? *objp : NULL);
@@ -2417,16 +2574,24 @@ find_overload_match (struct value **args, int nargs,
   /* Index of best overloaded function.  */
   int func_oload_champ = -1;
   int method_oload_champ = -1;
+  int src_method_oload_champ = -1;
+  int src_method_oload_champ_orig = -1;
+  int ext_method_oload_champ = -1;
+  int src_and_ext_equal = 0;
 
   /* The measure for the current best match.  */
   struct badness_vector *method_badness = NULL;
   struct badness_vector *func_badness = NULL;
+  struct badness_vector *ext_method_badness = NULL;
+  struct badness_vector *src_method_badness = NULL;
 
   struct value *temp = obj;
   /* For methods, the list of overloaded methods.  */
   struct fn_field *fns_ptr = NULL;
   /* For non-methods, the list of overloaded function symbols.  */
   struct symbol **oload_syms = NULL;
+  /* For debug methods, the VEC of debug method workers.  */
+  VEC (debug_method_worker_ptr) *dm_worker_vec = NULL;
   /* Number of overloaded instances being considered.  */
   int num_fns = 0;
   struct type *basetype = NULL;
@@ -2438,6 +2603,8 @@ find_overload_match (struct value **args, int nargs,
   const char *func_name = NULL;
   enum oload_classification match_quality;
   enum oload_classification method_match_quality = INCOMPATIBLE;
+  enum oload_classification src_method_match_quality = INCOMPATIBLE;
+  enum oload_classification ext_method_match_quality = INCOMPATIBLE;
   enum oload_classification func_match_quality = INCOMPATIBLE;
 
   /* Get the list of overloaded methods or functions.  */
@@ -2466,12 +2633,12 @@ find_overload_match (struct value **args, int nargs,
 	}
 
       /* Retrieve the list of methods with the name NAME.  */
-      fns_ptr = value_find_oload_method_list (&temp, name, 
-					      0, &num_fns, 
-					      &basetype, &boffset);
+      value_find_oload_method_list (&temp, name, 0, &fns_ptr, &num_fns,
+				    dm_worker ? &dm_worker_vec : NULL,
+				    &basetype, &boffset);
       /* If this is a method only search, and no methods were found
          the search has faild.  */
-      if (method == METHOD && (!fns_ptr || !num_fns))
+      if (method == METHOD && (!fns_ptr || !num_fns) && !dm_worker_vec)
 	error (_("Couldn't find method %s%s%s"),
 	       obj_type_name,
 	       (obj_type_name && *obj_type_name) ? "::" : "",
@@ -2482,18 +2649,87 @@ find_overload_match (struct value **args, int nargs,
       if (fns_ptr)
 	{
 	  gdb_assert (TYPE_DOMAIN_TYPE (fns_ptr[0].type) != NULL);
-	  method_oload_champ = find_oload_champ (args, nargs,
-	                                         num_fns, fns_ptr,
-	                                         NULL, &method_badness);
 
-	  method_match_quality =
-	      classify_oload_match (method_badness, nargs,
-	                            oload_method_static (method, fns_ptr,
-	                                                 method_oload_champ));
+	  src_method_oload_champ = find_oload_champ (args, nargs,
+						     num_fns, fns_ptr, NULL,
+						     NULL, &src_method_badness);
+
+	  src_method_match_quality = classify_oload_match
+	    (src_method_badness, nargs,
+	     oload_method_static (method, fns_ptr, src_method_oload_champ));
 
-	  make_cleanup (xfree, method_badness);
+	  make_cleanup (xfree, src_method_badness);
 	}
 
+      if (dm_worker && VEC_length (debug_method_worker_ptr, dm_worker_vec))
+	{
+	  ext_method_oload_champ = find_oload_champ (args, nargs,
+						     0, NULL, dm_worker_vec,
+						     NULL, &ext_method_badness);
+	  ext_method_match_quality = classify_oload_match (ext_method_badness,
+							   nargs, 0);
+	  make_cleanup (xfree, ext_method_badness);
+	  make_debug_method_worker_vec_cleanup (dm_worker_vec);
+	}
+
+      if (src_method_oload_champ >= 0 && ext_method_oload_champ >= 0)
+	{
+	  switch (compare_badness (ext_method_badness, src_method_badness))
+	    {
+	      case 0: /* Src method and debug method are equally good.  */
+		src_and_ext_equal = 1;
+		/* If src method and debug method are equally good, then debug
+		   method should be the winner.  Hence, fall through to the
+		   case where a debug method is better than the source
+		   method, except when the debug method match quality is
+		   non-standard.  */
+		/* FALLTHROUGH */
+	      case 1: /* Src method and ext method are incompatible.  */
+		/* If ext method match is not standard, then let source method
+		   win.  Otherwise, fallthrough to let debug method win.  */
+		if (ext_method_match_quality != STANDARD)
+		  {
+		    method_oload_champ = src_method_oload_champ;
+		    method_badness = src_method_badness;
+		    ext_method_oload_champ = -1;
+		    method_match_quality = src_method_match_quality;
+		    break;
+		  }
+		/* FALLTHROUGH */
+	      case 2: /* Ext method is champion.  */
+		method_oload_champ = ext_method_oload_champ;
+		method_badness = ext_method_badness;
+		/* We save the source overload champ index so that it can be
+		   used to determine whether the source method is virtual
+		   later in this function.  */
+		src_method_oload_champ_orig = src_method_oload_champ;
+		src_method_oload_champ = -1;
+		method_match_quality = ext_method_match_quality;
+		break;
+	      case 3: /* Src method is champion.  */
+		method_oload_champ = src_method_oload_champ;
+		method_badness = src_method_badness;
+		ext_method_oload_champ = -1;
+		method_match_quality = src_method_match_quality;
+		break;
+	      default:
+		gdb_assert_not_reached ("Internal error: unexpected overload "
+					"comparison result");
+		break;
+	    }
+	}
+      else if (src_method_oload_champ >= 0)
+	{
+	  method_oload_champ = src_method_oload_champ;
+	  method_badness = src_method_badness;
+	  method_match_quality = src_method_match_quality;
+	}
+      else if (ext_method_oload_champ >= 0)
+	{
+	  method_oload_champ = ext_method_oload_champ;
+	  method_badness = ext_method_badness;
+	  method_match_quality = ext_method_match_quality;
+	}
     }
 
   if (method == NON_METHOD || method == BOTH)
@@ -2636,21 +2872,6 @@ find_overload_match (struct value **args, int nargs,
 		 func_name);
     }
 
-  if (staticp != NULL)
-    *staticp = oload_method_static (method, fns_ptr, method_oload_champ);
-
-  if (method_oload_champ >= 0)
-    {
-      if (TYPE_FN_FIELD_VIRTUAL_P (fns_ptr, method_oload_champ))
-	*valp = value_virtual_fn_field (&temp, fns_ptr, method_oload_champ,
-					basetype, boffset);
-      else
-	*valp = value_fn_field (&temp, fns_ptr, method_oload_champ,
-				basetype, boffset);
-    }
-  else
-    *symp = oload_syms[func_oload_champ];
-
   if (objp)
     {
       struct type *temp_type = check_typedef (value_type (temp));
@@ -2660,10 +2881,112 @@ find_overload_match (struct value **args, int nargs,
 	  && (TYPE_CODE (objtype) == TYPE_CODE_PTR
 	      || TYPE_CODE (objtype) == TYPE_CODE_REF))
 	{
-	  temp = value_addr (temp);
+	  *objp = value_addr (temp);
+	}
+      else
+	*objp = temp;
+    }
+
+  if (staticp != NULL)
+    *staticp = oload_method_static (method, fns_ptr, method_oload_champ);
+
+  if (method_oload_champ >= 0)
+    {
+      if (src_method_oload_champ >= 0)
+	{
+	  /* Even if the source method was the winner, it could be a virtual
+	     function.  In such a case, we should look for the possibility of
+	     debug methods defined on the object's dynamic type.  */
+	  if (TYPE_FN_FIELD_VIRTUAL_P (fns_ptr, method_oload_champ))
+	    {
+	      struct type *dtype;
+
+	      dtype = value_has_indirect_dynamic_type (args[0]);
+	      /* Look for better methods in the dynamic type only if the
+		 dynamic type does not hide the base class virtual method.  */
+	      if (dtype != NULL
+		  && !derived_hides_base_method (TYPE_TARGET_TYPE (dtype),
+						 name, fns_ptr,
+						 method_oload_champ))
+		{
+		  /* If the object has a dynamic type, then look for matching
+		     methods for its dynamic type.  */
+		  args[0] = value_cast (dtype, args[0]);
+		  do_cleanups (all_cleanups);
+		  /* Even if the derived class does not hide the base class
+		     method, it could define another method with the same name
+		     but with compatible parameters.  Avoid the chance of
+		     picking up the wrong function by explicitly casting the
+		     args to the virtual function param types.  
+
+		     Example:
+
+		     class base
+		     {
+		     public:
+		       virtual int foo (char i);
+		     };
+
+		     class derived : public base
+		     {
+		     public:
+		       virtual int foo (char i);
+		       int foo (int i);
+		     };
+
+		     If the arg to base::foo was as int, then looking for
+		     matching methods in the dynamic object will match foo(int)
+		     and not foo (char) even though the derived class overrides
+		     (does not hide) the base class virtual method.  */
+		  cast_args_to_param_types (args, nargs, fns_ptr,
+					    method_oload_champ);
+		  return find_overload_match (args, nargs, name, method,
+					      objp, fsym, valp, symp, dm_worker,
+					      staticp, no_adl);
+		}
+	      else
+		*valp = value_virtual_fn_field (&temp, fns_ptr,
+						method_oload_champ,
+						basetype, boffset);
+	    }
+	  else
+	    *valp = value_fn_field (&temp, fns_ptr, method_oload_champ,
+				    basetype, boffset);
+	}
+      else
+	{
+	  /* Debug methods cannot be virtual.  However, if a debug method is as
+	     good as the source method, and if the source method is virtual, we
+	     should look for the possibility of better matching methods defined
+	     for the dynamic type of the object.  */
+	  if (src_and_ext_equal
+	      && TYPE_FN_FIELD_VIRTUAL_P (fns_ptr, src_method_oload_champ_orig))
+	    {
+	      struct type *dtype;
+
+	      dtype = value_has_indirect_dynamic_type (args[0]);
+	      if (dtype != NULL
+		  && !derived_hides_base_method (TYPE_TARGET_TYPE (dtype),
+						 name, fns_ptr,
+						 src_method_oload_champ_orig))
+		{
+		  args[0] = value_cast (dtype, args[0]);
+		  do_cleanups (all_cleanups);
+		  cast_args_to_param_types (args, nargs, fns_ptr,
+					    src_method_oload_champ_orig);
+		  return find_overload_match (args, nargs, name, method,
+					      objp, fsym, valp, symp, dm_worker,
+					      staticp, no_adl);
+		}
+	    }
+
+	  *dm_worker = clone_debug_method_worker (
+	     VEC_index (debug_method_worker_ptr, dm_worker_vec,
+			ext_method_oload_champ));
 	}
-      *objp = temp;
     }
+  else
+    *symp = oload_syms[func_oload_champ];
 
   do_cleanups (all_cleanups);
 
@@ -2798,7 +3121,7 @@ find_oload_champ_namespace_loop (struct value **args, int nargs,
     ++num_fns;
 
   new_oload_champ = find_oload_champ (args, nargs, num_fns,
-				      NULL, new_oload_syms,
+				      NULL, NULL, new_oload_syms,
 				      &new_oload_champ_bv);
 
   /* Case 1: We found a good match.  Free earlier matches (if any),
@@ -2836,9 +3159,13 @@ find_oload_champ_namespace_loop (struct value **args, int nargs,
 
 /* Look for a function to take NARGS args of ARGS.  Find
    the best match from among the overloaded methods or functions
-   given by FNS_PTR or OLOAD_SYMS, respectively.  One, and only one of
-   FNS_PTR and OLOAD_SYMS can be non-NULL.  The number of
-   methods/functions in the non-NULL list is given by NUM_FNS.
+   given by FNS_PTR or OLOAD_SYMS or DM_WORKER_VEC, respectively.
+   One, and only one of FNS_PTR, OLOAD_SYMS and DM_WORKER_VEC can be
+   non-NULL.
+
+   If DM_WORKER_VEC is NULL, then the length of the arrays FNS_PTR
+   or OLOAD_SYMS (whichever is non-NULL) is specified in NUM_FNS.
+
    Return the index of the best match; store an indication of the
    quality of the match in OLOAD_CHAMP_BV.
 
@@ -2847,10 +3174,13 @@ find_oload_champ_namespace_loop (struct value **args, int nargs,
 static int
 find_oload_champ (struct value **args, int nargs,
 		  int num_fns, struct fn_field *fns_ptr,
+		  VEC (debug_method_worker_ptr) *dm_worker_vec,
 		  struct symbol **oload_syms,
 		  struct badness_vector **oload_champ_bv)
 {
   int ix;
+  int fn_count;
+  int dm_worker_vec_n = VEC_length (debug_method_worker_ptr, dm_worker_vec);
   /* A measure of how good an overloaded instance is.  */
   struct badness_vector *bv;
   /* Index of best overloaded function.  */
@@ -2860,40 +3190,49 @@ find_oload_champ (struct value **args, int nargs,
   /* 0 => no ambiguity, 1 => two good funcs, 2 => incomparable funcs.  */
 
   /* A champion can be found among methods alone, or among functions
-     alone, but not both.  */
-  gdb_assert ((fns_ptr != NULL) + (oload_syms != NULL) == 1);
+     alone, or in debug methods alone, but not in more than one of these
+     groups.  */
+  gdb_assert ((fns_ptr != NULL) + (oload_syms != NULL) + (dm_worker_vec != NULL)
+	      == 1);
 
   *oload_champ_bv = NULL;
 
+  fn_count = (dm_worker_vec != NULL
+	      ? VEC_length (debug_method_worker_ptr, dm_worker_vec)
+	      : num_fns);
   /* Consider each candidate in turn.  */
-  for (ix = 0; ix < num_fns; ix++)
+  for (ix = 0; ix < fn_count; ix++)
     {
       int jj;
-      int static_offset;
+      int static_offset = 0;
       int nparms;
       struct type **parm_types;
+      struct debug_method_worker *worker = NULL;
 
-      if (fns_ptr != NULL)
+      if (dm_worker_vec != NULL)
 	{
-	  nparms = TYPE_NFIELDS (TYPE_FN_FIELD_TYPE (fns_ptr, ix));
-	  static_offset = oload_method_static (1, fns_ptr, ix);
+	  worker = VEC_index (debug_method_worker_ptr, dm_worker_vec, ix);
+	  parm_types = get_debug_method_arg_types (worker, &nparms);
 	}
       else
 	{
-	  /* If it's not a method, this is the proper place.  */
-	  nparms = TYPE_NFIELDS (SYMBOL_TYPE (oload_syms[ix]));
-	  static_offset = 0;
+	  if (fns_ptr != NULL)
+	    {
+	      nparms = TYPE_NFIELDS (TYPE_FN_FIELD_TYPE (fns_ptr, ix));
+	      static_offset = oload_method_static (1, fns_ptr, ix);
+	    }
+	  else
+	    nparms = TYPE_NFIELDS (SYMBOL_TYPE (oload_syms[ix]));
+
+	  parm_types = (struct type **)
+	    xmalloc (nparms * (sizeof (struct type *)));
+	  for (jj = 0; jj < nparms; jj++)
+	    parm_types[jj] = (fns_ptr != NULL
+			      ? (TYPE_FN_FIELD_ARGS (fns_ptr, ix)[jj].type)
+			      : TYPE_FIELD_TYPE (SYMBOL_TYPE (oload_syms[ix]),
+			      jj));
 	}
 
-      /* Prepare array of parameter types.  */
-      parm_types = (struct type **) 
-	xmalloc (nparms * (sizeof (struct type *)));
-      for (jj = 0; jj < nparms; jj++)
-	parm_types[jj] = (fns_ptr != NULL
-			  ? (TYPE_FN_FIELD_ARGS (fns_ptr, ix)[jj].type)
-			  : TYPE_FIELD_TYPE (SYMBOL_TYPE (oload_syms[ix]), 
-					     jj));
-
       /* Compare parameter types to supplied argument types.  Skip
          THIS for static methods.  */
       bv = rank_function (parm_types, nparms, 
@@ -2927,10 +3266,14 @@ find_oload_champ (struct value **args, int nargs,
       xfree (parm_types);
       if (overload_debug)
 	{
-	  if (fns_ptr)
+	  if (fns_ptr != NULL)
 	    fprintf_filtered (gdb_stderr,
 			      "Overloaded method instance %s, # of parms %d\n",
 			      fns_ptr[ix].physname, nparms);
+	  else if (dm_worker_vec != NULL)
+	    fprintf_filtered (gdb_stderr,
+			      "Debug method worker, # of parms %d\n",
+			      nparms);
 	  else
 	    fprintf_filtered (gdb_stderr,
 			      "Overloaded function instance "
diff --git a/gdb/value.h b/gdb/value.h
index 9425a0e..d37e7f9 100644
--- a/gdb/value.h
+++ b/gdb/value.h
@@ -31,6 +31,7 @@ struct type;
 struct ui_file;
 struct language_defn;
 struct value_print_options;
+struct debug_method_worker;
 
 /* The structure which defines the type of a value.  It should never
    be possible for a program lval value to survive over a call to the
@@ -690,6 +691,7 @@ extern int find_overload_match (struct value **args, int nargs,
 				enum oload_search_type method,
 				struct value **objp, struct symbol *fsym,
 				struct value **valp, struct symbol **symp,
+				struct debug_method_worker **dm_worker,
 				int *staticp, const int no_adl);
 
 extern struct value *value_field (struct value *arg1, int fieldno);

Re: [PATCH v14 3/4] Add support for lookup, overload resolution and invocation of C++ debug methods

[Doug Evans]

Siva Chandra writes:
 > Attached is the v14 of this part of the patch series. This version
 > addresses a concern raised by Doug during his v12/v13 review.

Hi.

I've been playing with the patch set a bit.

Another issue I have is this bit of code in, e.g., value_x_binop:

  value_user_defined_op (&arg1, argvec + 1, tstr, &static_memfuncp, 2,
			 &argvec[0], &dm_worker);

  if (argvec[0])
    {
      if (static_memfuncp)
	{
	  argvec[1] = argvec[0];
	  argvec++;
	}
      if (noside == EVAL_AVOID_SIDE_EFFECTS)
	{
	  struct type *return_type;

	  return_type
	    = TYPE_TARGET_TYPE (check_typedef (value_type (argvec[0])));
	  return value_zero (return_type, VALUE_LVAL (arg1));
	}
      return call_function_by_hand (argvec[0], 2 - static_memfuncp,
				    argvec + 1);
    }
  if (dm_worker != NULL)
    {
      struct cleanup *dm_worker_cleanup = make_cleanup (xfree, dm_worker);
      struct value *ret_val = invoke_debug_method (dm_worker, arg1, &arg2, 1);

      do_cleanups (dm_worker_cleanup);
      return ret_val;
    }

Does this mean we call the debug method (assuming it wins) for the
case of noside == EVAL_AVOID_SIDE_EFFECTS?

Here and elsewhere there's logic that the debug method code is not
being included in, and it makes me want to do things differently.

Fortunately, I think (though I haven't implemented it) it won't be hard.
Just like we have TYPE_CODE_INTERNAL_FUNCTION we could also have
TYPE_CODE_EXTERNAL_FUNCTION, and use that to make debug methods less
special case.
clone_debug_method_worker could return a struct value that is a
TYPE_CODE_EXTERNAL_FUNCTION, and then just before the call to
call_function_by_hand that would happen for normal methods,
we'd have a check for TYPE_CODE_EXTERNAL_FUNCTION and call
call_debug_method instead.

This would be akin to this code in eval.c:

      if (TYPE_CODE (value_type (argvec[0])) == TYPE_CODE_INTERNAL_FUNCTION)
	return call_internal_function (exp->gdbarch, exp->language_defn,
				       argvec[0], nargs, argvec + 1);

      return call_function_by_hand (argvec[0], nargs, argvec + 1);

[I know the function to do this is currently named invoke_debug_method
but Consistency Is Good makes me want to have:
call_function_by_hand, call_internal_function, and call_debug_method
instead of:
call_function_by_hand, call_internal_function, and invoke_debug_method.]

This might(!) then let us remove the dm_worker arg to find_overload_match.
If a debug method is found *valp is a TYPE_CODE_EXTERNAL_FUNCTION
instead of a TYPE_CODE_FUNC.

Thoughts?

Re: [PATCH v14 3/4] Add support for lookup, overload resolution and invocation of C++ debug methods

[Siva Chandra]

On Mon, Apr 21, 2014 at 4:56 PM, Doug Evans <dje@google.com> wrote:
> Another issue I have is this bit of code in, e.g., value_x_binop:
>
>   value_user_defined_op (&arg1, argvec + 1, tstr, &static_memfuncp, 2,
>                          &argvec[0], &dm_worker);
>
>   if (argvec[0])
>     {
>       if (static_memfuncp)
>         {
>           argvec[1] = argvec[0];
>           argvec++;
>         }
>       if (noside == EVAL_AVOID_SIDE_EFFECTS)
>         {
>           struct type *return_type;
>
>           return_type
>             = TYPE_TARGET_TYPE (check_typedef (value_type (argvec[0])));
>           return value_zero (return_type, VALUE_LVAL (arg1));
>         }
>       return call_function_by_hand (argvec[0], 2 - static_memfuncp,
>                                     argvec + 1);
>     }
>   if (dm_worker != NULL)
>     {
>       struct cleanup *dm_worker_cleanup = make_cleanup (xfree, dm_worker);
>       struct value *ret_val = invoke_debug_method (dm_worker, arg1, &arg2, 1);
>
>       do_cleanups (dm_worker_cleanup);
>       return ret_val;
>     }
>
> Does this mean we call the debug method (assuming it wins) for the
> case of noside == EVAL_AVOID_SIDE_EFFECTS?

To be frank, I never understood why this option exists at all. If side
effects (like even reading target memory) are not desired, why
evaluate expressions at all? And, fwiw, there is a piece of code in
find_oload_match which looks up the most derived type of an object
which involves reading the vtable much before all this.

> Here and elsewhere there's logic that the debug method code is not
> being included in, and it makes me want to do things differently.

Can you point me to an example different from EVAL_AVOID_SIDE_EFFECTS?

> Fortunately, I think (though I haven't implemented it) it won't be hard.
> Just like we have TYPE_CODE_INTERNAL_FUNCTION we could also have
> TYPE_CODE_EXTERNAL_FUNCTION, and use that to make debug methods less
> special case.
> clone_debug_method_worker could return a struct value that is a
> TYPE_CODE_EXTERNAL_FUNCTION, and then just before the call to
> call_function_by_hand that would happen for normal methods,
> we'd have a check for TYPE_CODE_EXTERNAL_FUNCTION and call
> call_debug_method instead.

I am putting down my notes and questions.

1. I think TYPE_CODE_INTERNAL_FUNCTIONs can also be implemented in
extension languages. So, what makes them internal and different from
TYPE_CODE_EXTERNAL_FUNCTION? I have tried to convince myself that the
name "INTERNAL" exists because of historical reasons and does not mean
anything today. Hence, is TYPE_CODE_EXTERNAL_FUNCTION a good name? Am
I missing something?

2.If debug methods are to be represented by a value (say, of type
TYPE_CODE_DEBUG_METHOD), then a new value should be created for every
debug method invocation. I do not know if this is good or bad, and I
do not know if this happens for internal functions as well. A
fundamental difference (atleast in my mind) is that internal functions
have a name by definition (at least currently) and are invoked by that
name. Debug methods do not have a name by definition that they are
invoked with. In fact, debug methods have different invocation names
based on the context (ie. template arguments).

3. Extending debug methods to functions (if that is what you meant by
making debug methods less special case); I think this would lead to a
lot of confusion between convenience functions, debug methods and
"debug functions". Best would be to support only two mutually
exclusive features "Debug Methods" and "Convenience Functions" with
latter extended to have the ability to replace existing source
functions. This is my personal opinion which I am ready to change if
you prescribe a specific different way.

4. What benefit, apart from lesser number of arguments to
find_oload_match, do you see? I guess this is related to the above
question on what else is the debug methods logic currently missing.

Thanks,
Siva Chandra

Re: [PATCH v14 3/4] Add support for lookup, overload resolution and invocation of C++ debug methods

[Doug Evans]

On Tue, Apr 22, 2014 at 1:46 PM, Siva Chandra <sivachandra@google.com> wrote:
> On Mon, Apr 21, 2014 at 4:56 PM, Doug Evans <dje@google.com> wrote:
>> Another issue I have is this bit of code in, e.g., value_x_binop:
>>
>>   value_user_defined_op (&arg1, argvec + 1, tstr, &static_memfuncp, 2,
>>                          &argvec[0], &dm_worker);
>>
>>   if (argvec[0])
>>     {
>>       if (static_memfuncp)
>>         {
>>           argvec[1] = argvec[0];
>>           argvec++;
>>         }
>>       if (noside == EVAL_AVOID_SIDE_EFFECTS)
>>         {
>>           struct type *return_type;
>>
>>           return_type
>>             = TYPE_TARGET_TYPE (check_typedef (value_type (argvec[0])));
>>           return value_zero (return_type, VALUE_LVAL (arg1));
>>         }
>>       return call_function_by_hand (argvec[0], 2 - static_memfuncp,
>>                                     argvec + 1);
>>     }
>>   if (dm_worker != NULL)
>>     {
>>       struct cleanup *dm_worker_cleanup = make_cleanup (xfree, dm_worker);
>>       struct value *ret_val = invoke_debug_method (dm_worker, arg1, &arg2, 1);
>>
>>       do_cleanups (dm_worker_cleanup);
>>       return ret_val;
>>     }
>>
>> Does this mean we call the debug method (assuming it wins) for the
>> case of noside == EVAL_AVOID_SIDE_EFFECTS?
>
> To be frank, I never understood why this option exists at all. If side
> effects (like even reading target memory) are not desired, why
> evaluate expressions at all? And, fwiw, there is a piece of code in
> find_oload_match which looks up the most derived type of an object
> which involves reading the vtable much before all this.

A good question we need to answer is: What happens if a user does
"ptype" on an expression involving a debug method?
[ptype uses evaluate_type which is an example of using EVAL_AVOID_SIDE_EFFECTS]
A user should be able to do that and have confidence that inferior
state will not be modified.
Reading the vtable is ok, but if the debug method is implementing,
say, some kind of iterator then modifying inferior state would not be
ok.

I tried "ptype a1 + a2" from your py-debugmethods.exp testcase, and
the debug method does get invoked.
Is that ok?  In many cases probably, but it doesn't seem likely to be
ok in the general case.

How to achieve that is another good question.
[Ideally we wouldn't impose EVAL_AVOID_SIDE_EFFECTS on debug method writers.]
I have been assuming that debug methods could fit in with the existing
logic and the solution would just fall out.
That needs to be verified of course. :-)

>> Here and elsewhere there's logic that the debug method code is not
>> being included in, and it makes me want to do things differently.
>
> Can you point me to an example different from EVAL_AVOID_SIDE_EFFECTS?

I was thinking of the invoke_debug_method call in evaluate_subexp_standard.
There's a lot of code for case OP_FUNCALL, and the call to
call_function_by_hand appears at the very bottom of the case.
IWBN if the actual call to the debug method was done there too.

>> Fortunately, I think (though I haven't implemented it) it won't be hard.
>> Just like we have TYPE_CODE_INTERNAL_FUNCTION we could also have
>> TYPE_CODE_EXTERNAL_FUNCTION, and use that to make debug methods less
>> special case.
>> clone_debug_method_worker could return a struct value that is a
>> TYPE_CODE_EXTERNAL_FUNCTION, and then just before the call to
>> call_function_by_hand that would happen for normal methods,
>> we'd have a check for TYPE_CODE_EXTERNAL_FUNCTION and call
>> call_debug_method instead.
>
> I am putting down my notes and questions.
>
> 1. I think TYPE_CODE_INTERNAL_FUNCTIONs can also be implemented in
> extension languages. So, what makes them internal and different from
> TYPE_CODE_EXTERNAL_FUNCTION? I have tried to convince myself that the
> name "INTERNAL" exists because of historical reasons and does not mean
> anything today. Hence, is TYPE_CODE_EXTERNAL_FUNCTION a good name? Am
> I missing something?

The names are a bit confusing alright.
I left TYPE_CODE_INTERNAL_FUNCTION alone because it is used for $foo(bar).
Convenience functions have their own syntax.
Maybe we *could* morph TYPE_CODE_INTERNAL_FUNCTION into something more
general purpose, I'm not sure.

> 2.If debug methods are to be represented by a value (say, of type
> TYPE_CODE_DEBUG_METHOD), then a new value should be created for every
> debug method invocation. I do not know if this is good or bad, and I
> do not know if this happens for internal functions as well. A
> fundamental difference (atleast in my mind) is that internal functions
> have a name by definition (at least currently) and are invoked by that
> name. Debug methods do not have a name by definition that they are
> invoked with. In fact, debug methods have different invocation names
> based on the context (ie. template arguments).

Yeah.  Though once we have support for static methods will they still
be "debug methods"?
I'd like to avoid pinning down the term "debug methods" to a specific
use-case (c++ non-static methods).
[assuming that is the name we're sticking with for this feature]

> 3. Extending debug methods to functions (if that is what you meant by
> making debug methods less special case); I think this would lead to a
> lot of confusion between convenience functions, debug methods and
> "debug functions". Best would be to support only two mutually
> exclusive features "Debug Methods" and "Convenience Functions" with
> latter extended to have the ability to replace existing source
> functions. This is my personal opinion which I am ready to change if
> you prescribe a specific different way.

It's mostly just a matter of naming, but the term "Convenience
Functions" already has a specific connotation that implies a leading $
in the name.
Extending convenience functions to foo() in addition to $foo() could
also inflict confusion.
It would be odd, I think, to the user to have non-static methods be
"debug methods" and static methods be "convenience functions".
I don't have a strong opinion though.
IWBN to hear from others in the community but we may be on our own on this one.

btw, "making debug methods less special case" was more aimed at the
implementation (IOW treating them internally no different than other
functions: TYPE_CODE_FUNC and TYPE_CODE_INTERNAL_FUNCTION).

> 4. What benefit, apart from lesser number of arguments to
> find_oload_match, do you see? I guess this is related to the above
> question on what else is the debug methods logic currently missing.

To the person reading gdb code, debug methods are conceptually no
different than TYPE_CODE_INTERNAL_FUNCTION when it comes to actually
invoking them.  Yeah, they're looked up differently, but once they're
looked up they're just some "external" code that gdb will invoke to
evaluate an expression the user entered.  Thus my mental model of how
I expect to see debug methods implemented in the code should match to
the extent possible and reasonable how I see
TYPE_CODE_INTERNAL_FUNCTION implemented.  Where it doesn't it means
more mental effort has to go in to understand and maintain them, and
more state to have to keep in cache while working with the code.

Re: [PATCH v14 3/4] Add support for lookup, overload resolution and invocation of C++ debug methods

[Siva Chandra]

On Wed, Apr 23, 2014 at 1:06 PM, Doug Evans <dje@google.com> wrote:
> A good question we need to answer is: What happens if a user does
> "ptype" on an expression involving a debug method?
> [ptype uses evaluate_type which is an example of using EVAL_AVOID_SIDE_EFFECTS]
> A user should be able to do that and have confidence that inferior
> state will not be modified.
> Reading the vtable is ok, but if the debug method is implementing,
> say, some kind of iterator then modifying inferior state would not be
> ok.
>
> I tried "ptype a1 + a2" from your py-debugmethods.exp testcase, and
> the debug method does get invoked.
> Is that ok?  In many cases probably, but it doesn't seem likely to be
> ok in the general case.

The example answers it! As a user I never used ptype on expressions,
and have never seen one do it. The only doc about
EVAL_AVOID_SIDE_EFFECTS I have is the comment in expression.h. I think
we should add there that one use of this option is when ptype-ing on
expressions. I can send a comment patch if you feel its meaningful.

So then, I will take your suggestion about encapsulating debug methods
in a value and send a new version. The rest of your comments will be
automatically taken care of I think. The name, I will go with
"xmethods" and TYPE_CODE_XMETHOD if that is OK.

Thanks,
Siva Chandra

Re: [PATCH v14 3/4] Add support for lookup, overload resolution and invocation of C++ debug methods

[Doug Evans]

On Wed, Apr 23, 2014 at 2:40 PM, Siva Chandra <sivachandra@google.com> wrote:
> So then, I will take your suggestion about encapsulating debug methods
> in a value and send a new version. The rest of your comments will be
> automatically taken care of I think. The name, I will go with
> "xmethods" and TYPE_CODE_XMETHOD if that is OK.

"works for me"