Re: [patch 2/2] Fix overload resolution of int* vs void*

[sami wagiaalla <swagiaal@redhat.com>]

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

On 10/12/2010 04:40 PM, Tom Tromey wrote:
>>>>>> "Sami" == sami wagiaalla<swagiaal@redhat.com>  writes:
>
> Sami>  I understood that one could convert 'Class ***' to 'BaseClass ***',
> Sami>  but it turns out that is in correct. I should not have assumed that
> Sami>  without testing, and there is nothing in the spec that should have
> Sami>  made me think so. Since that is the case, the calling function can
> Sami>  just deference the pointers.
>
> Is there a test for this?  There should be.  (I didn't check.)
>

I added one. The test attempts to make the conversion and fails as expected.

> Tom>  I am curious why you didn't just give POINTER_CONVERSION_BADNESS a new
> Tom>  value and instead introduced BASE_PTR_CONVERSION_BADNESS.
>
> Tom>  But then, I also don't understand the existing code that returns
> Tom>  POINTER_CONVERSION_BADNESS...
>
> Sami>  base pointer conversion (BASE_PTR_CONVERSION_BADNESS) is meant to be a
> Sami>  slightly better option than generic (POINTER_CONVERSION_BADNESS)
>
> I still don't understand.
>
> I guess POINTER_CONVERSION_BADNESS is used for really bogus operations,
> like converting an int to a pointer.  It seems to be a gdb extension.
>
> I'm not sure this is worth supporting.
>

A pointer can be legally converted to a base pointer a void pointer or a 
boolean. These are ranked in that order. All other conversions are 
indeed bogus. To rank these we use:

BASE_PTR_CONVERSION_BADNESS 1
VOID_PTR_CONVERSION_BADNESS 2
POINTER_CONVERSION_BADNESS  2

Note: Converting a pointer to a matching array is considered an exact 
match (rank 0).

Having typed this out, I think POINTER_CONVERSION_BADNESS should be 
renamed to BOOL_PTR_CONVERSION_BADNESS and given a rank of 3. And all 
other conversions outlawed. I'll do this in and test it another patch. 
This patch really only deals with the first clauses of the nested switch 
statements.

I totally agree on not supporting the other conversions as extensions.

> Tom>  Also, why specifically is_public_ancestor and not is_ancestor?
>
> Sami>  You can convert a pointer to B to a pointer to A only if A is an
> Sami>  accessible ancestor of B.
>
> GDB generally ignores access protection.  It seems like it ought to here
> as well.
>

Hmm, I am worried that this will make overload resolution more 
complicated, and lead to incorrect resolution. I have no objection to 
calling the inaccessible base function if the user casts the the type:
B b;
((A)b).foo();
or
foo((A)b))
but in overload resolution I think we should stick to the standard at 
least until we are more confident in our implementation.

> Sami>  +  /* If a and b are both pointers types or both reference types then
> Sami>  +     they are equal of the same type iff the objects they refer to are
> Sami>  +     of the same type.  */
> Sami>  +  if (TYPE_CODE (a) == TYPE_CODE_PTR
> Sami>  +      || TYPE_CODE (a) == TYPE_CODE_REF)
> Sami>  +    return types_equal (TYPE_TARGET_TYPE (a),
> Sami>  +                        TYPE_TARGET_TYPE (b));
>
> This recursive call seems a little odd.
> The existence of a check for the "Class ***" case would help prove that
> it is ok, though.
>

I added the 'Class ***' check as requested, and I should point out that 
there is also foo2_2, foo2_3, and foo2_4 to test this spot of the code.

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

Fixed void* vs int* overload issue (PR C++/10343).

2010-10-12  Sami Wagiaalla  <swagiaal@redhat.com>

	* gdbtypes.h: Create BASE_PTR_CONVERSION_BADNESS.
	* gdbtypes.c (rank_one_type): Move type comparison code out of here
	to...
	(types_equal): ...here. And changed it as follows:
	Outside of typedefs type must be of the same TYPE_CODE.
	When compairing two pointers or references they are equal if their
	targets are equal.
	Correct pointer conversions.

2010-10-12  Sami Wagiaalla  <swagiaal@redhat.com>

	* gdb.cp/converts.cc: New test program.
	* gdb.cp/converts.exp: New test.
	* gdb.cp/overload.exp: Added test for void* vs int*.
	* gdb.cp/overload.exp: Ditto.
	* gdb.cp/oranking.exp: Removed related kfail.
	* gdb.cp/virtfunc2.cc (Obj2): Change inheritance to public.
	Add calls to the function calls being tested.
	* gdb.cp/derivation.cc (G): Changed inheritance to public.
	Add calls to the function calls being tested.

diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index 9c3152d..5af7491 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -2104,6 +2104,56 @@ integer_types_same_name_p (const char *first, const char *second)
   return 1;
 }
 
+/* Compares type A to type B returns 1 if the represent the same type
+   0 otherwise.  */
+
+static int
+types_equal (struct type *a, struct type *b)
+{
+  /* Identical type pointers.  */
+  /* However, this still doesn't catch all cases of same type for b
+     and a.  The reason is that builtin types are different from
+     the same ones constructed from the object.  */
+  if (a == b)
+    return 1;
+
+  /* Resolve typedefs */
+  if (TYPE_CODE (a) == TYPE_CODE_TYPEDEF)
+    a = check_typedef (a);
+  if (TYPE_CODE (b) == TYPE_CODE_TYPEDEF)
+    b = check_typedef (b);
+
+  /* If after resolving typedefs a and b are not of the same type
+     code then they are not equal.  */
+  if (TYPE_CODE (a) != TYPE_CODE (b))
+    return 0;
+
+  /* If a and b are both pointers types or both reference types then
+     they are equal of the same type iff the objects they refer to are
+     of the same type.  */
+  if (TYPE_CODE (a) == TYPE_CODE_PTR
+      || TYPE_CODE (a) == TYPE_CODE_REF)
+    return types_equal (TYPE_TARGET_TYPE (a),
+                        TYPE_TARGET_TYPE (b));
+
+  /*
+     Well, damnit, if the names are exactly the same, I'll say they
+     are exactly the same.  This happens when we generate method
+     stubs.  The types won't point to the same address, but they
+     really are the same.
+  */
+
+  if (TYPE_NAME (a) && TYPE_NAME (b)
+      && strcmp (TYPE_NAME (a), TYPE_NAME (b)) == 0)
+    return 1;
+
+  /* Check if identical after resolving typedefs.  */
+  if (a == b)
+    return 1;
+
+  return 0;
+}
+
 /* Compare one type (PARM) for compatibility with another (ARG).
  * PARM is intended to be the parameter type of a function; and
  * ARG is the supplied argument's type.  This function tests if
@@ -2117,11 +2167,8 @@ integer_types_same_name_p (const char *first, const char *second)
 int
 rank_one_type (struct type *parm, struct type *arg)
 {
-  /* Identical type pointers.  */
-  /* However, this still doesn't catch all cases of same type for arg
-     and param.  The reason is that builtin types are different from
-     the same ones constructed from the object.  */
-  if (parm == arg)
+
+  if (types_equal (parm, arg))
     return 0;
 
   /* Resolve typedefs */
@@ -2130,21 +2177,6 @@ rank_one_type (struct type *parm, struct type *arg)
   if (TYPE_CODE (arg) == TYPE_CODE_TYPEDEF)
     arg = check_typedef (arg);
 
-  /*
-     Well, damnit, if the names are exactly the same, I'll say they
-     are exactly the same.  This happens when we generate method
-     stubs.  The types won't point to the same address, but they
-     really are the same.
-  */
-
-  if (TYPE_NAME (parm) && TYPE_NAME (arg) 
-      && !strcmp (TYPE_NAME (parm), TYPE_NAME (arg)))
-    return 0;
-
-  /* Check if identical after resolving typedefs.  */
-  if (parm == arg)
-    return 0;
-
   /* See through references, since we can almost make non-references
      references.  */
   if (TYPE_CODE (arg) == TYPE_CODE_REF)
@@ -2168,15 +2200,23 @@ rank_one_type (struct type *parm, struct type *arg)
       switch (TYPE_CODE (arg))
 	{
 	case TYPE_CODE_PTR:
-	  if (TYPE_CODE (TYPE_TARGET_TYPE (parm)) == TYPE_CODE_VOID
-	      && TYPE_CODE (TYPE_TARGET_TYPE (arg)) != TYPE_CODE_VOID)
+
+	  /* Allowed pointer conversions are:
+	     (a) pointer to void-pointer conversion.  */
+	  if (TYPE_CODE (TYPE_TARGET_TYPE (parm)) == TYPE_CODE_VOID)
 	    return VOID_PTR_CONVERSION_BADNESS;
-	  else
-	    return rank_one_type (TYPE_TARGET_TYPE (parm), 
-				  TYPE_TARGET_TYPE (arg));
+
+	  /* (b) pointer to ancestor-pointer conversion.  */
+	  if (is_public_ancestor (TYPE_TARGET_TYPE (parm),
+	                          TYPE_TARGET_TYPE (arg)))
+	    return BASE_PTR_CONVERSION_BADNESS;
+
+	  return INCOMPATIBLE_TYPE_BADNESS;
 	case TYPE_CODE_ARRAY:
-	  return rank_one_type (TYPE_TARGET_TYPE (parm), 
-				TYPE_TARGET_TYPE (arg));
+	  if (types_equal (TYPE_TARGET_TYPE (parm),
+	                   TYPE_TARGET_TYPE (arg)))
+	    return 0;
+	  return INCOMPATIBLE_TYPE_BADNESS;
 	case TYPE_CODE_FUNC:
 	  return rank_one_type (TYPE_TARGET_TYPE (parm), arg);
 	case TYPE_CODE_INT:
diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
index dba7284..62ade5f 100644
--- a/gdb/gdbtypes.h
+++ b/gdb/gdbtypes.h
@@ -1400,6 +1400,9 @@ extern int is_unique_ancestor (struct type *, struct value *);
 #define INTEGER_PROMOTION_BADNESS      1
 /* Badness of floating promotion */
 #define FLOAT_PROMOTION_BADNESS        1
+/* Badness of converting a derived class pointer
+   to a base class pointer.  */
+#define BASE_PTR_CONVERSION_BADNESS    1
 /* Badness of integral conversion */
 #define INTEGER_CONVERSION_BADNESS     2
 /* Badness of floating conversion */
diff --git a/gdb/testsuite/gdb.cp/converts.cc b/gdb/testsuite/gdb.cp/converts.cc
new file mode 100644
index 0000000..17d1476
--- /dev/null
+++ b/gdb/testsuite/gdb.cp/converts.cc
@@ -0,0 +1,54 @@
+class A {};
+class B : public A {};
+
+typedef A TA1;
+typedef A TA2;
+typedef TA2 TA3;
+
+int foo0_1 (TA1)  { return 1; }
+int foo0_2 (TA3)  { return 2; }
+int foo0_3 (A***) { return 3; }
+
+int foo1_1 (char *) {return 11;}
+int foo1_2 (char[]) {return 12;}
+int foo1_3 (int*)   {return 13;}
+int foo1_4 (A*)     {return 14;}
+int foo1_5 (void*)  {return 15;}
+int foo1_6 (void**) {return 15;}
+
+int foo2_1 (char**  )  {return 21;}
+int foo2_2 (char[][1]) {return 22;}
+int foo2_3 (char *[])  {return 23;}
+int foo2_4 (int  *[])  {return 24;}
+
+int main()
+{
+
+  TA2 ta;      // typedef to..
+  foo0_1 (ta); // ..another typedef
+  foo0_2 (ta); // ..typedef of a typedef
+
+  B*** bppp;    // Pointer-to-pointer-to-pointer-to-derived..
+//foo0_3(bppp); // Pointer-to-pointer-to-pointer base.
+  foo0_3((A***)bppp); // to ensure that the function is emitted.
+
+  char *a;             // pointer to..
+  B *bp;
+  foo1_1 (a);          // ..pointer
+  foo1_2 (a);          // ..array
+  foo1_3 ((int*)a);    // ..pointer of wrong type
+  foo1_3 ((int*)bp);   // ..pointer of wrong type
+  foo1_4 (bp);         // ..ancestor pointer
+  foo1_4 (bp);         // ..ancestor pointer
+  foo1_5 (bp);         // ..void pointer
+  foo1_6 ((void**)bp); // ..void pointer
+
+  char **b;          // pointer pointer to..
+  char ba[1][1];
+  foo1_5 (b);        // ..void pointer
+  foo2_1 (b);        // ..pointer pointer
+  foo2_2 (ba);       // ..array of arrays
+  foo2_3 (b);        // ..array of pointers
+  foo2_4 ((int**)b); // ..array of wrong pointers
+  return 0;          // end of main
+}
diff --git a/gdb/testsuite/gdb.cp/converts.exp b/gdb/testsuite/gdb.cp/converts.exp
new file mode 100644
index 0000000..d22998f
--- /dev/null
+++ b/gdb/testsuite/gdb.cp/converts.exp
@@ -0,0 +1,47 @@
+# Copyright 2008 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+set testfile converts
+set srcfile ${testfile}.cc
+if { [prepare_for_testing ${testfile}.exp ${testfile} ${srcfile} {debug c++}] } {
+     return -1
+}
+
+############################################
+
+if ![runto_main] then {
+    perror "couldn't run to breakpoint main"
+    continue
+}
+
+gdb_breakpoint [gdb_get_line_number "end of main"]
+gdb_continue_to_breakpoint "end of main"
+
+gdb_test "p foo0_1 (ta)"   "= 1"
+gdb_test "p foo0_2 (ta)"   "= 2"
+gdb_test "p foo0_3 (bppp)" "Cannot resolve function.*"
+
+gdb_test "p foo1_1 (a)"  "= 11"
+gdb_test "p foo1_2 (a)"  "= 12"
+gdb_test "p foo1_3 (a)"  "Cannot resolve function.*"
+gdb_test "p foo1_3 (bp)" "Cannot resolve function.*"
+gdb_test "p foo1_4 (bp)" "= 14"
+gdb_test "p foo1_5 (bp)" "= 15"
+
+gdb_test "p foo1_5 (b)" "= 15"
+gdb_test "p foo2_1 (b)" "= 21"
+gdb_test "p foo2_2 (b)" "Cannot resolve function.*"
+gdb_test "p foo2_3 (b)" "= 23"
+gdb_test "p foo2_4 (b)" "Cannot resolve function.*"
diff --git a/gdb/testsuite/gdb.cp/derivation.cc b/gdb/testsuite/gdb.cp/derivation.cc
index f6d42e7..02999c1 100644
--- a/gdb/testsuite/gdb.cp/derivation.cc
+++ b/gdb/testsuite/gdb.cp/derivation.cc
@@ -96,7 +96,7 @@ public:
     
 };
 
-class G : private A, public B, protected C {
+class G : public A, public B, public C {
 public:
     int g;
     int gg;
@@ -207,7 +207,10 @@ int main(void)
     E e_instance;
     F f_instance;
     G g_instance;
-    
+
+    g_instance.afoo();
+    g_instance.cfoo();
+
     #ifdef usestubs
        set_debug_traps();
        breakpoint();
diff --git a/gdb/testsuite/gdb.cp/oranking.exp b/gdb/testsuite/gdb.cp/oranking.exp
index abe8252..f06933a 100644
--- a/gdb/testsuite/gdb.cp/oranking.exp
+++ b/gdb/testsuite/gdb.cp/oranking.exp
@@ -56,7 +56,6 @@ setup_kfail "gdb/12098" *-*-*
 gdb_test "p foo5(c)" "26"
 
 gdb_test "p test6()"  "28"
-setup_kfail "gdb/10343" *-*-*
 gdb_test "p foo6(bp)" "28"
 
 gdb_test "p test7()"  "210"
diff --git a/gdb/testsuite/gdb.cp/overload.cc b/gdb/testsuite/gdb.cp/overload.cc
index 78fae14..dc117fb 100644
--- a/gdb/testsuite/gdb.cp/overload.cc
+++ b/gdb/testsuite/gdb.cp/overload.cc
@@ -24,6 +24,9 @@ int overload1arg (unsigned long);
 int overload1arg (float);
 int overload1arg (double);
 
+int overload1arg (int*);
+int overload1arg (void*);
+
 int overloadfnarg (void);
 int overloadfnarg (int);
 int overloadfnarg (int, int (*) (int));
@@ -99,6 +102,8 @@ int main ()
     unsigned long arg10 =10;
     float arg11 =100.0;
     double arg12 = 200.0;
+    int arg13 = 200.0;
+    char arg14 = 'a';
 
     char *str = (char *) "A";
     foo foo_instance1(111);
@@ -150,6 +155,8 @@ int foo::overload1arg (long arg)            { arg = 0; return 9;}
 int foo::overload1arg (unsigned long arg)   { arg = 0; return 10;}
 int foo::overload1arg (float arg)           { arg = 0; return 11;}
 int foo::overload1arg (double arg)          { arg = 0; return 12;}
+int foo::overload1arg (int* arg)            { arg = 0; return 13;}
+int foo::overload1arg (void* arg)           { arg = 0; return 14;}
 
 /* Test to see that we can explicitly request overloaded functions
    with function pointers in the prototype. */
diff --git a/gdb/testsuite/gdb.cp/overload.exp b/gdb/testsuite/gdb.cp/overload.exp
index f05cc23..25aeb07 100644
--- a/gdb/testsuite/gdb.cp/overload.exp
+++ b/gdb/testsuite/gdb.cp/overload.exp
@@ -80,6 +80,8 @@ set re_methods	"${re_methods}${ws}int overload1arg\\(long( int)?\\);"
 set re_methods	"${re_methods}${ws}int overload1arg\\((unsigned long|long unsigned)( int)?\\);"
 set re_methods	"${re_methods}${ws}int overload1arg\\(float\\);"
 set re_methods	"${re_methods}${ws}int overload1arg\\(double\\);"
+set re_methods	"${re_methods}${ws}int overload1arg\\(int \\*\\);"
+set re_methods	"${re_methods}${ws}int overload1arg\\(void \\*\\);"
 set re_methods	"${re_methods}${ws}int overloadfnarg\\((void|)\\);"
 set re_methods	"${re_methods}${ws}int overloadfnarg\\(int\\);"
 set re_methods	"${re_methods}${ws}int overloadfnarg\\(int, int ?\\(\\*\\) ?\\(int\\)\\);"
@@ -256,6 +258,14 @@ gdb_test "print foo_instance1.overload1arg((double)arg12)" \
     "\\$\[0-9\]+ = 12" \
     "print call overloaded func double arg"
 
+gdb_test "print foo_instance1.overload1arg(&arg13)" \
+    "\\$\[0-9\]+ = 13" \
+    "print call overloaded func int\\* arg"
+
+gdb_test "print foo_instance1.overload1arg(&arg14)" \
+    "\\$\[0-9\]+ = 14" \
+    "print call overloaded func char\\* arg"
+
 # ---
 
 # List overloaded functions.
diff --git a/gdb/testsuite/gdb.cp/virtfunc2.cc b/gdb/testsuite/gdb.cp/virtfunc2.cc
index 90f3eda..666a495 100644
--- a/gdb/testsuite/gdb.cp/virtfunc2.cc
+++ b/gdb/testsuite/gdb.cp/virtfunc2.cc
@@ -27,7 +27,7 @@ public:
   virtual int do_print() { return 123456; }
 };
 
-class Obj2 : Obj,  virtual public interface
+class Obj2 : public Obj,  virtual public interface
 {
   virtual int do_print2() { return 654321; }
 };
@@ -35,5 +35,7 @@ class Obj2 : Obj,  virtual public interface
 int main(int argc, char** argv) {
   Obj o;
   Obj2 o2;
+  o2.do_print();
+
   return 0;	// marker 1
 }