From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 4354 invoked by alias); 8 Sep 2018 00:12:24 -0000 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 Received: (qmail 4343 invoked by uid 89); 8 Sep 2018 00:12:24 -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,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy= X-HELO: aserp2120.oracle.com Received: from aserp2120.oracle.com (HELO aserp2120.oracle.com) (141.146.126.78) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sat, 08 Sep 2018 00:12:22 +0000 Received: from pps.filterd (aserp2120.oracle.com [127.0.0.1]) by aserp2120.oracle.com (8.16.0.22/8.16.0.22) with SMTP id w8809lRm002621; Sat, 8 Sep 2018 00:12:17 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=oracle.com; h=subject : to : references : from : message-id : date : mime-version : in-reply-to : content-type : content-transfer-encoding; s=corp-2018-07-02; bh=8A1/W/qAiRXoulXMhaSt0V1mu8Jm78xZ6K4I+xPYWXU=; b=0xIFIQiP0jEOxNyew0qYv7RtC6yuylll2aReEgSwJpFcDHtBJVtLU6EnMhzwQ2akzI49 1HHCh4oCLRzXycCW1mIsb4lPc098F2pgOFD313dF8QpLJwL82R0Kcfe93OmaRySTRYhV Z0um9LU8J4nnAU1JxywFQSepz1vvyVeFOLSR0xuX5MPUrAb9aiZbKs1vV9aAah5p21Qu jGeYMdQ9buDHS755bIHOUpkVPa5yr8np+X7exsjG/xbKOKtJxxbeC6R1tZwgCjUbIpIc HS1nBlKUgXgnyBomaWNNxDHsivM9LpZwd7QziqV24iTpNYHVaJTY1L4Wur4sWJ8JPdYl kw== Received: from userv0022.oracle.com (userv0022.oracle.com [156.151.31.74]) by aserp2120.oracle.com with ESMTP id 2m7jqq57cj-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Sat, 08 Sep 2018 00:12:17 +0000 Received: from aserv0121.oracle.com (aserv0121.oracle.com [141.146.126.235]) by userv0022.oracle.com (8.14.4/8.14.4) with ESMTP id w880CGgw017257 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Sat, 8 Sep 2018 00:12:16 GMT Received: from abhmp0017.oracle.com (abhmp0017.oracle.com [141.146.116.23]) by aserv0121.oracle.com (8.14.4/8.13.8) with ESMTP id w880CFBb031790; Sat, 8 Sep 2018 00:12:15 GMT Received: from [10.132.96.98] (/10.132.96.98) by default (Oracle Beehive Gateway v4.0) with ESMTP ; Sat, 08 Sep 2018 00:12:15 +0000 Subject: Re: [PATCH v2 PR gdb/16841] virtual inheritance via typedef cannot find base To: Simon Marchi , gdb-patches@sourceware.org References: <1535067835-60808-1-git-send-email-weimin.pan@oracle.com> <4958cf6a-a9ab-9448-0017-a4317d5a09ad@ericsson.com> From: Weimin Pan Message-ID: <4864ea95-37cc-a323-c330-7d87aee92dde@oracle.com> Date: Sat, 08 Sep 2018 00:12:00 -0000 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <4958cf6a-a9ab-9448-0017-a4317d5a09ad@ericsson.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-SW-Source: 2018-09/txt/msg00144.txt.bz2 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 . >> + >> +# 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