From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 11208 invoked by alias); 12 Oct 2010 20:01:34 -0000 Received: (qmail 11192 invoked by uid 22791); 12 Oct 2010 20:01:31 -0000 X-SWARE-Spam-Status: No, hits=-6.2 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,SPF_HELO_PASS,TW_DF,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 12 Oct 2010 20:01:24 +0000 Received: from int-mx03.intmail.prod.int.phx2.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.16]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id o9CK1KW8022171 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Tue, 12 Oct 2010 16:01:21 -0400 Received: from ns3.rdu.redhat.com (ns3.rdu.redhat.com [10.11.255.199]) by int-mx03.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id o9CK1KOs028952; Tue, 12 Oct 2010 16:01:20 -0400 Received: from [10.15.16.129] (dhcp-10-15-16-129.yyz.redhat.com [10.15.16.129]) by ns3.rdu.redhat.com (8.13.8/8.13.8) with ESMTP id o9CK1HZn019498; Tue, 12 Oct 2010 16:01:18 -0400 Message-ID: <4CB4BE8D.1000001@redhat.com> Date: Tue, 12 Oct 2010 20:01:00 -0000 From: sami wagiaalla User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.8) Gecko/20100806 Fedora/3.1.2-1.fc13 Lightning/1.0b2pre Thunderbird/3.1.2 MIME-Version: 1.0 To: Tom Tromey CC: Eli Zaretskii , gdb-patches@sourceware.org Subject: Re: [patch 2/2] Fix overload resolution of int* vs void* References: <4C7BCD42.9070308@redhat.com> <4CAF6B73.5090800@redhat.com> <8339sgo1jl.fsf@gnu.org> <4CAF889C.1080404@redhat.com> In-Reply-To: Content-Type: multipart/mixed; boundary="------------090509040404060509070204" X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2010-10/txt/msg00202.txt.bz2 This is a multi-part message in MIME format. --------------090509040404060509070204 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Content-length: 3676 On 10/08/2010 06:54 PM, Tom Tromey wrote: >>>>>> "Sami" == sami wagiaalla writes: > > Sami> @@ -1886,6 +1886,14 @@ do_is_ancestor (struct type *base, struct type *dclass, int public) > Sami> if (class_types_same_p (base, dclass)) > Sami> return 1; > > Sami> + if ((TYPE_CODE (base) == TYPE_CODE_PTR > Sami> +&& TYPE_CODE (dclass) == TYPE_CODE_PTR) > Sami> + || (TYPE_CODE (base) == TYPE_CODE_REF > Sami> + && TYPE_CODE (dclass) == TYPE_CODE_REF)) > Sami> + return do_is_ancestor (TYPE_TARGET_TYPE (base), > Sami> + TYPE_TARGET_TYPE (dclass), > Sami> + public); > > This seems strange to me. It ought to only be applicable at the > outermost call. But in that case, I think a wrapper function would be > more appropriate, so that do_is_ancestor has a single purpose. > > Also, is this the right thing for the caller when there are multiple > pointers (like "Class ***")? Or do you intend to only strip a single > pointer? > I understood that one could convert 'Class ***' to 'BaseClass ***', but it turns out that is in correct. I should not have assumed that without testing, and there is nothing in the spec that should have made me think so. Since that is the case, the calling function can just deference the pointers. > Sami> + /* Resolve typedefs */ > Sami> + if (TYPE_CODE (a) == TYPE_CODE_TYPEDEF) > Sami> + a = check_typedef (a); > Sami> + if (TYPE_CODE (b) == TYPE_CODE_TYPEDEF) > Sami> + b = check_typedef (b); > > It is ok to call check_typedef unconditionally, but I see this is just > copied from rank_one_type. > Yeah I wondered about that when I copied the code but could not think of a reason not to do so. I added a test case for the typedef scenario. > Sami> + /* (b) pointer to ancestor-pointer conversion. */ > Sami> + if (is_public_ancestor (parm, arg)) > Sami> + return BASE_PTR_CONVERSION_BADNESS; > > I am curious why you didn't just give POINTER_CONVERSION_BADNESS a new > value and instead introduced BASE_PTR_CONVERSION_BADNESS. > > But then, I also don't understand the existing code that returns > POINTER_CONVERSION_BADNESS... > base pointer conversion (BASE_PTR_CONVERSION_BADNESS) is meant to be a slightly better option than generic (POINTER_CONVERSION_BADNESS) > Also, why specifically is_public_ancestor and not is_ancestor? > You can convert a pointer to B to a pointer to A only if A is an accessible ancestor of B. > I suppose this is an incremental patch -- still not truly correct with > regards to the distance-to-base problem, but an improvement over what > gdb currently does. Yes. I am working on a second patch to solve the distance to base problem. I will post it in reply to my previous attempt at that problem. This is fine, even great, but it is handy for > reviewers if you note that sort of thing in the patch explanation. > Cool, will do in the future. Incidentally, I should mention that is there is also the conversion of pointer to other data types and ranking within them that needs to be covered. > Sami> case TYPE_CODE_ARRAY: > Sami> - return rank_one_type (TYPE_TARGET_TYPE (parm), > Sami> - TYPE_TARGET_TYPE (arg)); > Sami> + if (types_equal (TYPE_TARGET_TYPE (parm), > Sami> + TYPE_TARGET_TYPE (arg))) > Sami> + return 0; > Sami> + return INCOMPATIBLE_TYPE_BADNESS; > > Do the new tests cover this case? I could not immediately tell. > Yes, take a look at foo1_2 foo2_2, and foo2_4. Let me know if the tests need elaborations it is easy to miss cases when you are so close to the code. Revised patch attached. --------------090509040404060509070204 Content-Type: text/x-patch; name="overload_voidp.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="overload_voidp.patch" Content-length: 13277 Fixed void* vs int* overload issue (PR C++/10343). 2010-10-12 Sami Wagiaalla * 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 * 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..435ed13 --- /dev/null +++ b/gdb/testsuite/gdb.cp/converts.cc @@ -0,0 +1,49 @@ +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 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 + + 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..894f1cc --- /dev/null +++ b/gdb/testsuite/gdb.cp/converts.exp @@ -0,0 +1,46 @@ +# 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 . + +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 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 } --------------090509040404060509070204--