Message ID | 4958cf6a-a9ab-9448-0017-a4317d5a09ad@ericsson.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 92521 invoked by alias); 7 Sep 2018 21:26:38 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 92507 invoked by uid 89); 7 Sep 2018 21:26:37 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_SHORT autolearn=ham version=3.3.2 spammy=Applying, 20gdbccodingstandards, 20GDBCCodingStandards, sk:20gdb-c X-HELO: sesbmg23.ericsson.net Received: from sesbmg23.ericsson.net (HELO sesbmg23.ericsson.net) (193.180.251.37) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 07 Sep 2018 21:26:35 +0000 DKIM-Signature: v=1; a=rsa-sha256; d=ericsson.com; s=mailgw201801; c=relaxed/simple; q=dns/txt; i=@ericsson.com; t=1536355592; h=From:Sender:Reply-To:Subject:Date:Message-ID:To:Cc:MIME-Version:Content-Type: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:In-Reply-To:References:List-Id: List-Help:List-Unsubscribe:List-Subscribe:List-Post:List-Owner:List-Archive; bh=SkEZ+LWaH1nGlplEJj8OIk8N64PLEUkKmVp/CvlsrVo=; b=ENuUn7TMpSeeToIeEOsOtZsUB2BJpwMtGyLyJihrGqIHNPWgZ7HqNsx9xzpLBAV6 7xahKaxl1kd4Tiwjq/Dz6nF10/GCGVvqXvZQG8UcVrav4CCOL5+Om0XCUGm2qoeJ iOs8GFN9RhKp+pUdoH9Mr6+sXG+4YRDTOOCJEyqYfm0=; Received: from ESESSMB502.ericsson.se (Unknown_Domain [153.88.183.120]) by sesbmg23.ericsson.net (Symantec Mail Security) with SMTP id 25.59.05037.80DE29B5; Fri, 7 Sep 2018 23:26:32 +0200 (CEST) Received: from ESESBMR505.ericsson.se (153.88.183.201) by ESESSMB502.ericsson.se (153.88.183.190) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.1466.3; Fri, 7 Sep 2018 23:26:32 +0200 Received: from ESESSMB503.ericsson.se (153.88.183.164) by ESESBMR505.ericsson.se (153.88.183.201) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.1466.3; Fri, 7 Sep 2018 23:26:32 +0200 Received: from NAM03-CO1-obe.outbound.protection.outlook.com (153.88.183.157) by ESESSMB503.ericsson.se (153.88.183.164) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.1466.3 via Frontend Transport; Fri, 7 Sep 2018 23:26:31 +0200 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ericsson.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=mhcd4FBVBhlgZ3ofCsmaIVJC9Z0p1nN6P1Pfx40k9dg=; b=Fz9tsNek5hBbah8MjaQWfZFcXA0NAOy3eWFylxQr567zVVQyj97KSUH4qv+GCuCdVAHmC27LUudRcXhAXSMlS/qPhgaGtllWC92620vCaHZtSR6hDc2HJWmGCQD+PaPzZ06IH4hrVwqV4H99iLC/H5sP8RZ/95PBUI1FH6UxiUY= Authentication-Results: spf=none (sender IP is ) smtp.mailfrom=simon.marchi@ericsson.com; Received: from [100.68.184.40] (94.119.64.15) by SN6PR15MB2398.namprd15.prod.outlook.com (2603:10b6:805:24::18) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.1101.17; Fri, 7 Sep 2018 21:26:28 +0000 Subject: Re: [PATCH v2 PR gdb/16841] virtual inheritance via typedef cannot find base To: Weimin Pan <weimin.pan@oracle.com>, <gdb-patches@sourceware.org> References: <1535067835-60808-1-git-send-email-weimin.pan@oracle.com> From: Simon Marchi <simon.marchi@ericsson.com> Message-ID: <4958cf6a-a9ab-9448-0017-a4317d5a09ad@ericsson.com> Date: Fri, 7 Sep 2018 22:26:20 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <1535067835-60808-1-git-send-email-weimin.pan@oracle.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Return-Path: simon.marchi@ericsson.com Received-SPF: None (protection.outlook.com: ericsson.com does not designate permitted sender hosts) X-IsSubscribed: yes |
Commit Message
Simon Marchi
Sept. 7, 2018, 9:26 p.m. UTC
Hi Weimin, It seems like the typedef is not really a factor here. The bug is present even if you remove the typedef from you test case. Could you update the title of the patch to reflect this? Your patch does not need to have the same title as the PR that motivated it. Testing with a typedef is a good idea though. Your patch still has trailing whitespaces. Try to do: $ git format-patch HEAD^ $ git am 0001-virtual-inheritance-via-typedef-cannot-find-base.patch and make sure you have no message other than Applying: virtual inheritance via typedef cannot find base > diff --git a/gdb/testsuite/gdb.cp/virtbase2.exp b/gdb/testsuite/gdb.cp/virtbase2.exp > new file mode 100644 > index 0000000..c29ff1c > --- /dev/null > +++ b/gdb/testsuite/gdb.cp/virtbase2.exp > @@ -0,0 +1,50 @@ > +# Copyright 2018 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/>. > + > +# Make sure printing virtual base class data member correctly (PR16841) It sounds like it's missing a word... "works correctly"? > + > +if { [skip_cplus_tests] } { continue } > + > +standard_testfile .cc > + > +if {[prepare_for_testing "failed to prepare" $testfile $srcfile {debug c++}]} { > + return -1 > +} > + > +if {![runto_main]} then { > + perror "couldn't run to main" > + continue > +} > + > +gdb_breakpoint "derived::func_d" > +gdb_continue_to_breakpoint "continue to derived::func_d" > +gdb_test "print i" " = 55" "i in base class" > +gdb_test "print derived::i" " = 55" "i in base class" > +gdb_test "print derived::base::i" " = 55" "i in base class" > +gdb_test "print base::i" " = 55" "i in base class" > +gdb_test "print d" " = 6.5999999999999996" "d in base class" > +gdb_test "print derived::d" " = 6.5999999999999996" "d in base class" > +gdb_test "print derived::base::d" " = 6.5999999999999996" "d in base class" > +gdb_test "print base::d" " = 6.5999999999999996" "d in base class" > +gdb_breakpoint "foo::func_f" > +gdb_continue_to_breakpoint "continue to foo::func_f" > +gdb_test "print i" " = 55" "i in base class" > +gdb_test "print derived::i" " = 55" "i in base class" > +gdb_test "print derived::base::i" " = 55" "i in base class" > +gdb_test "print base::i" " = 55" "i in base class" > +gdb_test "print d" " = 6.5999999999999996" "d in base class" > +gdb_test "print derived::d" " = 6.5999999999999996" "d in base class" > +gdb_test "print derived::base::d" " = 6.5999999999999996" "d in base class" > +gdb_test "print base::d" " = 6.5999999999999996" "d in base class" Make sure test names are unique: https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Make_sure_test_messages_are_unique In these cases, I think you can omit the test names, gdb_test will default to use the command as the test name. I think I found another problematic case. If you modify your test like this: ... and try to print the x field with "print derived::x", you get a wrong value. It would be nice to have the test case generate all possible combinations of scopes. For example, with struct base { int x; } struct derived : base {} We should test with - x - base::x - derived::x - derived::base::x > diff --git a/gdb/valops.c b/gdb/valops.c > index 9bdbf22..754e7d0 100644 > --- a/gdb/valops.c > +++ b/gdb/valops.c > @@ -3329,6 +3329,35 @@ compare_parameters (struct type *t1, struct type *t2, int skip_artificial) > return 0; > } > > +/* C++: Given an aggregate type VT, and a class type CLS, > + search recursively for CLS and return its offset, > + relative to VT, if it is a virtual base member. */ Can you describe the return value and the parameter V? > + > +static int Return bool. > +add_virtual_base_offset (struct type *vt, struct type *cls, > + struct value *v, int &boffs) I suggest naming this function find_virtual_base_offset or get_virtual_base_offset, since it doesn't do the actual adding. The last line is too much indented. Please make boffs a pointer: https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#Avoid_non-const_reference_parameters.2C_use_pointers_instead The function comment should mention that the offset is returned in *BOFFS. > +{ > + for (int i = 0; i < TYPE_N_BASECLASSES (vt); i++) > + { > + struct type *ftype = TYPE_FIELD_TYPE (vt, i); > + if (check_typedef (ftype) == cls) > + { > + if (BASETYPE_VIA_VIRTUAL (vt, i)) > + { > + const gdb_byte *adr = value_contents_for_printing (v); > + boffs = baseclass_offset (vt, i, adr, value_offset (v), > + value_as_long (v), v); > + } > + return true; > + } > + > + if (add_virtual_base_offset (ftype, cls, v, boffs)) > + return true; > + } > + > + return false; > +} > + > /* C++: Given an aggregate type CURTYPE, and a member name NAME, > return the address of this member as a "pointer to member" type. > If INTYPE is non-null, then it will be the type of the member we > @@ -3393,6 +3422,15 @@ value_struct_elt_for_reference (struct type *domain, int offset, > tmp = lookup_pointer_type (TYPE_SELF_TYPE (type)); > v = value_cast_pointers (tmp, v, 1); > mem_offset = value_as_long (ptr); > + if (domain != curtype) > + { Can you add a small comment about what this block of code is doing? Simon
Comments
Hi Simon, Thanks very much for your comments. On 9/7/2018 2:26 PM, Simon Marchi wrote: > Hi Weimin, > > It seems like the typedef is not really a factor here. The bug is present even > if you remove the typedef from you test case. Could you update the title of the > patch to reflect this? Your patch does not need to have the same title as the > PR that motivated it. > > Testing with a typedef is a good idea though. > > Your patch still has trailing whitespaces. Try to do: > > $ git format-patch HEAD^ > $ git am 0001-virtual-inheritance-via-typedef-cannot-find-base.patch > > and make sure you have no message other than > > Applying: virtual inheritance via typedef cannot find base It's nice to know this "git am" command. Do I use the "git am --abort" command to restore my original patch, correct problems, then use another "git am" command? >> diff --git a/gdb/testsuite/gdb.cp/virtbase2.exp b/gdb/testsuite/gdb.cp/virtbase2.exp >> new file mode 100644 >> index 0000000..c29ff1c >> --- /dev/null >> +++ b/gdb/testsuite/gdb.cp/virtbase2.exp >> @@ -0,0 +1,50 @@ >> +# Copyright 2018 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/>. >> + >> +# Make sure printing virtual base class data member correctly (PR16841) > It sounds like it's missing a word... "works correctly"? Word "works" has been added. >> + >> +if { [skip_cplus_tests] } { continue } >> + >> +standard_testfile .cc >> + >> +if {[prepare_for_testing "failed to prepare" $testfile $srcfile {debug c++}]} { >> + return -1 >> +} >> + >> +if {![runto_main]} then { >> + perror "couldn't run to main" >> + continue >> +} >> + >> +gdb_breakpoint "derived::func_d" >> +gdb_continue_to_breakpoint "continue to derived::func_d" >> +gdb_test "print i" " = 55" "i in base class" >> +gdb_test "print derived::i" " = 55" "i in base class" >> +gdb_test "print derived::base::i" " = 55" "i in base class" >> +gdb_test "print base::i" " = 55" "i in base class" >> +gdb_test "print d" " = 6.5999999999999996" "d in base class" >> +gdb_test "print derived::d" " = 6.5999999999999996" "d in base class" >> +gdb_test "print derived::base::d" " = 6.5999999999999996" "d in base class" >> +gdb_test "print base::d" " = 6.5999999999999996" "d in base class" >> +gdb_breakpoint "foo::func_f" >> +gdb_continue_to_breakpoint "continue to foo::func_f" >> +gdb_test "print i" " = 55" "i in base class" >> +gdb_test "print derived::i" " = 55" "i in base class" >> +gdb_test "print derived::base::i" " = 55" "i in base class" >> +gdb_test "print base::i" " = 55" "i in base class" >> +gdb_test "print d" " = 6.5999999999999996" "d in base class" >> +gdb_test "print derived::d" " = 6.5999999999999996" "d in base class" >> +gdb_test "print derived::base::d" " = 6.5999999999999996" "d in base class" >> +gdb_test "print base::d" " = 6.5999999999999996" "d in base class" > Make sure test names are unique: > https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Make_sure_test_messages_are_unique > > In these cases, I think you can omit the test names, gdb_test will default > to use the command as the test name. I'm going to use the new virtbase2.exp that you suggested in your separate email which, exercise more possible combinations of scopes as you described below, and reveal more test failures (: Looking into these problems now. > I think I found another problematic case. If you modify your test like this: > > diff --git a/gdb/testsuite/gdb.cp/virtbase2.cc b/gdb/testsuite/gdb.cp/virtbase2.cc > index 4f7631e..4620ef5 100644 > --- a/gdb/testsuite/gdb.cp/virtbase2.cc > +++ b/gdb/testsuite/gdb.cp/virtbase2.cc > @@ -1,4 +1,9 @@ > -struct base { > +struct superbase { > + int x; > + superbase () : x(22) {} > +}; > + > +struct base : superbase { > int i; double d; > base() : i(55), d(6.6) {} > }; > > > ... and try to print the x field with "print derived::x", you get a wrong value. Yes, "print derived::x" shows a wrong value while "print x" and "print derived::base::x" work correctly. > It would be nice to have the test case generate all possible combinations of scopes. > For example, with > > struct base { int x; } > struct derived : base {} > > We should test with > > - x > - base::x > - derived::x > - derived::base::x > >> diff --git a/gdb/valops.c b/gdb/valops.c >> index 9bdbf22..754e7d0 100644 >> --- a/gdb/valops.c >> +++ b/gdb/valops.c >> @@ -3329,6 +3329,35 @@ compare_parameters (struct type *t1, struct type *t2, int skip_artificial) >> return 0; >> } >> >> +/* C++: Given an aggregate type VT, and a class type CLS, >> + search recursively for CLS and return its offset, >> + relative to VT, if it is a virtual base member. */ > Can you describe the return value and the parameter V? Will do. Tom also has pointed this out. > + > +static int > Return bool. > >> +add_virtual_base_offset (struct type *vt, struct type *cls, >> + struct value *v, int &boffs) > I suggest naming this function find_virtual_base_offset or get_virtual_base_offset, > since it doesn't do the actual adding. Will change the function name to get_virtual_base_offset. Tom made a similar comment. > The last line is too much indented. Fixed. > > Please make boffs a pointer: Done, Tom also suggested it. > > https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#Avoid_non-const_reference_parameters.2C_use_pointers_instead > > The function comment should mention that the offset is returned in *BOFFS. Done, Tom made the same comment. >> +{ >> + for (int i = 0; i < TYPE_N_BASECLASSES (vt); i++) >> + { >> + struct type *ftype = TYPE_FIELD_TYPE (vt, i); >> + if (check_typedef (ftype) == cls) >> + { >> + if (BASETYPE_VIA_VIRTUAL (vt, i)) >> + { >> + const gdb_byte *adr = value_contents_for_printing (v); >> + boffs = baseclass_offset (vt, i, adr, value_offset (v), >> + value_as_long (v), v); >> + } >> + return true; >> + } >> + >> + if (add_virtual_base_offset (ftype, cls, v, boffs)) >> + return true; >> + } >> + >> + return false; >> +} >> + >> /* C++: Given an aggregate type CURTYPE, and a member name NAME, >> return the address of this member as a "pointer to member" type. >> If INTYPE is non-null, then it will be the type of the member we >> @@ -3393,6 +3422,15 @@ value_struct_elt_for_reference (struct type *domain, int offset, >> tmp = lookup_pointer_type (TYPE_SELF_TYPE (type)); >> v = value_cast_pointers (tmp, v, 1); >> mem_offset = value_as_long (ptr); >> + if (domain != curtype) >> + { > Can you add a small comment about what this block of code is doing? Will do. Thanks again, Weimin > > Simon
On 2018-09-08 01:12, Weimin Pan wrote: >> Your patch still has trailing whitespaces. Try to do: >> >> $ git format-patch HEAD^ >> $ git am 0001-virtual-inheritance-via-typedef-cannot-find-base.patch >> >> and make sure you have no message other than >> >> Applying: virtual inheritance via typedef cannot find base > > It's nice to know this "git am" command. Do I use the "git am --abort" > command to > restore my original patch, correct problems, then use another "git am" > command? You shouldn't need to use --abort in this case... In my instructions, I forgot to mention to remove your commit before applying it again (it's expected that applying the same patch twice doesn't work). So it should be: $ git format-patch HEAD^ $ git reset --hard HEAD^ $ git am 0001-virtual-inheritance-via-typedef-cannot-find-base.patch Modifications to the patch can be done by using "git commit --amend", as usual. Thanks! Simon
diff --git a/gdb/testsuite/gdb.cp/virtbase2.cc b/gdb/testsuite/gdb.cp/virtbase2.cc index 4f7631e..4620ef5 100644 --- a/gdb/testsuite/gdb.cp/virtbase2.cc +++ b/gdb/testsuite/gdb.cp/virtbase2.cc @@ -1,4 +1,9 @@ -struct base { +struct superbase { + int x; + superbase () : x(22) {} +}; + +struct base : superbase { int i; double d; base() : i(55), d(6.6) {} };