From patchwork Fri Sep 7 21:26:20 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Simon Marchi X-Patchwork-Id: 29234 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: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , 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 , References: <1535067835-60808-1-git-send-email-weimin.pan@oracle.com> From: Simon Marchi 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> Return-Path: simon.marchi@ericsson.com Received-SPF: None (protection.outlook.com: ericsson.com does not designate permitted sender hosts) X-IsSubscribed: yes 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 . > + > +# 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 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) {} };