From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id BF1E4385801A for ; Thu, 25 Nov 2021 14:13:04 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org BF1E4385801A Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-335-VRQt7F1ROC-IC5TJgTfN5Q-1; Thu, 25 Nov 2021 09:13:00 -0500 X-MC-Unique: VRQt7F1ROC-IC5TJgTfN5Q-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 14E341023F4D; Thu, 25 Nov 2021 14:12:59 +0000 (UTC) Received: from [10.97.116.34] (ovpn-116-34.gru2.redhat.com [10.97.116.34]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 57B91908B; Thu, 25 Nov 2021 14:12:56 +0000 (UTC) Message-ID: <43bcd142-e668-361b-ac4b-a3b40641255f@redhat.com> Date: Thu, 25 Nov 2021 11:12:54 -0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.3.0 Subject: Re: [PATCH 1/1] gdb: Use a typedef's scoped type name to identify local typefs To: Christina Schimpe , gdb-patches@sourceware.org References: <20211125111604.311022-1-christina.schimpe@intel.com> From: Bruno Larsen In-Reply-To: <20211125111604.311022-1-christina.schimpe@intel.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-15.7 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP, WEIRD_PORT autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 25 Nov 2021 14:13:07 -0000 Thanks for looking at this. I only have one question, and a minor comment On 11/25/21 08:16, Christina Schimpe via Gdb-patches wrote: > GDB prints the wrong type for typedefs in case there is another typedef > available for the same raw type (gdb/16040). The reason is that the > current hashmap based substitution mechanism always compares the target > type of a typedef and not its scoped name. > > The original output of GDB for a program like > > ~~~~ > namespace ns > { > typedef double scoped_double; > } > > typedef double global_double; > > class TypedefHolder > { > public: > double a; > ns::scoped_double b; > global_double c; > > private: > typedef double class_double; > class_double d; > > double method1(ns::scoped_double) { return 24.0; } > double method2(global_double) { return 24.0; } > }; > > int main() > { > TypedefHolder th; > return 0; > } > ~~~~ > > is > ~~~~ > > (gdb) b 27 > Breakpoint 1 at 0x1131: file TypedefHolder.cc, line 27. > (gdb) r > Starting program: /tmp/typedefholder > > Breakpoint 1, main () at TypedefHolder.cc:27 > 27 return 0; > (gdb) ptype th > type = class TypedefHolder { > public: > class_double a; > class_double b; > class_double c; > private: > class_double d; > > class_double method1(class_double); > class_double method2(class_double); > > typedef double class_double; > } > ~~~~ > > Basically all attributes of a class which have the raw type "double" are > substituted by "class_double". > > With the patch the output is the following > > ~~~~ > type = class TypedefHolder { > public: > double a; > ns::scoped_double b; > global_double c; > private: > class_double d; > > double method1(ns::scoped_double); > double method2(global_double); > > typedef double class_double; > } > ~~~~ > > Any feedback about this patch is appreciated. > > gdb/ChangeLog: > 2021-11-25 Christina Schimpe > * gdb/typeprint.c: Change typedef comparison value. > > gdb/testsuite/ChangeLog: > 2021-11-25 Christina Schimpe > * gdb.cp/ptype-flags.cc: New test class. > * gdb.cp/ptype-flags.exp: New test. We don't require changelogs anymore, only if the patch is going to gdb-11-branch. > --- > gdb/testsuite/gdb.cp/ptype-flags.cc | 23 ++++++++ > gdb/testsuite/gdb.cp/ptype-flags.exp | 88 ++++++++++++++++++++++------ > gdb/typeprint.c | 6 +- > 3 files changed, 97 insertions(+), 20 deletions(-) > > diff --git a/gdb/testsuite/gdb.cp/ptype-flags.cc b/gdb/testsuite/gdb.cp/ptype-flags.cc > index fc92d3950c..564d272e57 100644 > --- a/gdb/testsuite/gdb.cp/ptype-flags.cc > +++ b/gdb/testsuite/gdb.cp/ptype-flags.cc > @@ -38,7 +38,30 @@ public: > double method(void) { return 23.0; } > }; > > +namespace ns > +{ > + typedef double scoped_double; > +} > + > +typedef double global_double; > + > +class TypedefHolder > +{ > +public: > + double a; > + ns::scoped_double b; > + global_double c; > + > +private: > + typedef double class_double; > + class_double d; > + > + double method1(ns::scoped_double) { return 24.0; } > + double method2(global_double) { return 24.0; } > +}; > + > Holder value; > +TypedefHolder value2; > > int main() > { > diff --git a/gdb/testsuite/gdb.cp/ptype-flags.exp b/gdb/testsuite/gdb.cp/ptype-flags.exp > index c368415793..c41e8f8744 100644 > --- a/gdb/testsuite/gdb.cp/ptype-flags.exp > +++ b/gdb/testsuite/gdb.cp/ptype-flags.exp > @@ -33,7 +33,9 @@ if ![runto_main] then { > gdb_test_no_output "set language c++" "" > gdb_test_no_output "set width 0" "" > > -proc do_check {name {flags ""} {show_typedefs 1} {show_methods 1} {raw 0}} { > +proc do_check_holder \ > + {name {flags ""} {show_typedefs 1} {show_methods 1} {raw 0}} { > + > set contents { > { base "public Base" } > { field public "Simple t;" } > @@ -62,24 +64,76 @@ proc do_check {name {flags ""} {show_typedefs 1} {show_methods 1} {raw 0}} { > "" {} $flags > } > > -do_check "basic test" > -do_check "no methods" "/m" 1 0 > -do_check "no typedefs" "/t" 0 1 > -do_check "no methods or typedefs" "/mt" 0 0 > +proc do_check_typedef_holder \ > + {name {flags ""} {show_typedefs 1} {show_methods 1} {raw 0}} { > + > + set contents { > + { field public "double a;" } > + { field public "ns::scoped_double b;" } > + { field public "global_double c;" } > + } > + > + if {$show_typedefs} { > + lappend contents { typedef private "typedef double class_double;" } > + } > + > + if {$show_methods} { > + lappend contents { method private "double method1(ns::scoped_double);" } > + lappend contents { method private "double method2(global_double);" } > + } > + > + if {$raw} { > + lappend contents { field private "TypedefHolder::class_double d;" } > + } else { > + lappend contents { field private "class_double d;" } > + } > + > + cp_test_ptype_class value2 $name "class" "TypedefHolder" $contents \ > + "" {} $flags > +} > > -do_check "raw" "/r" 1 1 1 > -do_check "raw no methods" "/rm" 1 0 1 > -do_check "raw no typedefs" "/rt" 0 1 1 > -do_check "raw no methods or typedefs" "/rmt" 0 0 1 > +do_check_holder "basic test" > +do_check_holder "no methods" "/m" 1 0 > +do_check_holder "no typedefs" "/t" 0 1 > +do_check_holder "no methods or typedefs" "/mt" 0 0 > +do_check_typedef_holder "typdefs class: basic test" > +do_check_typedef_holder "typdefs class: no methods" "/m" 1 0 > +do_check_typedef_holder "typdefs class: no typedefs" "/t" 0 1 0 > +do_check_typedef_holder "typdefs class:no methods or typedefs" "/mt" 0 0 > + > +do_check_holder "raw" "/r" 1 1 1 > +do_check_holder "raw no methods" "/rm" 1 0 1 > +do_check_holder "raw no typedefs" "/rt" 0 1 1 > +do_check_holder "raw no methods or typedefs" "/rmt" 0 0 1 > +do_check_typedef_holder "typedef class: raw" "/r" 1 1 1 > +do_check_typedef_holder "typedef class: raw no methods" "/rm" 1 0 1 > +do_check_typedef_holder "typedef class: raw no typedefs" "/rt" 0 1 1 > +do_check_typedef_holder "typedef class: raw no methods or typedefs" "/rmt" 0 0 1 > > gdb_test_no_output "set print type methods off" > -do_check "basic test, default methods off" "" 1 0 > -do_check "methods, default methods off" "/M" 1 1 > -do_check "no typedefs, default methods off" "/t" 0 0 > -do_check "methods, no typedefs, default methods off" "/Mt" 0 1 > +do_check_holder "basic test, default methods off" "" 1 0 > +do_check_holder "methods, default methods off" "/M" 1 1 > +do_check_holder "no typedefs, default methods off" "/t" 0 0 > +do_check_holder "methods, no typedefs, default methods off" "/Mt" 0 1 > +do_check_typedef_holder \ > + "typedef class: basic test, default methods off" "" 1 0 > +do_check_typedef_holder \ > + "typedef class: methods, default methods off" "/M" 1 1 > +do_check_typedef_holder \ > + "typedef class: no typedefs, default methods off" "/t" 0 0 > +do_check_typedef_holder \ > + "typedef class: methods, no typedefs, default methods off" "/Mt" 0 1 > > gdb_test_no_output "set print type typedefs off" > -do_check "basic test, default methods+typedefs off" "" 0 0 > -do_check "methods, default methods+typedefs off" "/M" 0 1 > -do_check "typedefs, default methods+typedefs off" "/T" 1 0 > -do_check "methods typedefs, default methods+typedefs off" "/MT" 1 1 > +do_check_holder "basic test, default methods+typedefs off" "" 0 0 > +do_check_holder "methods, default methods+typedefs off" "/M" 0 1 > +do_check_holder "typedefs, default methods+typedefs off" "/T" 1 0 > +do_check_holder "methods typedefs, default methods+typedefs off" "/MT" 1 1 > +do_check_typedef_holder \ > + "typedef class: basic test, default methods+typedefs off" "" 0 0 > +do_check_typedef_holder \ > + "typedef class: methods, default methods+typedefs off" "/M" 0 1 > +do_check_typedef_holder \ > + "typedef class: typedefs, default methods+typedefs off" "/T" 1 0 > +do_check_typedef_holder \ > + "typedef class: methods typedefs, default methods+typedefs off" "/MT" 1 1 > diff --git a/gdb/typeprint.c b/gdb/typeprint.c > index 1312111b60..f1b0ce0a71 100644 > --- a/gdb/typeprint.c > +++ b/gdb/typeprint.c > @@ -201,9 +201,8 @@ static hashval_t > hash_typedef_field (const void *p) > { > const struct decl_field *tf = (const struct decl_field *) p; > - struct type *t = check_typedef (tf->type); > > - return htab_hash_string (TYPE_SAFE_NAME (t)); > + return htab_hash_string (TYPE_SAFE_NAME (tf->type)); > } > > /* An equality function for a typedef field. */ > @@ -214,7 +213,8 @@ eq_typedef_field (const void *a, const void *b) > const struct decl_field *tfa = (const struct decl_field *) a; > const struct decl_field *tfb = (const struct decl_field *) b; > > - return types_equal (tfa->type, tfb->type); > + return (tfa->type->name () && tfb->type->name () > + && (strcmp (tfa->type->name (), tfb->type->name ()) == 0)); > } Why did you change from types_equal to explicitly comparing names? Types_equal already makes this comparison, and if I put it back in the code, your testcase still passes. Am I mising something? > > /* See typeprint.h. */ > -- Cheers! Bruno Larsen